Skip to content

Simplify continue_trace to reuse propagation_context values #5158

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 1 commit into from
Nov 27, 2025
+16 −18
Merged

Simplify continue_trace to reuse propagation_context values #5158

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 16 additions & 18 deletions sentry_sdk/scope.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1186,28 +1186,26 @@ def continue_trace(
"""
self.generate_propagation_context(environ_or_headers)

# When we generate the propagation context, the sample_rand value is set
# if missing or invalid (we use the original value if it's valid).
# We want the transaction to use the same sample_rand value. Due to duplicated
# propagation logic in the transaction, we pass it in to avoid recomputing it
# in the transaction.
# TYPE SAFETY: self.generate_propagation_context() ensures that self._propagation_context
# is not None.
sample_rand = typing.cast(
PropagationContext, self._propagation_context
)._sample_rand()

transaction = Transaction.continue_from_headers(
normalize_incoming_data(environ_or_headers),
_sample_rand=sample_rand,
Copy link
Contributor

@sentrivana sentrivana Nov 27, 2025

Choose a reason for hiding this comment

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

Can we already remove the _sample_rand arg and associated logic from continue_from_headers altogether or is that still used anywhere?

Copy link
Member Author

@sl0thentr0py sl0thentr0py Nov 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

will remove, technically breaking but it's an underscore so fine
nvm, let's do all this in the major, we will remove continue_from_headers itself.

# generate_propagation_context ensures that the propagation_context is not None.
propagation_context = typing.cast(PropagationContext, self._propagation_context)

optional_kwargs = {}
if name:
optional_kwargs["name"] = name
if source:
optional_kwargs["source"] = source

return Transaction(
Copy link
Member Author

@sl0thentr0py sl0thentr0py Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

annoyingly, the type signature of continue_trace that we decided to expose is inconsistent with the actual Transaction constructor, mainly that name and source should not be Optional in continue_trace but have default values instead.

op=op,
origin=origin,
name=name,
source=source,
baggage=propagation_context.baggage,
parent_sampled=propagation_context.parent_sampled,
trace_id=propagation_context.trace_id,
parent_span_id=propagation_context.parent_span_id,
same_process_as_parent=False,
**optional_kwargs,
Copy link

@cursor cursor bot Nov 26, 2025

Choose a reason for hiding this comment

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

Bug: Missing baggage freeze loses third-party baggage items

When continuing a trace, the old code explicitly called baggage.freeze() in Transaction.continue_from_headers if there was a sentry-trace header. The new code passes propagation_context.baggage directly to the Transaction, but this baggage may remain mutable when the incoming baggage header contains only third-party items (no sentry items). Later, when Transaction.get_baggage() is called, it checks self._baggage.mutable and replaces the baggage with one from populate_from_transaction(), which does not preserve third_party_items. This causes third-party baggage items to be lost when propagating traces from upstream services that don't send sentry baggage items.

Fix in Cursor Fix in Web

Copy link
Member Author

@sl0thentr0py sl0thentr0py Nov 27, 2025

Choose a reason for hiding this comment

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

we actually only call serialize with include_third_party=True in the celery case but not generally, so I will postpone this freezing / third party business for later after clarifying the requirements. As a refactor, this PR mostly does what it did before so it's fine.

)
Comment on lines +1197 to 1207
Copy link

@sentry sentry bot Nov 26, 2025

Choose a reason for hiding this comment

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

Bug: The new continue_trace logic incorrectly populates baggage as a trace originator when an upstream sentry-trace exists without a baggage header.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

When an incoming sentry-trace header is present but no baggage header, the new PropagationContext.from_incoming_data() method leaves propagation_context.baggage as None. Consequently, Transaction.get_baggage() then populates a new mutable baggage from the transaction, causing the downstream service to incorrectly act as the trace originator. This deviates from the intended behavior of propagating the upstream trace context and can disrupt dynamic sampling and trace consistency across distributed systems.

💡 Suggested Fix

If sentry-trace is present but baggage is not, initialize propagation_context.baggage as an empty, frozen Baggage object to prevent Transaction.get_baggage() from populating it as a new trace originator.

🤖 Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: sentry_sdk/scope.py#L1187-L1207

Potential issue: When an incoming `sentry-trace` header is present but no `baggage`
header, the new `PropagationContext.from_incoming_data()` method leaves
`propagation_context.baggage` as `None`. Consequently, `Transaction.get_baggage()` then
populates a new mutable `baggage` from the transaction, causing the downstream service
to incorrectly act as the trace originator. This deviates from the intended behavior of
propagating the upstream trace context and can disrupt dynamic sampling and trace
consistency across distributed systems.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 3911558

Copy link
Member Author

@sl0thentr0py sl0thentr0py Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is because we froze the DSC earlier in the transaction but not in the propagation context, I will fix this separately tomorrow. Also in some SDKs, we have given up freezing completely so maybe I'll just leave it out.


return transaction

def capture_event(self, event, hint=None, scope=None, **scope_kwargs):
# type: (Event, Optional[Hint], Optional[Scope], Any) -> Optional[str]
"""
Expand Down
Loading