Skip to content

ref: Allow to start and finish StreamedSpans (9)#5598

Open
sentrivana wants to merge 55 commits intomasterfrom
ivana/span-first-9-start-end
Open

ref: Allow to start and finish StreamedSpans (9)#5598
sentrivana wants to merge 55 commits intomasterfrom
ivana/span-first-9-start-end

Conversation

@sentrivana
Copy link
Contributor

@sentrivana sentrivana commented Mar 6, 2026

Description

This PR adds logic for starting and finishing spans, both via a context manager with start_span(...), as well as directly with span = start_span(...) and span.end().

Summary

  • Added internal _start() and _end() functions that handle the starting and ending logic.
    • _start() handles keeping track of what was called context_manager_state on old Spans, i.e., remembering which span was set on the scope before this span so that it can be restored once this span is over.
    • _end() handles restoring the old span, setting the end timestamp, adding final attributes on it, and queueing it in the span batcher.
  • Added __enter__ and __exit__ methods on both StreamedSpan and NoOpStreamedSpan. These call the internal methods above, with __exit__ additionally setting the span status to error if it encountered one.
  • Added a new span.end() public function that allows to set an optional end_timestamp. If provided, it sets it and calls _end() internally.
  • There is a span.finish() too, which is deprecated from the get go, just to make the migration to span first a bit easier (since the method is called finish() on legacy spans).

Issues

Reminders

Comment on lines +97 to +98
if item._timestamp:
res["end_timestamp"] = item._timestamp.timestamp()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no scenario where a span that's already in the span batcher wouldn't have an end _timestamp, but mypy doesn't know that. 🤡

try:
# profiling depends on this value and requires that
# it is measured in nanoseconds
self._start_timestamp_monotonic_ns = nanosecond_time()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything related to _start_timestamp_monotonic_ns is just copy-pasted from the Span implementation

"""
self._end(end_timestamp)

def finish(self, end_timestamp: "Optional[Union[float, datetime]]" = None) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just here for compat reasons, to make the transition from the old API easier (very slightly).

self.end(end_timestamp)

def _start(self) -> None:
if self._active:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_active basically controls whether a span will be set on the scope, so if it's False, we don't need to keep track of the old parent span to restore it later, because we're never replacing it.

Comment on lines +447 to +452
if self._scope is None:
return

old_span = self._scope.span
self._scope.span = self
self._previous_span_on_scope = old_span
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike the main span class, no-op spans don't have an active flag. The user cannot control whether a no-op span will be set on the scope; the SDK does this internally, and it uses the _scope attribute to remember the decision. In the case of normal spans, the _scope attribute plays a different role: they all need a scope, regardless of whether they'll actually be set on it or not, because they will be captured using that scope in either case.

Coming back to no-op spans, the decision whether to set them on the scope or not is tightly coupled to how we want ignore_spans to work (coming in a future PR). Basically, if a segment/root span would be ignored, all of its children should be as well. That's easiest to accomplish if we actually set the root no-op span on the scope. On the other hand, if a non-root span is ignored, it's not set on scope, so that any children spans effectively use the last not ignored span that's set on the scope as parent.

Base automatically changed from ivana/span-first-8-bucket-based-limits-in-batcher to ivana/span-first-7-add-trace-decorator March 9, 2026 08:50
Base automatically changed from ivana/span-first-7-add-trace-decorator to master March 9, 2026 09:12
@sentrivana sentrivana marked this pull request as ready for review March 9, 2026 10:00
@sentrivana sentrivana requested a review from a team as a code owner March 9, 2026 10:00
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: NoOpStreamedSpan._end() missing double-end guard causes error logs
    • Added a double-end guard in NoOpStreamedSpan._end() that returns when _previous_span_on_scope is already removed, preventing spurious internal error logging on repeated end() calls.

Create PR

Or push these changes by commenting:

@cursor push 048194f3a9
Preview (048194f3a9)
diff --git a/sentry_sdk/traces.py b/sentry_sdk/traces.py
--- a/sentry_sdk/traces.py
+++ b/sentry_sdk/traces.py
@@ -448,7 +448,7 @@
         self._previous_span_on_scope = old_span
 
     def _end(self, end_timestamp: "Optional[Union[float, datetime]]" = None) -> None:
-        if self._scope is None:
+        if self._scope is None or not hasattr(self, "_previous_span_on_scope"):
             return
 
         with capture_internal_exceptions():
This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if value is not None and should_be_treated_as_error(ty, value):
self.status = SpanStatus.ERROR

self._end()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status mutated on already-queued span after early end

Low Severity

When a user calls span.end() inside a with start_span() block and then an exception occurs, __exit__ sets self.status = SpanStatus.ERROR on the already-queued span object before _end() returns early due to its _timestamp is not None guard. Since the batcher holds a reference to the span, the status mutation retroactively changes the queued span from OK to ERROR, even though the span was explicitly finished before the exception happened.

Additional Locations (1)

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant