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 2 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
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Codecov Results 📊✅ 92122 passed | ⏭️ 6304 skipped | Total: 98426 | Pass Rate: 93.6% | Execution Time: 314m 49s 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 100.00%. Project has 2477 uncovered lines. Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 89.68% 89.69% +0.01%
==========================================
Files 193 193 —
Lines 24007 24021 +14
Branches 8342 8344 +2
==========================================
+ Hits 21530 21544 +14
- Misses 2477 2477 —
- Partials 1386 1386 —Generated by Codecov Action |
sentrivana
left a comment
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.
3 participants
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