-
Notifications
You must be signed in to change notification settings - Fork 436
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
implement w3c trace context response propagation #998
implement w3c trace context response propagation #998
Conversation
global::set_text_map_propagator(TraceContextPropagator::new()); | ||
|
||
// Install stdout exporter pipeline to be able to retrieve the collected spans. | ||
// For the demonstration, use `Sampler::AlwaysOn` sampler to sample all traces. In a production |
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.
nit: do we need to explicitly set AlwaysOn here? The default would be parent_based(ALwaysOn), which should be good enough for this example.
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.
IMO this is fine, it clearly illustrates that AlwaysOn
is being used for demonstration purposes only, it ensures it always gets logged to stdout no matter what happens in the client
example, for a demo this makes sense to me.
@cijothomas I'll be honest, for the example, I simply copied over what's in |
I'm also not sure what's up with the CI/coverage job, it works fine locally 🤔 |
np! I will take a look at the existing examples and propose some improvements. (I am totally new to Rust and this Repo :)) |
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #998 +/- ##
=====================================
Coverage 69.9% 69.9%
=====================================
Files 116 116
Lines 9027 9027
=====================================
Hits 6316 6316
Misses 2711 2711 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
9122b53
to
aeaca27
Compare
|
||
let cx = Context::current_with_span(span); | ||
|
||
cx.span().add_event("handling this...", Vec::new()); |
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.
cx.span().add_event("handling this...", Vec::new()); | |
span.add_event("handling this...", Vec::new()); |
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.
This change would also require importing opentelemetry::trace::Span
which would make span
a BoxedSpan
as opposed to a SpanRef
, which also means making span
mutable because BoxedSpan::add_event
takes a mutable reference, and moving the definition of cx
further down because Context::current_with_span
moves the span, and BoxedSpan
doesn't implement Clone
.
I don't mind doing this change, but is it really necessary, when the purpose here is to illustrate how traceresponse
works?
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.
No need of changing. (I am Rust newbie, so my comments could be wrong!)
Co-authored-by: Cijo Thomas <[email protected]>
Adds a
TraceContextResponsePropagator
to the contrib crate implementing the W3C Trace Context Response Header.Fixes #523