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

Add all named fields to the span name #7

Closed
wants to merge 3 commits into from

Conversation

superdump
Copy link
Contributor

This makes spans much more useful for me so I can see all the additional metadata in Tracy. Particularly having a span in bevy with a name of "system" and a field like name = system.name(), with this PR I can tell what systems are what in Tracy. I'm open to input for how to improve it.

@nagisa
Copy link
Owner

nagisa commented Nov 4, 2020

Can you please add a test that makes sure both branches are exercised?

@superdump
Copy link
Contributor Author

Done. I ran cargo fmt as well.

@nagisa
Copy link
Owner

nagisa commented Nov 4, 2020

I won't be able to look at/verify/check this for a couple more days, but I'm keeping the tab open as a reminder. If I don't get back to this please ping me.

Copy link
Owner

@nagisa nagisa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried running the newly added tests locally and checked them in Tracy, but I'm failing to see the change in behaviour I would intuitively expect from this PR. Am I missing something?

EDIT: to elaborate, I was expecting to see the name = test to show up somewhere in the trace, but I'm not and the span that gets recorded is named just "a sec" still.

A screenshot

let name = if let Some(fields) = span_data.extensions().get::<FormattedFields<F>>() {
format!("{}: {}", metadata.name(), fields.fields.as_str())
} else {
metadata.name().to_string()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a Cow<'_, str> here, perhaps? It would also make sense to me to lift and duplicate the Span::new call into the conditional here.

@nagisa
Copy link
Owner

nagisa commented Aug 29, 2021

This has been adapted and merged into main branch. Will be released shortly.

@nagisa nagisa closed this Aug 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants