Skip to content

feat(asgi): Migrate away from event processor in span first#5920

Draft
sentrivana wants to merge 6 commits intomasterfrom
ivana/migrate-asgi-event-processor
Draft

feat(asgi): Migrate away from event processor in span first#5920
sentrivana wants to merge 6 commits intomasterfrom
ivana/migrate-asgi-event-processor

Conversation

@sentrivana
Copy link
Copy Markdown
Contributor

Description

In span first, there are no event processors. Therefore, we need to be able to set the data we were setting in event processors in some other way.

As we migrate our integrations one by one, this will be an exercise in whether it's possible to achieve this without some sort of callback/lifecycle hooks. So far, in ASGI, it seems we can get by by simply using scope.set_attribute() for setting request related data, and updating the segment name/source just before the span ends.

Adding this enables us to actually test the new functionality.

Issues

Reminders

@linear-code
Copy link
Copy Markdown

linear-code bot commented Mar 30, 2026

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 30, 2026

Semver Impact of This PR

🟡 Minor (new features)

📋 Changelog Preview

This is how your changes will appear in the changelog.
Entries from this PR are highlighted with a left border (blockquote style).


This PR will not appear in the changelog.


🤖 This preview updates automatically when you update the PR.

1 similar comment
@github-actions
Copy link
Copy Markdown
Contributor

Semver Impact of This PR

🟡 Minor (new features)

📋 Changelog Preview

This is how your changes will appear in the changelog.
Entries from this PR are highlighted with a left border (blockquote style).


This PR will not appear in the changelog.


🤖 This preview updates automatically when you update the PR.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 30, 2026

Codecov Results 📊

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

All tests are passing successfully.

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

Files with missing lines (2)
File Patch % Lines
asgi.py 15.76% ⚠️ 171 Missing
_asgi_common.py 14.49% ⚠️ 59 Missing

Generated by Codecov Action

Comment on lines +1230 to +1232
"attributes": {
k: v["value"] for (k, v) in span_json["attributes"].items()
},
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

envelopes_to_spans raises KeyError when span has no attributes

The function directly accesses span_json["attributes"] which will raise a KeyError if the span has no attributes. Looking at _span_batcher.py:116-119, the attributes field is only serialized when item._attributes is truthy - spans without attributes won't have this key in the JSON. This will cause test failures if any span being parsed lacks attributes.

Verification

Read _span_batcher.py and confirmed _to_transport_format conditionally adds attributes (if item._attributes:). Read conftest.py and confirmed envelopes_to_spans uses direct dictionary access span_json["attributes"] without .get(). This would raise KeyError for spans serialized without attributes.

Suggested fix: Use .get() with an empty dict default to safely handle spans without attributes

Suggested change
"attributes": {
k: v["value"] for (k, v) in span_json["attributes"].items()
},
k: v["value"] for (k, v) in span_json.get("attributes", {}).items()

Identified by Warden code-review · 23K-KC9

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fix attempt detected (commit e93fd1b)

The envelopes_to_spans function was added to fix the issue, but line 1231 still uses direct dictionary access span_json["attributes"].items() instead of the suggested .get("attributes", {}) pattern, leaving the KeyError vulnerability unresolved for spans without attributes.

The original issue appears unresolved. Please review and try again.

Evaluated by Warden

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.

Event processor replacement for streamed spans

1 participant