feat(bottlecap): Add Composite Trace Propagator#413
Conversation
also updated unit tests, as we have custom behavior, we should check only the fields we care about in the tests
also known as our internal http propagator, but in reality, http doesnt make any sense to me, its just a composite propagator which we used based on our configuration
also updated the naming
duncanista
changed the base branch from
main
to
jordan.gonzalez/bottlecap/universal-instrumentation
logic was wrong in some parts, this was discovered through unit tests
also corrected some logic
alexgallotta
left a comment
alexgallotta
left a comment
There was a problem hiding this comment.
Great job overall!
I would argue that in a lot of places struct/traits could be avoided and make the code easier, more readable, easier to test and shorter though
| (contexts, styles) | ||
| } | ||
|
|
||
| fn resolve_contexts( |
There was a problem hiding this comment.
can this method be outside of the struct implementation?
There was a problem hiding this comment.
Since this is the only place being used, I'd rather keep it here, there's no other place where this is being used. What would be the benefit of keeping it outside of the struct?
| primary_context | ||
| } | ||
|
|
||
| fn attach_baggage(context: &mut SpanContext, carrier: &dyn Extractor) { |
There was a problem hiding this comment.
can this method be outside of the struct implementation?
There was a problem hiding this comment.
Same question as the other comment
|
|
||
| impl DatadogCompositePropagator { | ||
| #[must_use] | ||
| pub fn new(config: Arc<config::Config>) -> Self { |
There was a problem hiding this comment.
If also this method is removed, there is no need of the impl altogether.
It's a bit heavy for a constructor to have all this logic, it could be a builder/function
There was a problem hiding this comment.
How would you modify this? I decided to follow this because of the behavior changes on the configuration, and to save the state of the available propagators, at the end of the day, it will be very cheap in computation
There was a problem hiding this comment.
I took some time to modify it and give it an idea of what I thought
#418
I think it's already a great job and there is a lot of business logic I am not familiar with!
My take, from what I've read, is to use abstractions and traits/struct very sparingly and when they are really worth it.
Usually better start with basic function and modules most of the time. It helps to also see things differently when the struct impl is not around, and when refactoring or changing something you don't need to change 10 fields all around the code/function initialization!
That PR is not necessarily to merge or continue working on, just to show how it could look like with a different approach
duncanista
merged commit b5a049a
into
jordan.gonzalez/bottlecap/universal-instrumentation
duncanista
deleted the
jordan.gonzalez/bottlecap/add-composite-propagator
branch
* add `trace_propagation_style.rs` * add Trace Propagation to `config.rs` also updated unit tests, as we have custom behavior, we should check only the fields we care about in the tests * add `links` to `SpanContext` * add composite propagator also known as our internal http propagator, but in reality, http doesnt make any sense to me, its just a composite propagator which we used based on our configuration * update `TextMapPropagator`s to comply with interface also updated the naming * fmt * add unit testing for `config.rs` * add `PartialEq` to `SpanContext` * correct logic from `text_map_propagator.rs` logic was wrong in some parts, this was discovered through unit tests * add unit tests for `DatadogCompositePropagator` also corrected some logic
* add `trace_propagation_style.rs` * add Trace Propagation to `config.rs` also updated unit tests, as we have custom behavior, we should check only the fields we care about in the tests * add `links` to `SpanContext` * add composite propagator also known as our internal http propagator, but in reality, http doesnt make any sense to me, its just a composite propagator which we used based on our configuration * update `TextMapPropagator`s to comply with interface also updated the naming * fmt * add unit testing for `config.rs` * add `PartialEq` to `SpanContext` * correct logic from `text_map_propagator.rs` logic was wrong in some parts, this was discovered through unit tests * add unit tests for `DatadogCompositePropagator` also corrected some logic
* add `trace_propagation_style.rs` * add Trace Propagation to `config.rs` also updated unit tests, as we have custom behavior, we should check only the fields we care about in the tests * add `links` to `SpanContext` * add composite propagator also known as our internal http propagator, but in reality, http doesnt make any sense to me, its just a composite propagator which we used based on our configuration * update `TextMapPropagator`s to comply with interface also updated the naming * fmt * add unit testing for `config.rs` * add `PartialEq` to `SpanContext` * correct logic from `text_map_propagator.rs` logic was wrong in some parts, this was discovered through unit tests * add unit tests for `DatadogCompositePropagator` also corrected some logic
* add `trace_propagation_style.rs` * add Trace Propagation to `config.rs` also updated unit tests, as we have custom behavior, we should check only the fields we care about in the tests * add `links` to `SpanContext` * add composite propagator also known as our internal http propagator, but in reality, http doesnt make any sense to me, its just a composite propagator which we used based on our configuration * update `TextMapPropagator`s to comply with interface also updated the naming * fmt * add unit testing for `config.rs` * add `PartialEq` to `SpanContext` * correct logic from `text_map_propagator.rs` logic was wrong in some parts, this was discovered through unit tests * add unit tests for `DatadogCompositePropagator` also corrected some logic
2 participants
What?
Creates a composite propagator to extract trace context.
This is because we have specific logic on how we handle/resolve trace context in the tracers.
How?
DatadogCompositePropagatorusingDatadogHeaderPropagatorandTraceContextPropagator.Motivation
SVLS-5769 and...
Notes