Skip to content

fix(eap): Handle contexts in trace-item attributes#112524

Merged
wmak merged 3 commits intomasterfrom
wmak/fix/handle-contexts-in-trace-item-attributes
Apr 10, 2026
Merged

fix(eap): Handle contexts in trace-item attributes#112524
wmak merged 3 commits intomasterfrom
wmak/fix/handle-contexts-in-trace-item-attributes

Conversation

@wmak
Copy link
Copy Markdown
Member

@wmak wmak commented Apr 8, 2026

  • Take contexts into account for the trace item attributes so we return them as possible fields

- Take contexts into account for the trace item attributes so we return
  them as possible fields
@wmak wmak requested review from a team as code owners April 8, 2026 20:40
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 8, 2026
Comment thread src/sentry/api/endpoints/organization_trace_item_attributes.py Outdated
Copy link
Copy Markdown
Contributor

@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: Non-string context attributes get wrong public key format
    • I confirmed proxy-backed boolean contexts were incorrectly emitted as tags syntax and fixed as_attribute_key to keep proxy keys as their direct context name, with a regression test for is_starred_transaction.

Create PR

Or push these changes by commenting:

@cursor push 27e6082d25
Preview (27e6082d25)
diff --git a/src/sentry/api/endpoints/organization_trace_item_attributes.py b/src/sentry/api/endpoints/organization_trace_item_attributes.py
--- a/src/sentry/api/endpoints/organization_trace_item_attributes.py
+++ b/src/sentry/api/endpoints/organization_trace_item_attributes.py
@@ -239,6 +239,9 @@
 
     if public_key is not None and public_name is not None:
         pass
+    elif is_proxy:
+        public_key = name
+        public_name = name
     elif attr_type == "number":
         public_key = f"tags[{name},number]"
         public_name = name

diff --git a/tests/snuba/api/endpoints/test_organization_trace_item_attributes.py b/tests/snuba/api/endpoints/test_organization_trace_item_attributes.py
--- a/tests/snuba/api/endpoints/test_organization_trace_item_attributes.py
+++ b/tests/snuba/api/endpoints/test_organization_trace_item_attributes.py
@@ -950,6 +950,16 @@
         assert "tags[is_debug,boolean]" in keys
         assert "tags[is_production,boolean]" in keys
 
