feat(asgi): Migrate away from event processor in span first#5920
feat(asgi): Migrate away from event processor in span first#5920sentrivana wants to merge 6 commits intomasterfrom
Conversation
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. This PR will not appear in the changelog. 🤖 This preview updates automatically when you update the PR. |
1 similar comment
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. This PR will not appear in the changelog. 🤖 This preview updates automatically when you update the PR. |
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)
Generated by Codecov Action |
| "attributes": { | ||
| k: v["value"] for (k, v) in span_json["attributes"].items() | ||
| }, |
There was a problem hiding this comment.
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
| "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
There was a problem hiding this comment.
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
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
tox -e linters.feat:,fix:,ref:,meta:)