Skip to content

fix(tracing): drop trailing comma when serializing baggage without third-party items - #8119

Merged
pabloDeputter merged 5 commits into
getsentry:masterfrom
alphasafal:fix/baggage-trailing-comma
Oct 28, 2026
Merged

pabloDeputter merged 5 commits into
getsentry:masterfrom
alphasafal:fix/baggage-trailing-comma

Conversation

@alphasafal

@alphasafal alphasafal commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #8120

Summary

Baggage.serialize(include_third_party=True) appends third_party_items unconditionally. When there are no third-party items (third_party_items == ""), it appends an empty segment, so ",".join(...) leaves a trailing comma in the serialized baggage header:

sentry-trace_id=...,sentry-public_key=...,sentry-sample_rate=0.5,

Per the W3C Baggage spec a trailing comma is an empty list-member (malformed). Sentry's own parser tolerates it, but a strict W3C / OpenTelemetry baggage parser on a downstream consumer can reject or mishandle the header.

Where it's reachable

The Celery integration calls combined.serialize(include_third_party=True) (sentry_sdk/integrations/celery/__init__.py) when merging incoming and Sentry baggage. Whenever the merged baggage has only Sentry items and no third-party ones — the common Sentry → Celery → Sentry propagation case — the outgoing baggage header gets the stray comma.

Fix

Only append third_party_items when it is non-empty:

if include_third_party and self.third_party_items:
    items.append(self.third_party_items)

The output is unchanged when third-party items are present.

Test

Adds test_sentry_only_baggage in tests/tracing/test_baggage.py: it verifies that baggage containing only Sentry items serializes to the expected header, with identical output whether third-party items are included or not.

Validation on Python 3.13.7:

  • TESTPATH=tests/tracing/test_baggage.py uv run tox -e py3.13-common -- --no-cov: 4 passed.
  • Restoring the original if include_third_party: condition makes test_sentry_only_baggage fail on the trailing comma (1 failed, 3 passed); restoring the fix gives 4 passed.
  • Ruff lint passed for tests and sentry_sdk; formatting checks passed for the changed test files.
  • uv run --group typing mypy sentry_sdk: no issues in 203 source files.

…ird-party items

Baggage.serialize(include_third_party=True) appended third_party_items
unconditionally, so when there were no third-party items it added an empty
segment and the serialized header ended with a comma (e.g.
"sentry-trace_id=...,sentry-public_key=...,"). This is reachable from the
Celery integration, which calls combined.serialize(include_third_party=True)
whenever the merged baggage has only Sentry items.

Only append third_party_items when it is non-empty; the output is unchanged
when third-party items are present. Adds a regression test.
@alphasafal
alphasafal requested a review from a team as a code owner October 25, 2026 18:25
Copilot AI balanced review requested due to automatic review settings October 25, 2026 18:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@alphasafal

Copy link
Copy Markdown
Contributor Author

Ran the baggage tests locally against this branch — tests/tracing/test_baggage.py: 4 passed, including the new test_serialize_no_trailing_comma_without_third_party_items, with no change to the existing cases.

@chrikrah chrikrah left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@alphasafal CONTRIBUTING auto-closes outside PRs without a linked, maintainer-acknowledged issue. Otherwise, from my perspective this can be merged at e8f8ace.

# probe_celery.py: _update_celery_task_headers({"baggage": "sentry-trace_id=771a43a4192642f0b136d5159a501700,sentry-sample_rate=0.5"}, span, False)
$ python probe_celery.py   # e8f8ace with the base condition (if include_third_party:)
'sentry-trace_id=771a43a4192642f0b136d5159a501700,sentry-sample_rand=0.395125,sentry-environment=production,sentry-transaction=t,sentry-sample_rate=0.5,sentry-sampled=true,'
$ python probe_celery.py   # e8f8ace
'sentry-trace_id=771a43a4192642f0b136d5159a501700,sentry-sample_rand=0.051087,sentry-environment=production,sentry-transaction=t,sentry-sample_rate=0.5,sentry-sampled=true'

$ pytest tests/tracing/test_baggage.py tests/integrations/celery/test_update_celery_task_headers.py -q --no-cov
42 passed, 4 warnings in 4.42s                       # e8f8ace, Python 3.12
1 failed, 41 passed, 4 warnings in 1.11s             # base condition restored, test kept
FAILED tests/tracing/test_baggage.py::test_serialize_no_trailing_comma_without_third_party_items

# test_span_with_transaction_custom_headers, the one Celery merge test, sends custom=value
# and expects combined_baggage.serialize(include_third_party=True): it passes on both

non-blocking: add a Celery test without third-party baggage?

@alphasafal

Copy link
Copy Markdown
Contributor Author

@chrikrah thanks for running the Celery probe and checking it against the base condition. I've opened #8120 for the bug and linked it in the description, and I'll add a Celery test with no third-party baggage so that path is covered directly.

@alphasafal
alphasafal marked this pull request as draft October 26, 2026 02:12
Adds a Celery header test where the incoming baggage has only Sentry items.
The outgoing baggage must not end with a trailing comma.
@alphasafal

Copy link
Copy Markdown
Contributor Author

Added the Celery test for baggage without third-party items in 0bc5679. It passes with the fix and fails on the base condition. Marking this ready for review. @chrikrah could a maintainer also acknowledge #8120 so this isn't auto-closed?

@alphasafal
alphasafal marked this pull request as ready for review October 27, 2026 04:52

@chrikrah chrikrah left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@alphasafal the Celery test fails on the base condition; nothing blocks a merge.

$ pytest tests/tracing/test_baggage.py tests/integrations/celery/test_update_celery_task_headers.py -q --no-cov -p no:cacheprovider   # export of 0bc5679, sentry_sdk imported from it, Python 3.12.3, celery 5.6.3
43 passed, 4 warnings
2 failed, 41 passed, 4 warnings    # base condition `if include_third_party:` restored, tests kept
FAILED tests/tracing/test_baggage.py::test_serialize_no_trailing_comma_without_third_party_items
FAILED tests/integrations/celery/test_update_celery_task_headers.py::test_span_with_transaction_custom_headers_without_third_party_baggage

My 10-25 auto-close warning was wrong: validate-pr.yml only runs on open or reopen, so #8120 needs nothing.

Comment thread tests/integrations/celery/test_update_celery_task_headers.py Outdated
Comment thread tests/tracing/test_baggage.py Outdated
Comment thread tests/tracing/test_baggage.py Outdated
Comment thread tests/tracing/test_baggage.py Outdated
Comment thread tests/tracing/test_baggage.py Outdated

@pabloDeputter pabloDeputter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for picking this up! Looks good to me now 😃

@pabloDeputter
pabloDeputter merged commit 63707d7 into getsentry:master Oct 28, 2026
165 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Baggage.serialize(include_third_party=True) leaves a trailing comma when there are no third-party items

4 participants