ref: Add sampling to span first (11) #5617
2 issues
code-review: Found 2 issues (2 medium)
Medium
Baggage mutability check missing in _update_sample_rate - `sentry_sdk/scope.py:1255`
The _update_sample_rate method modifies baggage.sentry_items["sample_rate"] without checking if the baggage is mutable. According to the Baggage class documentation (lines 608-611 in tracing_utils.py), callers must verify baggage.mutable is True before mutating the object. When a baggage is parsed from an incoming header that already contains sentry items, mutable is set to False. This means in continued traces, the sample rate update would modify an immutable baggage, violating the documented invariant.
NoOpStreamedSpan lacks test coverage for new sampling/client report behavior - `sentry_sdk/traces.py:488-510`
The NoOpStreamedSpan class has been extended with sampling-related functionality (unsampled_reason parameter, client report recording in _end()), but there are no tests covering this new behavior. The existing tests/tracing/test_noop_span.py tests NoOpSpan but not NoOpStreamedSpan. Per the review checklist, every PR should have appropriate test coverage for functional changes.
Duration: 1m 53s · Tokens: 872.1k in / 11.0k out · Cost: $1.09 (+extraction: $0.00, +merge: $0.00, +fix_gate: $0.00)
Annotations
Check warning on line 1255 in sentry_sdk/scope.py
sentry-warden / warden: code-review
Baggage mutability check missing in _update_sample_rate
The `_update_sample_rate` method modifies `baggage.sentry_items["sample_rate"]` without checking if the baggage is mutable. According to the `Baggage` class documentation (lines 608-611 in tracing_utils.py), callers must verify `baggage.mutable` is `True` before mutating the object. When a baggage is parsed from an incoming header that already contains sentry items, `mutable` is set to `False`. This means in continued traces, the sample rate update would modify an immutable baggage, violating the documented invariant.
Check warning on line 510 in sentry_sdk/traces.py
sentry-warden / warden: code-review
NoOpStreamedSpan lacks test coverage for new sampling/client report behavior
The `NoOpStreamedSpan` class has been extended with sampling-related functionality (`unsampled_reason` parameter, client report recording in `_end()`), but there are no tests covering this new behavior. The existing `tests/tracing/test_noop_span.py` tests `NoOpSpan` but not `NoOpStreamedSpan`. Per the review checklist, every PR should have appropriate test coverage for functional changes.