fix: observe baggage limits on extraction#313
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c50e01910
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| // If the "dd" vendor entry's value exceeds 512 bytes, drop it and record a propagation error tag. | ||
| if (datadog_value.size() > 512) { |
There was a problem hiding this comment.
FYI: We only parse and manipulate our section of the W3C tracestate header, so always pass through the entries from other vendors but parse our portion or discard it if it exceeds the configured limits
There was a problem hiding this comment.
(Apologies for my verbosity, but I like to talk through the thought process when I'm reviewing urgent changes in codebases I don't know well)
baggage.h / .cpp
We're introducing two separate limits, max_bytes (total payload size) and max_items (total number of key/value pairs parsed). These limits are now enforced in parse_baggage.
max_bytes gets checked up-front with a simple length check on the input string: if the header value exceeds the maximum size, we reject it outright and perform no parsing. Simple enough.
max_items gets checked any time we're about to push a newly-parsed value into the result map. There are three existing places where the function calls result.emplace (and it doens't appear to mutate result in any other way, so this list oughta be exhaustive):
- In the
consume_valuebranch of the state machine logic, after we've reached the end of a value in the string - In the post-state-machine code that handles the last value, if we've concluded parsing in the
valuestate, meaning we have a final value to add to the map - In the same code, just in the
trailing_spaces_value || propertiesstate, which has slightly different arithmetic for grabbing the portion of the string that represents the value, but is otherwise duplicated (duplication which is extremely minor and would be silly to refactor in an urgent bug-fix change)
...so in each of those three places, we now have preemptive checks for result.size() >= max_items. This will correctly allow the case where we have max_items values, and abort with an error in any conceivable case where we could otherwise accumulate more than max_items.
Validation logic looks correct to me.
parse_baggage is internal to baggage.cpp, so the extra parameter is fine. Baggage::extract is public, but the newly-added parameter is declared with a default so it won't break usage. The Codex review note re: this nevertheless breaking linkage seems worth considering, but I don't know what level of binary compatibility we're concerned with here, so I'll leave that to you.
extraction_util.cpp
This looks like an entirely separate limit, and presumably X-Datadog-Tags is something that the library sets internally, hence the hardcoded, non-configurable limit. I'm guessing that exceeding 512 bytes would be unusual, and would indicate either a bug in the library or (more likely) the user configuring excessively long values.
Anyway, yup, logic is straightforward and looks good.
tracer.cpp
We're just passing baggage_opts_ to Baggage::extract. baggage_opts_ is preexisting. OK.
w3c_propagation.cpp
extract_tracestate (internal) gets a new parameter so that we can mutate span_tags if validation fails.
We have another separate, hardcoded 512-byte check, this time for datadog_value. Again, presumably internal-only and so a reasonable upper-bound-of-sanity to hardcode. Again, very simple logic to early-out if we exceed the max length.
test/
Tests look sane; test coverage on Baggage::extract looks good. It might be nice to ensure that there are enough test cases to cover both the value and trailing_spaces_value || properties branches for accumulation of the final value, but that's a nitpick.
Other tests look good.
(Sidenote: I've been doing the GENERATE(values<TestCase>) pattern but in a stupid way that uses DYNAMIC_SECTION; I knew there had to be a better way to do that. Thanks for the good example!)
As long as all of my stated assumptions line up with reality, LGTM.
|
Thanks Alex! Yes all of the assumptions and reasoning are accurate, thanks for providing a review. I'll re-run CI (seems to be flaky GitHub Actions runners / Docker build times) and then it should be good to merge |
BenchmarksBenchmark execution time: 2026-05-04 21:12:44 Comparing candidate commit d4c0de6 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.
|
|
🎯 Code Coverage (details) 🔗 Commit SHA: d4c0de6 | Docs | Datadog PR Page | Give us feedback! |
Description
Implements the baggage limits on context extraction. Previously the limits only applied on context injection. This PR also applies context extraction limits on the
ddtracestate information.Motivation
We want to implement a reasonable upper-bound on the size of incoming propagation headers.
Additional Notes
N/A