From bf906869fd00726ca7e8fcfc05ddb839dcbc2957 Mon Sep 17 00:00:00 2001 From: William Mak Date: Fri, 22 May 2026 13:49:41 -0400 Subject: [PATCH 1/2] feat(attribute-values): Add counts to response and orderby - This adds counts to the AttributeValues endpoint so its a part of the response and the default sort so we're getting the most common values - Also updates the protos to support this --- pyproject.toml | 2 +- .../web/rpc/v1/trace_item_attribute_values.py | 20 +++++++++++++++++-- .../v1/test_trace_item_attribute_values_v1.py | 14 ++++++++++++- uv.lock | 6 +++--- 4 files changed, 35 insertions(+), 7 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index b5e62c2d54..d8c88ca506 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,7 +31,7 @@ dependencies = [ "sentry-arroyo>=2.38.7", "sentry-conventions>=0.3.0", "sentry-kafka-schemas>=2.1.24", - "sentry-protos>=0.8.14", + "sentry-protos>=0.12.0", "sentry-redis-tools>=0.5.1", "sentry-relay>=0.9.25", "sentry-sdk>=2.35.0", diff --git a/snuba/web/rpc/v1/trace_item_attribute_values.py b/snuba/web/rpc/v1/trace_item_attribute_values.py index 3d670db85a..f3000d298a 100644 --- a/snuba/web/rpc/v1/trace_item_attribute_values.py +++ b/snuba/web/rpc/v1/trace_item_attribute_values.py @@ -25,7 +25,7 @@ from snuba.query.data_source.simple import Entity, LogicalDataSource from snuba.query.dsl import Functions as f from snuba.query.dsl import column -from snuba.query.expressions import Expression +from snuba.query.expressions import Expression, FunctionCall from snuba.query.logical import Query from snuba.query.query_settings import HTTPQuerySettings from snuba.request import Request as SnubaRequest @@ -128,10 +128,20 @@ def _build_query( name="attr_value", expression=f.distinct(column(attr_value.alias, alias="attr_value")), ), + SelectedExpression( + name="count()", + expression=FunctionCall( + alias="count()", + function_name="count", + parameters=(), + ), + ), ], order_by=[ + OrderBy(direction=OrderByDirection.DESC, expression=column("count()")), OrderBy(direction=OrderByDirection.ASC, expression=column("attr_value")), ], + groupby=[column("attr_value")], limit=request.limit, offset=(request.page_token.offset if request.page_token.HasField("offset") else 0), ) @@ -184,6 +194,7 @@ def _execute(self, in_msg: TraceItemAttributeValuesRequest) -> TraceItemAttribut if in_msg.key.name == "sentry.item_id" and in_msg.value_substring_match: return TraceItemAttributeValuesResponse( values=[in_msg.value_substring_match], + counts=[1], page_token=None, ) in_msg.limit = in_msg.limit or 1000 @@ -193,14 +204,19 @@ def _execute(self, in_msg: TraceItemAttributeValuesRequest) -> TraceItemAttribut request=snuba_request, timer=self._timer, ) - values = [r["attr_value"] for r in res.result.get("data", [])] + values, counts = [], [] + for row in res.result.get("data", []): + values.append(row["attr_value"]) + counts.append(row.get("count()", 0)) if len(values) == 0: return TraceItemAttributeValuesResponse( values=values, + counts=counts, page_token=None, ) return TraceItemAttributeValuesResponse( values=values, + counts=counts, page_token=( PageToken(offset=in_msg.page_token.offset + len(values)) if in_msg.page_token.HasField("offset") or len(values) == 0 diff --git a/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py b/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py index 7c05fe62b9..59c84bc094 100644 --- a/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py +++ b/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py @@ -93,6 +93,13 @@ def setup_teardown(eap: None, redis_db: None) -> Generator[List[bytes], None, No "tag2": AnyValue(string_value="derp"), }, ), + gen_item_message( + start_timestamp=start_timestamp, + attributes={ + "tag1": AnyValue(string_value="derpderp"), + "tag2": AnyValue(string_value="derp"), + }, + ), gen_item_message( start_timestamp=start_timestamp, attributes={"tag2": AnyValue(string_value="hehe")}, @@ -135,7 +142,8 @@ def test_simple_case(self, setup_teardown: Any) -> None: key=AttributeKey(name="tag1", type=AttributeKey.TYPE_STRING), ) response = AttributeValuesRequest().execute(message) - assert response.values == ["blah", "derpderp", "durp", "herp", "herpderp"] + assert response.values == ["derpderp", "blah", "durp", "herp", "herpderp"] + assert response.counts == [2, 1, 1, 1, 1] def test_with_value_substring_match(self, setup_teardown: Any) -> None: message = TraceItemAttributeValuesRequest( @@ -146,6 +154,7 @@ def test_with_value_substring_match(self, setup_teardown: Any) -> None: ) response = AttributeValuesRequest().execute(message) assert response.values == ["derpderp", "herp", "herpderp"] + assert response.counts == [2, 1, 1] def test_empty_results(self) -> None: req = TraceItemAttributeValuesRequest( @@ -162,6 +171,7 @@ def test_empty_results(self) -> None: ) res = AttributeValuesRequest().execute(req) assert res.values == [] + assert res.counts == [] def test_item_id_substring_match(self, setup_teardown: List[bytes]) -> None: first_msg_bytes = setup_teardown[0] @@ -182,6 +192,7 @@ def test_item_id_substring_match(self, setup_teardown: List[bytes]) -> None: ) res = AttributeValuesRequest().execute(req) assert res.values == [item_id] + assert res.counts == [1] def test_deprecated_alias_attribute(self) -> None: """db.system.name request returns values stored only under deprecated key db.system.""" @@ -218,3 +229,4 @@ def test_deprecated_alias_attribute(self) -> None: ) response = AttributeValuesRequest().execute(message) assert sorted(response.values) == ["postgresql", "redis"] + assert response.counts == [1, 1] diff --git a/uv.lock b/uv.lock index cbb0565717..1647b8b603 100644 --- a/uv.lock +++ b/uv.lock @@ -950,7 +950,7 @@ wheels = [ [[package]] name = "sentry-protos" -version = "0.8.14" +version = "0.12.0" source = { registry = "https://pypi.devinfra.sentry.io/simple" } dependencies = [ { name = "grpc-stubs", marker = "sys_platform == 'darwin' or sys_platform == 'linux'" }, @@ -958,7 +958,7 @@ dependencies = [ { name = "protobuf", marker = "sys_platform == 'darwin' or sys_platform == 'linux'" }, ] wheels = [ - { url = "https://pypi.devinfra.sentry.io/wheels/sentry_protos-0.8.14-py3-none-any.whl", hash = "sha256:109eb990d27c193ce7878fa3a37f3cb9bc4c9a57ad8d454ed8a8e6f104f0c21a" }, + { url = "https://pypi.devinfra.sentry.io/wheels/sentry_protos-0.12.0-py3-none-any.whl", hash = "sha256:4b838858b37fde7cbea821ea665d2313e764376810d381dccbf129dbba143810" }, ] [[package]] @@ -1158,7 +1158,7 @@ requires-dist = [ { name = "sentry-arroyo", specifier = ">=2.38.7" }, { name = "sentry-conventions", specifier = ">=0.3.0" }, { name = "sentry-kafka-schemas", specifier = ">=2.1.24" }, - { name = "sentry-protos", specifier = ">=0.8.14" }, + { name = "sentry-protos", specifier = ">=0.12.0" }, { name = "sentry-redis-tools", specifier = ">=0.5.1" }, { name = "sentry-relay", specifier = ">=0.9.25" }, { name = "sentry-sdk", specifier = ">=2.35.0" }, From 6da95a7bddeb99c3cb6b3a28febadb821769b8a5 Mon Sep 17 00:00:00 2001 From: William Mak Date: Wed, 3 Jun 2026 14:47:16 -0400 Subject: [PATCH 2/2] fix: Address PR comments - Removed the page_token that used the filter offset, can't do this with count() since PageToken doesn't accept Aggregate Filters - Related, this endpoint is only being called with offset by the Sentry application anyways so this should be fine - Add a pagination test --- .../web/rpc/v1/trace_item_attribute_values.py | 21 +++++-------------- .../v1/test_trace_item_attribute_values_v1.py | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/snuba/web/rpc/v1/trace_item_attribute_values.py b/snuba/web/rpc/v1/trace_item_attribute_values.py index f3000d298a..7a3057cb8d 100644 --- a/snuba/web/rpc/v1/trace_item_attribute_values.py +++ b/snuba/web/rpc/v1/trace_item_attribute_values.py @@ -7,11 +7,7 @@ TraceItemAttributeValuesResponse, ) from sentry_protos.snuba.v1.request_common_pb2 import PageToken -from sentry_protos.snuba.v1.trace_item_attribute_pb2 import AttributeKey, AttributeValue -from sentry_protos.snuba.v1.trace_item_filter_pb2 import ( - ComparisonFilter, - TraceItemFilter, -) +from sentry_protos.snuba.v1.trace_item_attribute_pb2 import AttributeKey from snuba.attribution.appid import AppID from snuba.attribution.attribution_info import AttributionInfo @@ -126,7 +122,7 @@ def _build_query( selected_columns=[ SelectedExpression( name="attr_value", - expression=f.distinct(column(attr_value.alias, alias="attr_value")), + expression=column(attr_value.alias, alias="attr_value"), ), SelectedExpression( name="count()", @@ -218,16 +214,9 @@ def _execute(self, in_msg: TraceItemAttributeValuesRequest) -> TraceItemAttribut values=values, counts=counts, page_token=( - PageToken(offset=in_msg.page_token.offset + len(values)) - if in_msg.page_token.HasField("offset") or len(values) == 0 - else PageToken( - filter_offset=TraceItemFilter( - comparison_filter=ComparisonFilter( - key=AttributeKey(type=AttributeKey.TYPE_STRING, name="attr_value"), - op=ComparisonFilter.OP_GREATER_THAN, - value=AttributeValue(val_str=values[-1]), - ) - ) + PageToken( + offset=(in_msg.page_token.offset if in_msg.page_token.HasField("offset") else 0) + + len(values) ) ), ) diff --git a/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py b/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py index 59c84bc094..d3fb60fe70 100644 --- a/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py +++ b/tests/web/rpc/v1/test_trace_item_attribute_values_v1.py @@ -230,3 +230,24 @@ def test_deprecated_alias_attribute(self) -> None: response = AttributeValuesRequest().execute(message) assert sorted(response.values) == ["postgresql", "redis"] assert response.counts == [1, 1] + + def test_pagination(self, setup_teardown: Any) -> None: + message = TraceItemAttributeValuesRequest( + meta=COMMON_META, + limit=1, + key=AttributeKey(name="tag1", type=AttributeKey.TYPE_STRING), + ) + response = AttributeValuesRequest().execute(message) + assert response.values == ["derpderp"] + assert response.counts == [2] + + for expected in ["blah", "durp", "herp", "herpderp"]: + message = TraceItemAttributeValuesRequest( + meta=COMMON_META, + limit=1, + key=AttributeKey(name="tag1", type=AttributeKey.TYPE_STRING), + page_token=response.page_token, + ) + response = AttributeValuesRequest().execute(message) + assert response.values == [expected] + assert response.counts == [1]