Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
task: add Tracing instrumentation to spawned tasks #2655
task: add Tracing instrumentation to spawned tasks #2655
Changes from 8 commits
2f03820
4239548
a190eae
d4e4aef
bd4068e
5136e1f
7b40188
093398c
468ed2d
e4f421f
59fec64
f749552
ba638df
d4391cd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked about this, but I don't think
tracing
merits being placed behind thetokio_unstable
unstable flag. @carllerche will need to weigh in on this, but unlike theCancelationToken
, changes to thetracing
output aren't what the Tokio project has historically considered to be a "breaking change". An off-by-default (e.g., excluded fromfull
) feature flag seems like a reasonable default.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at one point, @carllerche suggested that any dependency on a pre-1.0 crate should require the
tokio_unstable
flag...but, in this case, we aren't exposing anything fromtracing
in Tokio's public APIs. AFAICT, it shouldn't be possible for thetracing
dependency to cause any compile time breakage --- the worst failure mode, if there is eventually a breaking change to thetracing-core
crate that we can't work around, is that the tracing diagnostics might silently fail to be collected.IMO, you're right, and an off-by-default optional dep should be fine. That would be pretty similar to what we do for
parking_lot
, which is also pre-1.0 (and churns versions somewhat frequently!), since it's also not exposed publicly. Of course, if we were to exposetracing
types (such asSpan
orDispatch
) in a public API later on, those APIs should require the unstable flag.With all of that said: I made the more conservative choice of opening this PR with the
tokio_unstable
flag required, because I want to bias towards getting this merged sooner rather than later, and continuing to iterate once it merges. We can always remove the flag in a follow-up, but it's harder to add if these changes ship in a published release...