feat(eap): Sort trace item attribute names by frequency#8062
Merged
Conversation
Return attribute keys from TraceItemAttributeNamesRequest sorted by frequency (count) descending, with name ascending as a tiebreaker, so high-cardinality / commonly-used attributes surface first. The co-occurring attributes query now groups by attr_key and counts occurrences instead of using distinct(), and orders by count DESC, attr_key ASC. Synthetic non-stored attributes are assigned an infinite count so they continue to sort first. This mirrors the pattern used by the attribute values endpoint. Refs EAP-432 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0123HwN9no4ce3knzFWh9feV
phacops
added a commit
to getsentry/sentry
that referenced
this pull request
Jun 18, 2026
## Summary `OrganizationTraceItemAttributesEndpointSpansTest::test_tags_list_nums` asserted the response with an exact, order-sensitive list comparison (`assert response.data == [...]`). The number-attributes endpoint does not guarantee a stable ordering of returned values, so the test should assert that all expected values are present rather than depending on their order. This surfaced via snuba PR getsentry/snuba#8062, which changes the default sort. ## Change Switched the assertion to the order-independent pattern already used by neighboring tests in this file — sorting both sides by `key`: ```python assert sorted(response.data, key=itemgetter("key")) == sorted( [ ...expected values... ], key=itemgetter("key"), ) ``` `itemgetter` was already imported and this idiom matches the surrounding tests (`test_pagination`, etc.). ## Test Plan - `ruff format --check` and `ruff check` pass on the modified file. - Assertion now verifies all expected number attributes are present regardless of order. https://claude.ai/code/session_01WLKmLXRfRyJpW9u8zEGsy1 --- _Generated by [Claude Code](https://claude.ai/code/session_01WLKmLXRfRyJpW9u8zEGsy1)_ Co-authored-by: Claude <noreply@anthropic.com>
Instead of changing the default ordering of TraceItemAttributeNamesRequest (which reorders results for every consumer and broke downstream sentry tests), honor the new opt-in order_by field from sentry-protos: - order_by unset / COLUMN_NAME: the existing distinct, name-ascending result, unchanged for current consumers. - order_by.column = COLUMN_COUNT: group by key, count occurrences, order by count (direction from `descending`) then name ASC, and populate the new optional Attribute.count field in the response. Gated on sentry-protos 0.31.2, which adds TraceItemAttributeNamesRequest.OrderBy and Attribute.count. The pyproject/uv.lock and rust_snuba Cargo pins must be bumped to that release before CI can pass; until then this branch stays a draft. Refs EAP-432
The v1 co-occurring table's count() is a row count (approximate, not the literal per-item span count), so assert that high-frequency keys outrank low-frequency ones rather than pinning an exact value. Ordering behavior is unchanged; exact per-item counts come with the v2 storage follow-up.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Seer review caught that the in-memory sort used reverse=descending with an infinite-count sentinel, so under ascending count ordering the synthetic non-stored attribute (sentry.service) sorted last instead of first. Now the real counted rows are sorted in the requested direction and the non-stored attributes are prepended unconditionally, which also keeps the sentinel out of the response. Adds a regression test covering both directions.
MeredithAnya
approved these changes
Jun 18, 2026
…son-p024mw # Conflicts: # snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
Reviewed by Cursor Bugbot for commit 4b59a9c. Configure here.
…g results Cursor Bugbot caught that the in-memory re-sort in convert_co_occurring_results_to_attributes always sorted by name ascending, undoing a COLUMN_NAME + descending request that the ClickHouse query had ordered DESC. Make the re-sort direction-aware via a shared _order_by_name_descending helper (also used by the query builder) and add a regression test for descending name ordering.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds opt-in frequency sorting to
TraceItemAttributeNamesRequest, so callers can requestsort:-count()without changing the default order for everyone (EAP-432).Behavior
order_byunset /COLUMN_NAME→ the existingdistinct, name-ascending result. Unchanged for all current consumers (this is what keeps downstream sentry tests green).order_by.column = COLUMN_COUNT→ group by key,count()occurrences, order bycount(direction fromdescending) then name ASC, and populate the new optionalAttribute.countfield in the response.This replaces the earlier approach on this branch, which changed the default order and broke order-pinned tests in
getsentry/sentry(e.g.test_tags_list_nums,test_pagination).Blocked on sentry-protos
Depends on sentry-protos
0.31.2(getsentry/sentry-protos#316), which addsTraceItemAttributeNamesRequest.OrderBy { Column column; bool descending }andTraceItemAttributeNamesResponse.Attribute.count.Before CI can pass / this can leave draft:
0.31.2published.pyproject.toml+ regenerateuv.lock, and (if needed)rust_snuba/Cargo.toml+Cargo.lock.Until then this PR is intentionally a draft and its CI will be red (the new proto symbols don't exist in the pinned version yet).
Refs EAP-432
🤖 Generated with Claude Code
https://claude.ai/code/session_0123HwN9no4ce3knzFWh9feV