Skip to content

fix(baggage): drop entire baggage header when malformed#3498

Merged
darccio merged 5 commits into
mainfrom
rachel.yang/baggage-malformed-header
May 12, 2025
Merged

fix(baggage): drop entire baggage header when malformed#3498
darccio merged 5 commits into
mainfrom
rachel.yang/baggage-malformed-header

Conversation

@rachelyangdog

@rachelyangdog rachelyangdog commented May 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

According to the RFC,

"When APM SDKs are extracting baggage from propagation headers, they may encounter malformed header contents. 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."

So instead of dropping individual baggage items, we drop the entire header and log a warning.

What does this PR do?

Motivation

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • New code is free of linting errors. You can check this by running golangci-lint run locally.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

@rachelyangdog
rachelyangdog requested a review from a team as a code owner May 6, 2025 20:15
@rachelyangdog
rachelyangdog requested a review from mtoffl01 May 6, 2025 20:16
@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented May 6, 2025 •
edited
Loading

Copy link
Copy Markdown

Datadog Report

Branch report: rachel.yang/baggage-malformed-header
Commit report: 17cfecb
Test service: dd-trace-go

✅ 0 Failed, 4526 Passed, 65 Skipped, 3m 13.39s Total Time

@rachelyangdog rachelyangdog changed the title drop entire baggage header when malformed fix(baggage): drop entire baggage header when malformed May 6, 2025
@pr-commenter

pr-commenter Bot commented May 6, 2025 •
edited
Loading

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2025-05-09 14:47:43

Comparing candidate commit 0db2d6b in PR branch rachel.yang/baggage-malformed-header with baseline commit 448466c in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 56 metrics, 0 unstable metrics.

darccio
darccio reviewed May 7, 2025
View reviewed changes
Comment thread ddtrace/tracer/textmap.go Outdated
mtoffl01
mtoffl01 reviewed May 8, 2025
View reviewed changes
Comment thread ddtrace/tracer/textmap.go Outdated
mtoffl01
mtoffl01 reviewed May 8, 2025
View reviewed changes
Comment thread ddtrace/tracer/textmap_test.go Outdated
mtoffl01
mtoffl01 reviewed May 8, 2025
View reviewed changes

@mtoffl01 mtoffl01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just some nits in terms of implementation, but would love to see an approach to testing that tries to break the behavior, to show that our implementation is robust

mtoffl01
mtoffl01 reviewed May 8, 2025
View reviewed changes

@mtoffl01 mtoffl01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just some nits in terms of implementation, but would love to see an approach to testing that tries to break the behavior, to show that our implementation is robust

@mtoffl01 mtoffl01 mentioned this pull request May 8, 2025
Merged
7 tasks
@rachelyangdog
rachelyangdog force-pushed the rachel.yang/baggage-malformed-header branch from 49f65f0 to 48b6cb4 Compare May 8, 2025 17:25
mtoffl01
mtoffl01 reviewed May 8, 2025
View reviewed changes
Comment thread ddtrace/tracer/textmap.go Outdated
@rachelyangdog

rachelyangdog commented May 9, 2025

Copy link
Copy Markdown
Contributor Author

/merge

@dd-devflow

dd-devflow Bot commented May 9, 2025 •
edited
Loading

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2025-05-09 14:13:51 UTC ℹ️ Start processing command /merge

2025-05-09 14:13:55 UTC ℹ️ MergeQueue: queue is disabled

Added to the queue but the mergequeue is not enabled for now.

2025-05-09 14:18:11 UTC ❌ MergeQueue: This merge request was updated

This PR is rejected because it was updated

@darccio
darccio enabled auto-merge (squash) May 9, 2025 14:18
@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented May 12, 2025

Copy link
Copy Markdown

Datadog Summary

✅ Code Quality    ✅ Code Security    ✅ Dependencies

Was this helpful? Give us feedback!

@darccio
darccio merged commit 094af77 into main May 12, 2025
@darccio
darccio deleted the rachel.yang/baggage-malformed-header branch May 12, 2025 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@darccio darccio darccio approved these changes
@kakkoyun kakkoyun kakkoyun approved these changes
@mtoffl01 mtoffl01 mtoffl01 approved these changes

Assignees

No one assigned

Labels

mergequeue-status: rejected

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rachelyangdog @darccio @kakkoyun @mtoffl01