feature(bottlecap): add trace context extractor#401
Conversation
added `Datadog` and `Tracecontext` implementations
duncanista
changed the base branch from
main
to
jordan.gonzalez/bottlecap/universal-instrumentation
…to jordan.gonzalez/bottlecap/add-trace-propagator
alexgallotta
left a comment
alexgallotta
left a comment
There was a problem hiding this comment.
Looks amazing! Great job! Left 2 comments on some stuff that might be removed and avoid some extra code/layering
| pub struct TraceparentPropagator; | ||
|
|
||
| impl TextMapPropagator for TraceparentPropagator { | ||
| fn extract(&self, carrier: &dyn Extractor) -> SpanContext { |
There was a problem hiding this comment.
do we really need struct + trait + methods here for traceparent and tracestate propagators?
Can we get away with extract_parent and extract_state?
There was a problem hiding this comment.
traceparent and tracestate propagators
Do you mean TheTraceparentpropagator which includse the extraction of thetracestate+traceparent?
If so, I tried using this struct + trait + methods in order to make the code easier to maintain, and also following the current OTel implemenetation, which I think was simple enough to understand, what do you think?
| /// Injector provides an interface for a carrier to be used | ||
| /// with a Propagator to inject a Context into the carrier. | ||
| /// | ||
| pub trait Injector { |
There was a problem hiding this comment.
Same for these 2. Can we avoid/postpone creating extra traits?
There was a problem hiding this comment.
This allows us to directly implement this traits for carriers which we might not have control of, for example, here I tend to convert hyper::HeaderMap into a HashMap so I can use the extract and inject methods on the struct right away.
At the top of the file I explain that I directly copied this from the OTel rust implementation for extractor/injectors.
I think this would help us whenever we have to expand, and its also very generic, what do you think of this?
astuyve
left a comment
astuyve
left a comment
There was a problem hiding this comment.
couple todos but nothing looks particularly wrong to me here
| } | ||
|
|
||
| fn inject(&self, _context: SpanContext, _carrier: &mut dyn Injector) { | ||
| todo!(); |
There was a problem hiding this comment.
Yeah, this ones i'm not going to particularly adress in this PR, since we don't care about injection yet, maybe I mark them as that they're going to panic since they are not implemented?
* add `thiserror` and `lazystatic` * add Span/Trace `context` * update `mod.rs` * add `propagation` module * add `propagation::Error` * add interface for `carrier` and `HashMap` implementation * add `text_map_propagator` added `Datadog` and `Tracecontext` implementations * update `LICENSE-3rdparty.yml`
* add `thiserror` and `lazystatic` * add Span/Trace `context` * update `mod.rs` * add `propagation` module * add `propagation::Error` * add interface for `carrier` and `HashMap` implementation * add `text_map_propagator` added `Datadog` and `Tracecontext` implementations * update `LICENSE-3rdparty.yml`
* add `thiserror` and `lazystatic` * add Span/Trace `context` * update `mod.rs` * add `propagation` module * add `propagation::Error` * add interface for `carrier` and `HashMap` implementation * add `text_map_propagator` added `Datadog` and `Tracecontext` implementations * update `LICENSE-3rdparty.yml`
3 participants
What?
Adds an implementation of trace context propagator, currently only focusing on extraction.
Motivation
Inferred spans and SVLS-5289
Notes