Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

subscriber: make Compact formatter not be useless #1069

Merged
merged 4 commits into from Oct 26, 2020
Merged

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Oct 26, 2020

Depends on #1067

Motivation

Currently, the Compact event formatter in tracing-subscriber
is...kind of bad, and nobody seems to use it. It outputs the names of
all the spans in the current context, and (inexplicably) the names of
all the fields on the current span, but not their values. This isn't
very useful.

Solution

This branch rewrites the Compact formatter to be a little less bad.
Now, we output all the fields from the current span context, but skip
span names, and we shorten the level to a single character when it's
enabled. Additionally, using the compact formatter disables targets by
default. Now, the lines are nicely short, but maybe still useful.

Example output before this change:
Screenshot_20201024_122857
...and after:
Screenshot_20201024_133352

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested a review from a team as a code owner October 26, 2020 19:31
@hawkw
Copy link
Member Author

hawkw commented Oct 26, 2020

this PR was already approved by @davidbarsky as #1068, but I accidentally merged that into my dev branch for #1067 instead of into master (my bad). I'm just going to go ahead and merge this once CI passes, since it was already approved.

@hawkw hawkw merged commit 70d55af into master Oct 26, 2020
@hawkw hawkw deleted the eliza/good-compact branch October 26, 2020 21:06
@ForsakenHarmony
Copy link

ForsakenHarmony commented Nov 4, 2020

So this no longer displays the current span at all?

Any way to have this, but with span names?

@hawkw
Copy link
Member Author

hawkw commented Nov 4, 2020

@ForsakenHarmony it displays the fields on all spans in the current trace. We could add span names, or the current span's name, as well. How would you like it to be formatted?

@ForsakenHarmony
Copy link

@hawkw sorry to only get back to you now, to me the current span / whole tree of spans is one of the most important parts, maybe there could be an option to configure adding them like the previously were?

@hawkw
Copy link
Member Author

hawkw commented Dec 21, 2020

@hawkw sorry to only get back to you now, to me the current span / whole tree of spans is one of the most important parts, maybe there could be an option to configure adding them like the previously were?

Okay, what do you think about something like this:
Screenshot_20201221_105854

With this format, do you think it's sufficiently clear which fields belong to which span? Would you prefer the spans to be printed first, the way the default formatter does?

@ForsakenHarmony
Copy link

Hmmm, yes I think I'd prefer the spans printed first, I personally don't have a big need to know where a field comes from, as it should still be relevant for the current span.

Thanks for looking into it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants