Adding baggage limits for OT Baggage - #12303
Conversation
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b4215ce74
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
More details
The shared limit accounting covers all changed extraction paths and preserves reserved Haystack trace identifiers.
🤖 Datadog Autotest · Commit 1b4215c · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
🎯 Code Coverage (details) 🔗 Commit SHA: 3424e8d | Docs | View more details | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
There was a problem hiding this comment.
Pull request overview
This PR adds configurable limits for legacy OpenTracing-style baggage extraction (including ot-baggage-* headers and equivalent baggage segments in other propagators) by centralizing baggage acceptance, byte-budgeting, and “reserved” key protection in ContextInterpreter, then updating codec implementations and tests accordingly.
Changes:
- Enforce
trace.baggage.max.itemsandtrace.baggage.max.bytesduring extraction across Datadog, W3C, X-Ray, and Haystack propagators. - Prevent caller-controlled baggage (including mapped baggage) from overwriting reserved propagation bookkeeping keys (e.g., Haystack lossless IDs).
- Add/extend extractor tests to validate item/byte truncation, replacement semantics, and reserved-key behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java | Centralizes baggage limiting (items/bytes), replacement accounting, and reserved-key protection for extraction. |
| dd-trace-core/src/main/java/datadog/trace/core/propagation/DatadogHttpCodec.java | Routes OT baggage extraction through the new ContextInterpreter baggage limiter. |
| dd-trace-core/src/main/java/datadog/trace/core/propagation/W3CHttpCodec.java | Routes OT baggage extraction through the new ContextInterpreter baggage limiter. |
| dd-trace-core/src/main/java/datadog/trace/core/propagation/XRayHttpCodec.java | Routes X-Ray “key=value” segment baggage extraction through the new limiter and preserves parsing after truncation. |
| dd-trace-core/src/main/java/datadog/trace/core/propagation/HaystackHttpCodec.java | Treats Haystack trace/span IDs as reserved baggage (non-evictable by caller baggage) while keeping Parent-ID subject to limits. |
| dd-trace-core/src/test/java/datadog/trace/core/propagation/DatadogHttpExtractorTest.java | Adds tests for OT baggage item/byte limits, replacement semantics, and UTF-8 byte counting. |
| dd-trace-core/src/test/java/datadog/trace/core/propagation/W3CHttpExtractorTest.java | Adds tests for OT baggage item/byte truncation under W3C extraction. |
| dd-trace-core/src/test/java/datadog/trace/core/propagation/XRayHttpExtractorTest.java | Adds tests for X-Ray header baggage truncation and for continued parsing of trace context after truncation. |
| dd-trace-core/src/test/java/datadog/trace/core/propagation/HaystackHttpExtractorTest.java | Adds tests ensuring reserved Haystack IDs survive baggage limits and mapped baggage cannot overwrite them. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💭 thought: I think the PR is confusing in multiple parts.
First the introduction of "reversed" baggage.
Then the tree map for the size. If it's only to keep track of the size, why don't we only compute the size of the evicted element to apply the difference? That would save the tree at the cost of duplicated key (which should only happen on rare occasion and never from the dd-trace-* SDK as defined by spec).
Then the variables are now confusing: baggage should be baggageItems, and baggageItems should be baggageItemCount, etc... But getting rid of the baggageItemBytes would help.
| } | ||
|
|
||
| protected final boolean addBaggageItem(String key, String value) { | ||
| if (key == null || value == null || baggageMaxItems <= 0 || baggageMaxBytes <= 0) { |
There was a problem hiding this comment.
❔ question: Do we want to allow negative max item/bytes values as possible config value?
Or should we set the min as 0 during parsing? Having negative value as errors.
There was a problem hiding this comment.
Setting min to 0 is cleaner. This will affect W3C Baggage as well but positively. :)
| return false; | ||
| } | ||
| // Charge the raw key and value before URL decoding so percent-encoded input keeps its size. | ||
| final long itemBytes = Utf8.size(key) + Utf8.size(value); |
There was a problem hiding this comment.
❔ question: Can you use an approximate size here rather than depending on OkHttp lib?
We will need to remove it at some point (hopefully next quarter)
| final String decodedValue = HttpCodec.decode(value); | ||
| if (baggage.isEmpty()) { | ||
| baggage = new TreeMap<>(); | ||
| baggageItemBytes = new TreeMap<>(); |
There was a problem hiding this comment.
❔ question: Why do we need ordering here? (TreeMap)
There was a problem hiding this comment.
TreeMap was used in the old diff when OT Baggage was introduced, so I kept it here. Unsure of reasons it was used then (commit message says "use TreeMap everywhere for now"), but if unnecessary I can replace it w/ a HashMap.
| // that caller-supplied headers cannot evict it. Exempt from the configured limits, but not | ||
| // unbounded: the value is capped here rather than left to each caller to validate, so the total | ||
| // retained stays within trace.baggage.max.bytes plus a fixed amount per reserved key. | ||
| protected final void addReservedBaggageItem(String key, String value) { |
There was a problem hiding this comment.
💭 thought: I don't get the whole reserved baggage… If it's only for XRay and in case of conflict, I don't see the point having it.
| // Stores a value the tracer itself round-trips, outside the caller-controlled baggage budget so | ||
| // that caller-supplied headers cannot evict it. Exempt from the configured limits, but not | ||
| // unbounded: the value is capped here rather than left to each caller to validate, so the total | ||
| // retained stays within trace.baggage.max.bytes plus a fixed amount per reserved key. |
There was a problem hiding this comment.
💭 thought: Why is there method comment that is not part of the Javadoc?
There was a problem hiding this comment.
Good point, I'll update here. Also will open a follow-up PR to add to AGENTS.md to ensure that method comments are Javadoc style.
What Does This Do
This PR adds limits to the size and number of objects allowed to be extracted for the legacy OT Baggage. This PR also fixes an old bug where comma-concatenated Haystack traceID/spanID headers were parsed using the entire header value, causing context extraction to fail. We now parse and preserve only the first traceID/spanID from the headers.
Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]