Simplify continue_trace to reuse propagation_context values #5158
Conversation
sl0thentr0py
force-pushed
the
neel/unify-continue-trace
branch
from
9d6e74d to
f8073f3
Compare
|
|
||
| optional_kwargs = {"source": source} if source else {} | ||
|
|
||
| return Transaction( |
There was a problem hiding this comment.
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.
We are already parsing both `sentry-trace` and `baggage` headers and filling in `sample_rand` while constructing the `propagation_context`. We don't need the redundant logic in `Transaction.continue_from_headers` anymore.
sl0thentr0py
force-pushed
the
neel/unify-continue-trace
branch
from
f8073f3 to
2b06f4f
Compare
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
|
|
||
| return Transaction( | ||
| 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, | ||
| ) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5158 +/- ##
==========================================
- Coverage 84.01% 83.97% -0.05%
==========================================
Files 180 180
Lines 18303 18307 +4
Branches 3249 3251 +2
==========================================
- Hits 15377 15373 -4
- Misses 1927 1932 +5
- Partials 999 1002 +3
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| trace_id=propagation_context.trace_id, | ||
| parent_span_id=propagation_context.parent_span_id, | ||
| same_process_as_parent=False, | ||
| **optional_kwargs, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| transaction = Transaction.continue_from_headers( | ||
| normalize_incoming_data(environ_or_headers), | ||
| _sample_rand=sample_rand, |
There was a problem hiding this comment.
Can we already remove the _sample_rand arg and associated logic from continue_from_headers altogether or is that still used anywhere?
There was a problem hiding this comment.
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.
Description
We are already parsing both
sentry-traceandbaggageheaders and filling insample_randwhile constructing thepropagation_context. We don't need the redundant logic inTransaction.continue_from_headersanymore.