feat: add baggage API #179
Conversation
| @@ -435,7 +435,6 @@ bool TraceSegment::inject(DictWriter& writer, const SpanData& span, | |||
| additional_w3c_tracestate_)); | |||
| break; | |||
| default: | |||
There was a problem hiding this comment.
- reset and continue on
PropagationStyle::BAGGAGE.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
==========================================
+ Coverage 93.12% 93.35% +0.22%
==========================================
Files 74 76 +2
Lines 4290 4558 +268
==========================================
+ Hits 3995 4255 +260
- Misses 295 303 +8 ☔ View full report in Codecov by Sentry. |
dmehala
force-pushed
the
dmehala/baggage-api
branch
from
e3394c9 to
4328f39
Compare
src/datadog/baggage.cpp
Outdated
| if (baggage_.empty()) return {}; | ||
| if (baggage_.size() > opts.max_items) | ||
| return datadog::tracing::Error{ | ||
| datadog::tracing::Error::Code::BAGGAGE_MAXIMUM_BYTES_REACHED, ""}; |
There was a problem hiding this comment.
Our RFC states that when we exceed the configured item/bytes limits upon injection, the the APM SDKs should drop baggage name/value pairs until under the limit (and then continue to send the new value). Can you double-check with @lucaspimentel or @rachelyangdog to confirm whether this really is optional (SHOULD) or whether this behavior is required (MUST)?
There was a problem hiding this comment.
The W3C spec says:
- tracer MUST send all items if both conditions are met (max number of items, and max size in bytes)
- if either condition it not met, tracer MAY drop items until conditions are met (which items are dropped or in which order is undefined)
- only propagate complete items, never propagate partial items (e.g. key without value)
There is no mention of dropping the entire header, and I don't think we should ever to that.
There was a problem hiding this comment.
addressed in 5b8f0b9
| } break; | ||
|
|
||
| case state::trailing_spaces_value: { | ||
| if (c == ',') { |
There was a problem hiding this comment.
I'm noticing that this algorithm doesn't account for additional property items in baggage, as described in the format here. That is, if there is a baggage key-value pair, then OWS, then a ; to begin a list of properties, it will be considered malformed. While I do not advocate that we must support this, we should make sure that we are handling this consistently across tracers and author a corresponding system-tests case.
@lucaspimentel @rachelyangdog has this been clearly documented in our system-tests?
There was a problem hiding this comment.
When we wrote the RFC, we decided not to support properties (aka metadata) yet since OpenTelemetry themeselves have not fully adopted it. For example, the metadata parameter in the Set API is optional in the spec. Some implementations of the OpenTelemetry API support properties, but others don't. See open-telemetry/opentelemetry-specification#3972 (comment).
We should've been more clear about this in our RFC. If we are not going to support properties yet, we should probably ignoring them (not extract them). If we ignore them, for a baggage header like this:
baggage: key1=value1;property1;property2, key2 = value2, key3=value3; propertyKey=propertyValue
We should extract:
key1=value1
key2=value2
key3=value3
Related: in .NET we are not ignoring properties (yet?), so we end up (accidentally) extracting them as part of the value instead of separately:
key1=value1;property1;property2
key2=value2
key3=value3; propertyKey=propertyValue
This will be problematic because when we inject those values back into a baggage header, we will end up encoding the semi-colons:
baggage: key1=value1%3Bproperty1%3Bproperty2,key2=value2,key3=value3%3BpropertyKey=propertyValue
This will result in the properties being part of the values for any other downstream service instead of being separate metadata, which is... not great.
There was a problem hiding this comment.
addressed in 5b8f0b9
There was a problem hiding this comment.
Overall this looks good but I would like some confirmation on some edge-cases I referenced in new comment threads.
Once you confirm that the implementation is in line with the expected cross-tracer expectations on these edge-cases, I'll be happy to approve. Thanks!
|
@zacharycmontoya, thank you for reviewing. Your feedback really helped clarify the RFC and the expected behavior. I appreciate it. |
Description
Add a baggage API.
Todo:
Additional Notes
Jira ticket: APMAPI-196