davidbarsky / tracing-tree

Apache License 2.0
124 stars 31 forks source link

Print the span info on span exit and entry #16

Closed oli-obk closed 4 years ago

oli-obk commented 4 years ago

I may have went a bit overboard with this one. I wanted to somehow render the fact that we're just re-emitting the current span info

Screenshot from 2020-08-17 15-19-16

oli-obk commented 4 years ago

Do you mean just the current duration from the start, or the duration the span was open? In the latter case we have to store the opening somewhere, any tips?

davidbarsky commented 4 years ago

I'm sorry my message wasn't more clear, let me try to rephrase. I think that there are a few distinct requests that I didn't properly separate.

In the latter case we have to store the opening somewhere, any tips?

Sure! Here's the approach I was thinking of:

Again, the above approach can be done in a followup, but I'd like to hash out an approach first in an issue. I really love this PR as is and I'd be happy to merge it as-is, as I don't want to let perfect be the enemy of good 😄.

oli-obk commented 4 years ago

We have a few subtle ways to signal that something is different. One would be to use some of the bold or double box drawing chars from https://en.wikipedia.org/wiki/Box-drawing_character This could look like

│┌┘
â•žâ•›

(without the space between the lines, that's just github)

Also we can use bold lines instead of double lines.

I'll open an issue for the timings

davidbarsky commented 4 years ago

We have a few subtle ways to signal that something is different. One would be to use some of the bold or double box drawing chars from https://en.wikipedia.org/wiki/Box-drawing_character. This could look like [redacted]

That's a reasonable approach. For what it's worth, I think this PR is fine as-is. We can add some specific formatting for a span closing, if there's a need for it. I think that printing:

will allow us to distinguish between an exited span and a closed span, respectively.

oli-obk commented 4 years ago

For what it's worth, I think this PR is fine as-is.

ok :D then let's merge it for now and figure out the details about other improvements in the issue.