+    def test_boolean_virtual_context_key_format(self) -> None:
+        response = self.do_request(
+            query={"attributeType": "boolean", "substringMatch": "is_starred_transaction"}
+        )
+        assert response.status_code == 200, response.content
+
+        keys = {item["key"] for item in response.data}
+        assert "is_starred_transaction" in keys
+        assert "tags[is_starred_transaction,boolean]" not in keys
+
     def test_aliased_attribute(self) -> None:
         span1 = self.create_span(
             {"sentry_tags": {"op": "foo"}}, start_ts=before_now(days=0, minutes=10)

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

Comment thread src/sentry/api/endpoints/organization_trace_item_attributes.py Outdated
- Fix a bug with pagination
Comment on lines +580 to +581
if attr_key["name"] in attribute_keys:
del attribute_keys[attr_key["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.

exclude_attributes loop checks wrong dict key, exclusions never happen for tag attributes

The attribute_keys dict is populated using attr_key["key"] as the dictionary key (line 570), but the exclusion loop at lines 580-581 checks membership and deletes using attr_key["name"]. For tag attributes, these differ: "key" is like "tags[my_tag,number]" while "name" is just "my_tag". Since the membership check fails, the exclusion is silently skipped, causing duplicate attributes to appear across pagination pages.

Verification

Read as_attribute_key (lines 244-252) which shows key = f"tags[{name},number]" while name = name. Confirmed attribute_keys is populated with ["key"] at line 570. The membership check at line 580 uses ["name"] which won't match the stored keys for tag attributes.

Suggested fix: Use attr_key["key"] consistently for both dict key storage and deletion lookup

Suggested change
if attr_key["name"] in attribute_keys:
del attribute_keys[attr_key["name"]]
if attr_key["key"] in attribute_keys:
del attribute_keys[attr_key["key"]]

Identified by Warden sentry-backend-bugs · 3GY-QXY

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.

Fix attempt detected (commit 63b88cd)

The code was refactored to use a dictionary approach at line 570 with attr_key['key'] as the key, but the exclusion logic at lines 580-581 still incorrectly uses attr_key['name'] for both the membership check and deletion, so the fix was attempted but the core issue persists.

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

Evaluated by Warden

Comment thread src/sentry/api/endpoints/organization_trace_item_attributes.py
Comment on lines 445 to 448
aliased_attributes = all_aliased_attributes[offset : offset + limit]
with sentry_sdk.start_span(op="query", name="attribute_names") as span:
if len(aliased_attributes) < limit - 1:
if len(aliased_attributes) < limit:
offset -= len(all_aliased_attributes) - len(aliased_attributes)
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.

Bug: When paginating without a substring_match, the offset calculation for the RPC call is incorrect because it fails to account for standard columns, leading to incorrect pagination results.
Severity: HIGH

Suggested Fix

Adjust the offset calculation to correctly account for the items that are not included in all_aliased_attributes when substring_match is falsy. The calculation should subtract the number of standard columns (len(column_definitions.columns)) from the offset if the pagination has moved past them.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/api/endpoints/organization_trace_item_attributes.py#L445-L448

Potential issue: In `query_trace_attributes`, when paginating without a
`substring_match` filter, the code incorrectly calculates the offset for the subsequent
RPC request. The logic populates `all_aliased_attributes` with only context attributes,
but the offset calculation `offset -= len(all_aliased_attributes) -
len(aliased_attributes)` implicitly assumes it also contains standard column attributes
that were conceptually skipped by the pagination offset. This results in an incorrect
offset being sent to the backend, leading to missing or duplicated attributes in the
paginated response.

Copy link
Copy Markdown
Contributor

@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 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 53fd538. Configure here.

)
)
if attr_key["name"] in attribute_keys:
del attribute_keys[attr_key["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.

Dictionary keyed by "key" but exclusion uses "name"

Medium Severity

In serialize_trace_attributes, the attribute_keys dict is keyed by attr_key["key"] (line 570 and 594), but the exclusion loop checks and deletes using attr_key["name"] (lines 580–581). For number/boolean attributes, "key" is tags[name,type] while "name" is just the raw name, so the lookup silently fails and exclusion doesn't work. The sibling function serialize_trace_attributes_using_sentry_conventions consistently uses attr_key["name"] throughout, so this appears to be an oversight.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 53fd538. Configure here.

internal_name=public_label,
search_type=virtual_context.search_type,
)
)
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.

Identical context iteration duplicated across if/else branches

Low Severity

The context iteration in the else branch (lines 427–444) is an exact copy of the context iteration in the if substring_match branch (lines 410–426). Since substring_match is "" in the else branch, the substring_match in public_label check is always True, making the two blocks functionally identical. The context loop could be extracted outside the if/else to eliminate this duplication.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 53fd538. Configure here.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

Backend Test Failures

Failures on 2457edf in this run:

tests/snuba/api/endpoints/test_seer_attributes.py::OrganizationTraceItemAttributesEndpointSpansTest::test_get_attribute_nameslog
[gw1] linux -- Python 3.13.1 /home/runner/work/sentry/sentry/.venv/bin/python3
tests/snuba/api/endpoints/test_seer_attributes.py:42: in test_get_attribute_names
    assert result == {
E   AssertionError: assert {'built_in_fi..., 'project']}} == {'built_in_fi...ransaction']}}
E     
E     Omitting 1 identical items, use -vv to show
E     Differing items:
E     {'fields': {'number': ['span.duration'], 'string': ['span.description', 'transaction', 'device.class', 'span.module', 'project']}} != {'fields': {'number': ['span.duration'], 'string': ['span.description', 'project', 'transaction']}}
E     
E     Full diff:
E       {
E           'built_in_fields': [
E               {
E                   'key': 'id',
E                   'type': 'string',
E               },
E               {
E                   'key': 'project',
E                   'type': 'string',
E               },
E               {
E                   'key': 'span.description',
E                   'type': 'string',
E               },
E               {
E                   'key': 'span.op',
E                   'type': 'string',
E               },
E               {
E                   'key': 'timestamp',
E                   'type': 'string',
E               },
E               {
E                   'key': 'transaction',
E                   'type': 'string',
E               },
E               {
E                   'key': 'trace',
E                   'type': 'string',
E               },
E               {
E                   'key': 'is_transaction',
E                   'type': 'string',
E               },
E               {
E                   'key': 'sentry.normalized_description',
E                   'type': 'string',
E               },
E               {
E                   'key': 'release',
... (45 more lines)

Comment on lines +427 to +436
else:
for (
public_label,
virtual_context,
) in column_definitions.contexts.items():
if (
substring_match in public_label
and virtual_context.search_type is not None
and not virtual_context.secondary_alias
and constants.TYPE_MAP[virtual_context.search_type] == attr_type
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.

Bug: When substring_match is empty, the else branch in query_trace_attributes fails to iterate over column_definitions.columns, breaking pagination logic.
Severity: MEDIUM

Suggested Fix

Update the else block to mirror the if block's logic. It should iterate through column_definitions.columns and append them to the all_aliased_attributes list, ensuring consistent pagination behavior regardless of whether substring_match is provided.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/api/endpoints/organization_trace_item_attributes.py#L427-L436

Potential issue: In the `query_trace_attributes` method, when `substring_match` is an
empty string, the logic enters an `else` block that only processes
`column_definitions.contexts`. It omits iterating over `column_definitions.columns`,
unlike the `if` block which handles both. This omission results in an incomplete
`all_aliased_attributes` list, which is critical for pagination. Consequently,
pagination offset calculations will be incorrect, leading to skipped, repeated, or
duplicate attributes in the API response when no substring filter is applied.

@wmak wmak merged commit 60656d2 into master Apr 10, 2026
63 checks passed
@wmak wmak deleted the wmak/fix/handle-contexts-in-trace-item-attributes branch April 10, 2026 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants