Skip to content

feat(tornado): Support span streaming#6206

Open
sl0thentr0py wants to merge 1 commit intomasterfrom
neel/span-first/tornado
Open

feat(tornado): Support span streaming#6206
sl0thentr0py wants to merge 1 commit intomasterfrom
neel/span-first/tornado

Conversation

@sl0thentr0py
Copy link
Copy Markdown
Member

Issues

Add span-streaming support to the Tornado integration. When span
streaming is enabled, the request handler emits a StreamedSpan with
HTTP request attributes (method, headers, query, URL, client address)
and sets the response status on completion. The legacy transaction
path is preserved for non-streaming mode.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@sl0thentr0py sl0thentr0py requested a review from a team as a code owner May 5, 2026 13:32
@linear-code
Copy link
Copy Markdown

linear-code Bot commented May 5, 2026

@sl0thentr0py sl0thentr0py changed the title ref(tornado): migrate to span-first ref(tornado): migrate to span first May 5, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

Codecov Results 📊

13 passed | Total: 13 | Pass Rate: 100% | Execution Time: 8.94s

All tests are passing successfully.

❌ Patch coverage is 6.52%. Project has 15006 uncovered lines.

Files with missing lines (1)
File Patch % Lines
tornado.py 11.76% ⚠️ 135 Missing

Generated by Codecov Action

@sl0thentr0py sl0thentr0py changed the title ref(tornado): migrate to span first feat(tornado): Support span streaming May 5, 2026
Copy link
Copy Markdown
Contributor

@alexander-alderman-webb alexander-alderman-webb left a comment

Choose a reason for hiding this comment

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

Comment not blocking, looks good to me!

Comment on lines +171 to +173
tx_name = transaction_from_function(method) or ""
if tx_name:
span.name = tx_name
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fallback is dead code and I'm not sure if we want transaction concepts to bleed into the streaming path when it comes to variable naming.

Suggested change
tx_name = transaction_from_function(method) or ""
if tx_name:
span.name = tx_name
span_name = transaction_from_function(method)
if span_name:
span.name = span_name

Comment on lines +158 to 165
server_segment = next(
s for s in spans if s["attributes"].get("sentry.op") == "http.server"
)
client_segment = next(
s
for s in spans
if s["attributes"].get("sentry.op") != "http.server" and s.get("is_segment")
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we unfurl the spans directly without this filtering? In the legacy path, we implicitly assert on the number and order of spans by assigning them directly; would be good to preserve this.

The clanker loves to do this sort of filtering instead for some reason, but it does get it if you tell it to unfurl the items directly.

Comment on lines +532 to +535
segment = next(
s for s in spans if s["attributes"].get("sentry.op") == "http.server"
)
assert segment["attributes"]["sentry.origin"] == "auto.http.tornado"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same comment re: unfurling here

span.status = "error" if status_int >= 400 else "ok"


def _get_request_attributes(request: "Any") -> "Dict[str, Any]":
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we still capturing the request body in the streaming path? See https://getsentry.github.io/sentry-conventions/attributes/http/#http-request-body-data

For prior art IIRC @ericapisani did this for another integration, not sure which exactly

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

starlette and aiohttp were the couple that I worked on that had this


weak_handler = weakref.ref(self)
client = sentry_sdk.get_client()
span_streaming = has_span_streaming_enabled(client.options)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Because this is communicating a state rather than a "thing", would suggest a name like is_span_streaming_enabled

span.name = tx_name
span.set_attribute(
"sentry.span.source",
SegmentSource.COMPONENT.value,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🧹 : both approaches work, but I noticed that this spot uses .value and line 143 doesn't. As a clean up, would be good to make this consistent.

span.status = "error" if status_int >= 400 else "ok"


def _get_request_attributes(request: "Any") -> "Dict[str, Any]":
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

starlette and aiohttp were the couple that I worked on that had this

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate tornado to span first

4 participants