feat(span-first): Support before_send_span#6239
Conversation
sentrivana
changed the title
before_send_span
Codecov Results 📊✅ 282 passed | Total: 282 | Pass Rate: 100% | Execution Time: 41.95s 📊 Comparison with Base Branch
✨ No test changes detected All tests are passing successfully. ❌ Patch coverage is 15.22%. Project has 14797 uncovered lines. Files with missing lines (6)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 33.63% 33.60% -0.03%
==========================================
Files 190 190 —
Lines 22260 22284 +24
Branches 7536 7548 +12
==========================================
+ Hits 7485 7487 +2
- Misses 14775 14797 +22
- Partials 747 747 —Generated by Codecov Action |
Codecov Results 📊✅ 44 passed | ❌ 13 failed | Total: 57 | Pass Rate: 77.19% | Execution Time: 9.62s ❌ Failed Tests
|
| File | Patch % | Lines |
|---|---|---|
utils.py |
54.32% | |
client.py |
61.82% | |
consts.py |
99.22% |
Generated by Codecov Action
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
| if serialized is None: | ||
| return | ||
|
|
||
| elif ty == "span" and isinstance(telemetry, StreamedSpan): |
There was a problem hiding this comment.
The isinstance(telemetry, StreamedSpan) part of the condition is only there because mypy was complaining :( It can't deal with dispatchers/generics very well in general.
| class fake_record_sql_queries: # noqa: N801 | ||
| def __init__(self, *args, **kwargs): | ||
| with record_sql_queries_supporting_streaming( | ||
| self._ctx_mgr = record_sql_queries_supporting_streaming( |
There was a problem hiding this comment.
Had to change this test because it was failing -- it was closing the span too early.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e65b1c2. Configure here.
alexander-alderman-webb
left a comment
alexander-alderman-webb
left a comment
There was a problem hiding this comment.
Looks good!
ericapisani
left a comment
ericapisani
left a comment
There was a problem hiding this comment.
LGTM to me overall, just saw a few polishing opportunities. Approving so as not to block 
| # estimate the attributes separately. | ||
| estimate = 210 | ||
| for value in item._attributes.values(): | ||
| for value in (item.get("attributes") or {}).values(): |
There was a problem hiding this comment.
This can be made slightly more concise by doing the following:
| for value in (item.get("attributes") or {}).values(): | |
| for value in (item.get("attributes", {})).values(): |
There was a problem hiding this comment.
If item["attributes"] exists and is None, then item.get("attributes", {}) would return None and the subsequent .values() call would fail.
FWIW I don't think attributes can be None at this point, but wrote it that way just to be extra safe.
| if isinstance(serialized, dict) and serialized and "name" in serialized: | ||
| telemetry.name = serialized["name"] # type: ignore[typeddict-item] | ||
| telemetry._attributes = {} | ||
| for k, v in (serialized.get("attributes") or {}).items(): |
There was a problem hiding this comment.
Similar to my comment above, you can remove the or by adding the {} as the fallback on the get:
| for k, v in (serialized.get("attributes") or {}).items(): | |
| for k, v in (serialized.get("attributes", {})).items(): |
There was a problem hiding this comment.
Same concern as above -- there's a case where the two are not equivalent and I'd opt for the more paranoid version
| def test_before_send_span_basic(sentry_init, capture_items): | ||
| def before_send_span(span, hint): | ||
| assert isinstance(span, dict) | ||
|
|
||
| span["name"] = "Better span name" | ||
| del span["attributes"]["drop"] | ||
| span["attributes"]["sanitize"] = "[Removed]" | ||
| span["attributes"]["add"] = "new" |
There was a problem hiding this comment.
before_send_span exceptions propagate to user code via __exit__, unlike before_send for events
If a user's before_send_span callback raises, the exception propagates through StreamedSpan.__exit__, crashing the surrounding with block. The main before_send for events wraps the callback in capture_internal_exceptions(); _capture_telemetry in client.py does not — add a with capture_internal_exceptions(): guard there and add a test case here.
Verification
Checked sentry_sdk/client.py line 1199: serialized = before_send(serialized, {}) is called bare inside _capture_telemetry with no try/except or capture_internal_exceptions wrapper. The main event before_send at client.py:872 is protected: with capture_internal_exceptions(): new_event = before_send(event, hint or {}). The call path for spans is: StreamedSpan.__exit__ → _end() → scope._capture_span() → client._capture_span() → _capture_telemetry() → before_send(serialized, {}). An exception escapes all the way back to user code in the with block. This is also true for before_send_log/before_send_metric (consistent pattern), but spans are called from context-manager __exit__, making the crash surface more user-visible. The four new tests never exercise a raising callback, so the gap is undetected.
Identified by Warden find-bugs · S67-F2P
3 participants
Description
Add support for
before_send_spanin span streaming mode.before_send_spanis different frombefore_send_metricandbefore_send_login that:None)To that end, we're now serializing the span earlier, and exposing the serialized dictionary in the
before_sendcallback. This is consistent with metrics and logs. It also means we're now queuing dictionaries instead ofStreamedSpaninstances in the span batcher, which should also decrease our memory footprint.This aligns our implementation with JS.
See https://develop.sentry.dev/sdk/telemetry/spans/scrubbing-data/ for spec.
Issues
before_send_span#5388