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

trace: When a span's explicit parent is disabled, the parent should probably be contextual #88

Open
hawkw opened this issue May 28, 2019 · 2 comments
Labels
crate/tracing Related to the `tracing` crate kind/feature New feature or request meta/breaking This is a breaking change, and should wait until the next breaking release.
Milestone

Comments

@hawkw
Copy link
Member

hawkw commented May 28, 2019

In tokio-trace, when constructing a span using Span::child_of, we accept a type that can provide us an Option<&Id> as the span's parent. This allows writing code like this:

let foo = span!(Level::TRACE, ...);
let bar = span!(Level::INFO, parent: &foo, ...);

It's necessary to take an Option<&Id> here rather than just &Id, as the parent span may not be enabled --- for example, in the snippet above, we might have a filter set that accepts spans with Level::DEBUG and lower, so foo would be disabled.

Currently, if the parent span is disabled, we call Attributes::new_root:
https://github.com/tokio-rs/tokio/blob/84d5a7f5a0660dbe1e2be0e831e715f13cb59a2b/tokio-trace/src/span.rs#L251-L254

I think this behaviour is fairly surprising. Consider the following:

let foo = info_span!(...);
let _guard = foo.enter();

let bar = trace_span!(...);
let baz = info_span!(parent: &bar, ...);

If we run this code with the max verbosity level set to INFO, bar is disabled, so baz's parent is None, making it the root of a new trace tree. This seems incorrect to me --- I'd only expect a span to be a root if I explicitly set it to not have a parent. Instead, in the case where the explicitly set parent is disabled, I'd expect the span to have a contextual parent.

@carllerche carllerche transferred this issue from tokio-rs/tokio Jun 24, 2019
@hawkw hawkw added crate/tracing Related to the `tracing` crate kind/feature New feature or request labels Jun 24, 2019
@hawkw hawkw changed the title trace: When a span's explicit parent is disabled, the parent should probably be contetual trace: When a span's explicit parent is disabled, the parent should probably be contextual Jun 26, 2019
@hawkw
Copy link
Member Author

hawkw commented Jun 26, 2019

This may require us to think a bit more about the syntax for explicit root spans in the macros. We can no longer use parent: None to indicate an explicit root, since we want to match the parent argument as an expression. We could consider adding a new "keyword" to the macros, like parent: root, instead --- it would be great to get some input from folks on syntax before making this change.

@hawkw
Copy link
Member Author

hawkw commented Jun 26, 2019

@carllerche, @LucioFranco, @davidbarsky, @jonhoo, any thoughts on the syntax question above? If we're making a breaking change here, it would be good to get it in before tracing 0.1.

@hawkw hawkw modified the milestone: tracing 0.1 Jun 26, 2019
@hawkw hawkw added the meta/breaking This is a breaking change, and should wait until the next breaking release. label Apr 9, 2020
@hawkw hawkw added this to the tracing 0.2 milestone Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/tracing Related to the `tracing` crate kind/feature New feature or request meta/breaking This is a breaking change, and should wait until the next breaking release.
Projects
None yet
Development

No branches or pull requests

1 participant