fix(tracing): handle exceptions raised within traces_sampler and other callbacks#6853
fix(tracing): handle exceptions raised within traces_sampler and other callbacks#6853ericapisani wants to merge 3 commits into
Conversation
When a user-provided traces_sampler callback raises, catch the exception, log a warning, and fall back to the parent sample rate or the configured traces_sample_rate instead of propagating the error. Refs PY-2617 Refs #6849
Wrap the before_send callback invocation in capture_internal_exceptions() so that exceptions raised by user-provided before_send_log, before_send_metric, and before_send_span callbacks do not crash the application. When a callback raises, the original, unmodified telemetry item is sent. Refs PY-2617 Refs #6849
Codecov Results 📊✅ 92197 passed | ⏭️ 6304 skipped | Total: 98501 | Pass Rate: 93.6% | Execution Time: 323m 36s All tests are passing successfully. ✅ Patch coverage is 100.00%. Project has 2475 uncovered lines. Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 89.68% 89.70% +0.02%
==========================================
Files 193 193 —
Lines 24007 24028 +21
Branches 8342 8346 +4
==========================================
+ Hits 21529 21553 +24
- Misses 2478 2475 -3
- Partials 1387 1384 -3Generated by Codecov Action |
sentrivana
left a comment
There was a problem hiding this comment.
Looks good, just a couple places where the behavior or tests need some tweaking.
Re: dropping/keeping the metrics/logs/spans that went through a faulty before_send_*: might need more alignment on this. Spans likely shouldn't be dropped, but metrics and logs we might want to drop. There's a convo on slack about this.
| sample_rate = propagation_context.parent_sampled | ||
| else: | ||
| sample_rate = client.options["traces_sample_rate"] | ||
| sample_rate = _get_fallback_sample_rate( |
There was a problem hiding this comment.
I'd change the name of the helper function since in the context of this else branch, it's not really a fallback.
Maybe something like _get_effective_sample_rate?
| # parent said "don't sample", but its propagated sample rate of 0.75 | ||
| # combined with sample_rand 0.5 should win over both the flag and | ||
| # traces_sample_rate=0.0 |
There was a problem hiding this comment.
This is actually not correct. If the parent sample decision is 0, that should take precedence over anything else.
Can we remove this test? I think I know where this needs fixing (it's not something you've introduced in this PR), and I can follow up with a fix after we've merged this. (I expect some other tests to break so don't want to tack this onto your PR.)
| _experiments={ | ||
| "trace_lifecycle": "stream", | ||
| }, |
There was a problem hiding this comment.
| _experiments={ | |
| "trace_lifecycle": "stream", | |
| }, | |
| trace_lifecycle="stream", |
| _experiments={ | ||
| "trace_lifecycle": "stream", | ||
| }, |
There was a problem hiding this comment.
| _experiments={ | |
| "trace_lifecycle": "stream", | |
| }, | |
| trace_lifecycle="stream", |
| # The exception in before_send_log is swallowed and the original, | ||
| # unmodified log is sent. | ||
| assert len(logs) == 1 | ||
| assert logs[0]["body"] == "This is an error log..." | ||
| assert logs[0]["attributes"]["sentry.severity_text"] == "error" |
There was a problem hiding this comment.
We should drop the log in this case
| # The exception in before_send_metric is swallowed and the original, | ||
| # unmodified metric is sent. | ||
| metrics = [item.payload for item in items] | ||
| assert len(metrics) == 1 | ||
| assert metrics[0]["name"] == "test.keep" |
There was a problem hiding this comment.
Same here -- the metric should not be sent
| "traces_sample_rate,expected_decision", | ||
| [(0.0, False), (0.25, False), (0.75, True), (1.00, True)], | ||
| ) | ||
| def test_traces_sampler_raising_falls_back_to_traces_sample_rate_span_streaming( |
There was a problem hiding this comment.
Can we also add a span streaming test case with no incoming trace propagation headers/no continue_trace and check that we fall back to traces_sample_rate?
| assert span.sampled is traces_sampler_return_value | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
[not related to this specific line or testcase] One case I'm not quite sure of: there is no incoming trace, traces_sampler is defined and throws, and there is no traces_sample_rate defined. We should have a test case (or two: streaming and non streaming) for that too.
There was a problem hiding this comment.
Will add a couple, where the assertion is that the span/transaction isn't sampled
sentrivana
left a comment
There was a problem hiding this comment.
Approving to not block -- as long as the comments have been addressed feel free to merge 🙏🏻
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0f8a64a. Configure here.
| raise | ||
|
|
||
| if exception_raised_in_before_send_func: | ||
| return |
There was a problem hiding this comment.
Span dropped on callback exception
Medium Severity
The early return on a before_send exception applies to spans as well, so a raising before_send_span drops the span. That conflicts with the nearby rule that spans cannot be dropped, and with invalid-return handling that keeps the original span. Review feedback only asked to drop logs and metrics; the PR description still says the unmodified item is sent. A buggy sanitizer can silently break traces.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0f8a64a. Configure here.
| if exception_raised_in_before_send_func: | ||
| return |
There was a problem hiding this comment.
Bug: If the telemetry before_send callback raises an exception, the telemetry item is silently dropped instead of sending the original item.
Severity: MEDIUM
Suggested Fix
Remove the early return statement if exception_raised_in_before_send_func: return. This will allow execution to continue after the before_send callback fails, sending the original serialized data that was preserved.
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/client.py#L1255-L1256
Potential issue: In `_capture_telemetry`, if the user-provided `before_send` callback
raises an exception, the code sets a flag `exception_raised_in_before_send_func` and the
exception is suppressed. An `if` statement then checks this flag and executes an early
`return`, which silently drops the telemetry item. This contradicts the intended
behavior of sending the original, unmodified telemetry item when a callback fails.
| ) -> "Union[float, bool]": | ||
| if propagation_context.parent_sampled is not None: | ||
| propagation_context_sample_rate = propagation_context._sample_rate() | ||
|
|
||
| if propagation_context_sample_rate is not None: | ||
| return propagation_context_sample_rate | ||
| else: | ||
| return propagation_context.parent_sampled | ||
| else: | ||
| return client.options["traces_sample_rate"] | ||
|
|
There was a problem hiding this comment.
Bug: When traces_sampler raises an exception, the fallback can return None as the sample rate, causing all associated traces to be silently dropped.
Severity: HIGH
Suggested Fix
Modify _get_effective_sample_rate to ensure it does not return None when the traces_sampler fails. The fallback logic should return a valid float, such as 0.0, if traces_sample_rate is not set, to prevent traces from being dropped.
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/tracing_utils.py#L1579-L1591
Potential issue: The function `_get_effective_sample_rate` can return `None` if
`traces_sampler` raises an exception and `traces_sample_rate` is not explicitly
configured by the user. The downstream function `is_valid_sample_rate` treats `None` as
an invalid rate, logs a misleading warning, and causes all traces for that context to be
silently discarded. This affects users who only configure `traces_sampler`.


When a user-provided traces_sampler callback raises, catch the exception,
log a warning, and fall back to the parent sample rate or the configured
traces_sample_rate instead of propagating the error.
Wrap the before_send callback invocation in capture_internal_exceptions()
so that exceptions raised by user-provided before_send_log,
before_send_metric, and before_send_span callbacks do not crash the
application. When a callback raises, the original, unmodified telemetry
item is sent.
Fixes PY-2617
Fixes #6850