-
Notifications
You must be signed in to change notification settings - Fork 460
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
Update trace flags to match spec #565
Conversation
Remove all trace flags except `sampled` as that is the only supported flag in the current trace spec. Removes `SpanContext::is_deferred` and `SpanContex::is_debug` as they are also no longer supported.
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.
Looks good! Just a small nit: I wonder if use !TraceFlags::SAMPLED
in tests will be better than TraceFlags::default()
as readers don't need to figure out what's the default of TraceFlags
?
Codecov Report
@@ Coverage Diff @@
## main #565 +/- ##
=====================================
Coverage 54.6% 54.7%
=====================================
Files 98 98
Lines 8550 8576 +26
=====================================
+ Hits 4674 4694 +20
- Misses 3876 3882 +6
Continue to review full report at Codecov.
|
@TommyCpp yeah little confusing. As |
This seems like an improvement. Would you consider adding documentation on SpanContext and on TraceFlags::SAMPLED that mentions that some tracing tools will completely ignore spans that don't have the sampled flag set? |
@@ -371,6 +372,9 @@ impl FromStr for TraceState { | |||
} | |||
|
|||
/// Immutable portion of a `Span` which can be serialized and propagated. | |||
/// | |||
/// Spans that do not have the `sampled` flag set in their [`TraceFlags`] will | |||
/// be ignored by most tracing tools. |
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.
Perfect, thanks!
@TommyCpp I can think more about test clarity and do a follow up PR if a better solution emerges there. |
Remove all trace flags except
sampled
as that is the only supported flag in the current trace spec. RemovesSpanContext::is_deferred
andSpanContext::is_debug
as they are also no longer supported.Resolves #559