8000 Enabling baggage cache to support limits and non-ascii characters by mhlidd · Pull Request #8713 · DataDog/dd-trace-java · GitHub
[go: up one dir, main page]
Skip to content

Enabling baggage cache to support limits and non-ascii characters #8713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
May 7, 2025
Merged

Enabling baggage cache to support limits and non-ascii characters #8713

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0ac63be
enabling baggage cache to support limits
mhlidd Apr 21, 2025
eb3ae15
updating limits to consider non-ascii characters
mhlidd Apr 22, 2025
03d4b1c
adding edge case
mhlidd Apr 25, 2025
e111266
PR comments
mhlidd Apr 29, 2025
576aab8
feat(core): Simplify baggage config usage
PerfectSlayer May 2, 2025
f9c53d2
feat(core): Add non ASCII character baggage extraction
PerfectSlayer May 2, 2025
a1e5ffb
Merge branch 'master' into mhlidd/update_baggage_limits
mhlidd May 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
adding edge case
  • Loading branch information
@mhlidd
mhlidd committed Apr 25, 2025
commit 03d4b1c252c7e2685f307b109bad498004cb254e
8 changes: 6 additions & 2 deletions dd-trace-core/src/main/java/datadog/trace/core/baggage/BaggagePropagator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public BaggagePropagator(boolean injectBaggage, boolean extractBaggage) {
public <C> void inject(Context context, C carrier, CarrierSetter<C> setter) {
int maxItems = this.config.getTraceBaggageMaxItems();
int maxBytes = this.config.getTraceBaggageMaxBytes();
//noinspection ConstantValue
// noinspection ConstantValue
if (!this.injectBaggage
|| maxItems == 0
|| maxBytes == 0
Expand Down Expand Up @@ -166,7 +166,11 @@ private Map<String, String> parseBaggageHeaders(String input) {
truncatedCache = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s not strictly truncated, it’s invalid in this case as non ascii characters are not allowed in HTTP headers. I though the RFC asked to drop the header and not parse it if it was invalid.

From RFC encoding:

In other words, the HTTP baggage header is encoded using only ASCII characters.

From RFC error handling:

When this occurs, the APM SDK should not try to extract any values from the entire header. In other words, APM SDKs should ignore the header and not try to extract "good" values while ignoring "bad" ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that this isn't an invalid case and should be something that we can support since HTTP clients technically can send non-ascii characters as headers and we do support non-ascii characters as baggage. I would say that the invalid case is more for parsing issues with not being able to figure out what baggage should be used.

this.w3cHeader = null;
} else if (!truncatedCache && (end > this.maxBytes || baggage.size() > this.maxItems)) {
this.w3cHeader = input.substring(0, start - 1); // -1 to ignore the k/v separator
if(start == 0) { // if we go out of range after first k/v pair, there is no cache
this.w3cHeader = null;
} else{
this.w3cHeader = input.substring(0, start - 1); // -1 to ignore the k/v separator
}
truncatedCache = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should break here if you reach max size or max bytes.
It will simply the if clause too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we don't cache here, we still need to maintain all of the raw baggage data so we can't break.

}

Expand Down
0