Conversation
Adds span streaming support to the httpx integration by adding a new
code path that uses `start_span` from `sentry_sdk.traces` when
`_experiments={"trace_lifecycle": "stream"}` is enabled. The existing
legacy path is preserved unchanged.
Test coverage is updated to:
- Rename span-data-asserting tests with a `_legacy` suffix
- Fix broken mock targets in duration threshold tests (`start_span` →
`legacy_start_span` after the rename introduced two names)
- Add `_span_streaming` variants for all tests that assert on span data,
using `capture_items("span")` and `span["attributes"]` instead of
`event["spans"][*]["data"]`
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛
Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
Codecov Results 📊✅ 13 passed | Total: 13 | Pass Rate: 100% | Execution Time: 8.00s All tests are passing successfully. ❌ Patch coverage is 6.03%. Project has 15098 uncovered lines. Files with missing lines (3)
Generated by Codecov Action |
Codecov Results 📊Generated by Codecov Action |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
There was a problem hiding this comment.
TY! Left some comments, the gist:
- Any attributes we set on the span in streaming mode need to be in Sentry Conventions. I've tried to point out some that need changing but please go through the rest too and double check 🙏🏻
- I'd remove all of the
start_timestamp_monotonic_mschanges. I don't really see the benefit over just using thestart_timestampand removing it makes the new and oldadd_http_request_sourcesimilar enough that we can unify them.
| @property | ||
| def start_timestamp_monotonic_ns(self) -> "Optional[int]": | ||
| return self._start_timestamp_monotonic_ns |
There was a problem hiding this comment.
Let's not expose new properties on the span -- the idea is to keep the API surface minimal. We can still use the attribute directly in our code if we need to, but I think we don't (see other comments).
| # profiling depends on this value and requires that | ||
| # it is measured in nanoseconds | ||
| self._start_timestamp_monotonic_ns = nanosecond_time() |
There was a problem hiding this comment.
For context, this code including the try..except was ported as-is from the existing tracing implementation. I also don't know why we're guarding against AttributeErrors here (and later in _end), but it seems deliberate (this was before LLMs 😆). I wouldn't change it, or at least not in this PR unless we're 100% sure the exception handling is not necessary.
There was a problem hiding this comment.
I also don't know why we're guarding against AttributeErrors here (and later in _end), but it seems deliberate (this was before LLMs 😆).
From my commit splunking, it looks like this try-except was introduced a while ago in the existing tracing implementation when Python 2 was still supported, and the perf_counter didn't exist on the time module just yet as it was first made available in Python 3.3..
Since we've dropped support for Python 2 within the SDK, this is safe to remove 🔥
| if span.start_timestamp_monotonic_ns is not None: | ||
| elapsed = nanosecond_time() - span.start_timestamp_monotonic_ns | ||
| end_timestamp = span.start_timestamp + timedelta(microseconds=elapsed / 1000) | ||
| else: | ||
| end_timestamp = datetime.now(timezone.utc) |
There was a problem hiding this comment.
Why work with the monotonic start timestamp here at all? The original add_http_request_source just uses start_timestamp, so I believe that should be good enough?
| if span.start_timestamp_monotonic_ns is not None: | |
| elapsed = nanosecond_time() - span.start_timestamp_monotonic_ns | |
| end_timestamp = span.start_timestamp + timedelta(microseconds=elapsed / 1000) | |
| else: | |
| end_timestamp = datetime.now(timezone.utc) | |
| end_timestamp = datetime.now(timezone.utc) |
There was a problem hiding this comment.
If we make the change I proposed above, then as far as I can see the only difference to the original add_http_request_source is just the source of end_timestamp and then I believe we don't need a separate function for the streaming case at all. We just need something like this in the original function:
if span.timestamp is not None:
end_timestamp = span.timestamp
else:
end_timestamp = datetime.now(timezone.utc)There was a problem hiding this comment.
Why work with the monotonic start timestamp here at all? The original add_http_request_source just uses start_timestamp, so I believe that should be good enough?
I opted to use it in this way because the span.timestamp is set within _end using the start_timestamp_monotonic_ns (snippet below from within StreamedSpan but here's the link on main as well):
if self._timestamp is None:
if self._start_timestamp_monotonic_ns is not None:
elapsed = nanosecond_time() - self._start_timestamp_monotonic_ns
self._timestamp = self._start_timestamp + timedelta(
microseconds=elapsed / 1000
)
else:
self._timestamp = datetime.now(timezone.utc)and I was trying to emulate the behaviour as closely as possible (which is also why I exposed it as a property on the class).
If we want to specifically avoid exposing start_timestamp_monotonic_ns as a property, we could also explore adding a method that does this calculation for us to retain the behaviour.
I'm also realizing that, since the changes that I introduced unconditionally set the self._start_timestamp_monotonic_ns within the __init__, this can never be None and so the else falling back to a datetime stamp is dead code and needs to be removed.
-- edit: this is wrong, I forgot about the NoOpStreamedSpan class which would leave this uninitialized
Also - I think you're right that this can be consolidated into the single function (add_http_request_source), so will look to make those changes regardless.
There was a problem hiding this comment.
I've consolidated into a single function in 25a953e , but have left the start_timestamp_monotonic_ns changes in there for now
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
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 9d2546f. Configure here.
2 participants
Migrates the httpx integration to the span-first (streaming span) architecture.
When
_experiments={"trace_lifecycle": "stream"}is enabled, the integrationnow uses
StreamedSpanviasentry_sdk.traces.start_spaninstead of the legacySpan-based (transactions) path. The legacy path remains unchanged for backwards compatibility.Supporting changes:
add_http_request_source_for_streamed_span()added totracing_utils— aStreamedSpan-compatible variant that must be called inside the span contextmanager so source data is attached before the span is flushed
add_source()updated to dispatch betweenset_data/set_attributedependingon span type
start_timestamp_monotonic_nsproperty added toStreamedSpanandNoOpStreamedSpanto support the request source duration threshold checkPart of the broader span-first integration migration.
Fixes PY-2330
Fixes #6028