diff --git a/pyproject.toml b/pyproject.toml index 7899312cbf..64b80a957f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -33,7 +33,7 @@ dependencies = [ "sentry-conventions>=0.17.0", "sentry-kafka-schemas>=2.1.39", "sentry-options>=1.2.1", - "sentry-protos>=0.46.0", + "sentry-protos>=0.47.0", "sentry-redis-tools>=0.5.1", "sentry-relay>=0.9.25", "sentry-sdk>=2.35.0", diff --git a/snuba/web/rpc/common/common.py b/snuba/web/rpc/common/common.py index 809d9c0b1f..403f24475c 100644 --- a/snuba/web/rpc/common/common.py +++ b/snuba/web/rpc/common/common.py @@ -92,6 +92,41 @@ def attribute_key_to_expression(attr_key: AttributeKey) -> Expression: raise BadSnubaRPCRequestException(str(e)) from e +_SEMVER_COMPONENT_COUNT = 4 # major.minor.patch.build + + +def semver_sort_key(expr: Expression, alias: str | None = None) -> Expression: + """Return a Tuple(Array(UInt32), UInt8, String) semver sort key for ``SORT_SEMVER``. + + Callers opt in per request (no hardcoded attribute list) and the key is + applied to whatever column they order by. Strips a 'package@' prefix and + '+build' metadata, maps the release part to 4 UInt32 components (so "1.2" == + "1.2.0"), then adds a stability flag (0=prerelease, 1=stable) so prereleases + sort before their stable release, and the raw string as a tiebreaker for a + deterministic total order. Works on Altinity 25.3/25.8 (no naturalSortKey). + """ + x = Argument(None, "x") + # sentry.release is coalesced, so Nullable(String); strip the nullable + # wrapper (ClickHouse forbids Nullable(Array(…))) before string→array funcs. + non_null = f.ifNull(expr, literal("")) + version_no_prefix = f.arrayElement(f.splitByChar(literal("@"), non_null), literal(-1)) + # Drop build metadata (does not affect precedence); left attached it would + # zero the last component and fail the stability match. + version_no_build = f.arrayElement(f.splitByChar(literal("+"), version_no_prefix), literal(1)) + release_part = f.arrayElement(f.splitByChar(literal("-"), version_no_build), literal(1)) + numeric_key = f.arrayResize( + f.arrayMap( + Lambda(None, ("x",), f.toUInt32OrZero(x)), + f.splitByChar(literal("."), release_part), + ), + literal(_SEMVER_COMPONENT_COUNT), + ) + # Stable only for plain dotted-numeric versions; anything else (SemVer + # "-beta.1" or PEP 440 dot-dev "24.7.0.dev0+") is a prerelease. + is_stable = f.match(version_no_build, literal(r"^[0-9]+(\.[0-9]+)*$")) + return FunctionCall(alias, "tuple", (numeric_key, is_stable, non_null)) + + def _trace_item_filter_key_expression( attr_to_key_expression_callable: Callable[[AttributeKey], Expression], key: AttributeKey, diff --git a/snuba/web/rpc/common/pagination.py b/snuba/web/rpc/common/pagination.py index 5abcd23f9c..f9c3693ec0 100644 --- a/snuba/web/rpc/common/pagination.py +++ b/snuba/web/rpc/common/pagination.py @@ -18,7 +18,10 @@ from snuba.query.dsl import Functions as f from snuba.query.dsl import column, literal from snuba.query.expressions import Expression -from snuba.web.rpc.common.common import attribute_key_to_expression +from snuba.web.rpc.common.common import ( + attribute_key_to_expression, + semver_sort_key, +) from snuba.web.rpc.storage_routing.routing_strategies.storage_routing import TimeWindow @@ -27,6 +30,9 @@ class FlexibleTimeWindowPageWithFilters: _TIME_WINDOW_START_KEY = f"{_TIME_WINDOW_PREFIX}.start_timestamp" _TIME_WINDOW_END_KEY = f"{_TIME_WINDOW_PREFIX}.end_timestamp" _FILTER_PREFIX = "sentry__filter" + # Marks a page-boundary column whose ORDER BY used SORT_SEMVER, so + # get_filters applies the semver key on both sides of the comparison. + _SEMVER_FILTER_PREFIX = "sentry__semver_filter" def __init__(self, page_token: PageToken): self._page_token = page_token @@ -70,38 +76,66 @@ def get_filters(self) -> Expression | None: column_names: list[str] = [] column_values: list[Expression] = [] + # Parallel to column_names: True when that column's ORDER BY used + # SORT_SEMVER, so the boundary comparison must use the semver key too. + column_is_semver: list[bool] = [] for filter in self.page_token.filter_offset.and_filter.filters: - if filter.HasField( - "comparison_filter" - ) and filter.comparison_filter.key.name.startswith(self._FILTER_PREFIX): - if filter.comparison_filter.key.name == f"{self._FILTER_PREFIX}.timestamp": - column_names.append("timestamp") - if filter.comparison_filter.value.HasField("val_str"): - column_values.append(f.toDateTime(filter.comparison_filter.value.val_str)) - elif filter.comparison_filter.value.HasField("val_double"): - column_values.append(literal(filter.comparison_filter.value.val_double)) - elif filter.comparison_filter.value.HasField("val_int"): - column_values.append(literal(filter.comparison_filter.value.val_int)) - + if not filter.HasField("comparison_filter"): + continue + key_name = filter.comparison_filter.key.name + is_semver = key_name.startswith(f"{self._SEMVER_FILTER_PREFIX}.") + is_regular = key_name.startswith(f"{self._FILTER_PREFIX}.") + if not (is_semver or is_regular): + continue + + if key_name == f"{self._FILTER_PREFIX}.timestamp": + # Resolve the value first and raise on an unsupported type + # (tokens are client-supplied) so the parallel lists stay in + # sync and the strict zip() below can't crash. Mirrors create(). + value = filter.comparison_filter.value + if value.HasField("val_str"): + column_values.append(f.toDateTime(value.val_str)) + elif value.HasField("val_double"): + column_values.append(literal(value.val_double)) + elif value.HasField("val_int"): + column_values.append(literal(value.val_int)) else: - # strip the _FILTER_PREFIX from the attribute key and the dot - column_names.append( - filter.comparison_filter.key.name[len(self._FILTER_PREFIX) + 1 :] + raise ValueError( + f"Timestamp value type {value.WhichOneof('value')} not supported " + "in page token" ) - column_values.append( - literal( - getattr( - filter.comparison_filter.value, - str(filter.comparison_filter.value.WhichOneof("value")), - ) + column_names.append("timestamp") + column_is_semver.append(False) + else: + # strip the matching prefix (and the dot) to recover the alias + prefix = self._SEMVER_FILTER_PREFIX if is_semver else self._FILTER_PREFIX + column_names.append(key_name[len(prefix) + 1 :]) + column_is_semver.append(is_semver) + column_values.append( + literal( + getattr( + filter.comparison_filter.value, + str(filter.comparison_filter.value.WhichOneof("value")), ) ) + ) # Assumes everything in the ORDER BY is ordered by DESC if column_names: - res = f.less( - f.tuple(*(column(c_name) for c_name in column_names)), f.tuple(*column_values) - ) + col_exprs = [] + val_exprs = [] + for c_name, c_value, is_semver in zip( + column_names, column_values, column_is_semver, strict=True + ): + # For SORT_SEMVER columns, apply the same semver key on both sides + # so the page-boundary comparison uses the same ordering as ORDER BY. + if is_semver: + col_exprs.append(semver_sort_key(column(c_name))) + val_exprs.append(semver_sort_key(c_value)) + else: + col_exprs.append(column(c_name)) + val_exprs.append(c_value) + res = f.less(f.tuple(*col_exprs), f.tuple(*val_exprs)) return res return None @@ -177,22 +211,34 @@ def create( # find the attribute in the in_msg.columns attribute that has the same label as the `column` attribute in the order_by_clause # call `attribute_key_to_expression` on it and us its alias as the name of the AttributeKey in the ComparisonFilter attribute_expression = None + selected_key = None for selected_column in in_msg.columns: if selected_column.label == order_by_clause.column.label: attribute_expression = attribute_key_to_expression( selected_column.key ) + selected_key = selected_column.key break if attribute_expression is None: raise ValueError( f"No attribute expression found for column: {order_by_clause.column.label}" ) + # Mark SORT_SEMVER string columns so get_filters wraps + # both sides in the semver key (matching ORDER BY); mirrors + # the resolver's string-only guard. + is_semver = ( + order_by_clause.sort == TraceItemTableRequest.OrderBy.SORT_SEMVER + and selected_key is not None + and selected_key.type == AttributeKey.TYPE_STRING + ) + prefix = cls._SEMVER_FILTER_PREFIX if is_semver else cls._FILTER_PREFIX + filters.append( TraceItemFilter( comparison_filter=ComparisonFilter( key=AttributeKey( - name=f"{cls._FILTER_PREFIX}.{attribute_expression.alias}", + name=f"{prefix}.{attribute_expression.alias}", ), op=ComparisonFilter.OP_LESS_THAN, value=last_result_value, diff --git a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py index dc737bd35d..00279b3ee2 100644 --- a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py +++ b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py @@ -1,4 +1,7 @@ +import re import uuid +from collections.abc import Mapping +from typing import Any from google.protobuf.json_format import MessageToDict from sentry_protos.snuba.v1.endpoint_trace_item_attributes_pb2 import ( @@ -33,6 +36,7 @@ next_monday, prev_monday, project_id_and_org_conditions, + semver_sort_key, treeify_or_and_conditions, ) from snuba.web.rpc.common.debug_info import extract_response_meta @@ -67,18 +71,61 @@ def _order_by_count(request: TraceItemAttributeNamesRequest) -> bool: return request.order_by.column == TraceItemAttributeNamesRequest.OrderBy.Column.COLUMN_COUNT +def _order_by_semver(request: TraceItemAttributeNamesRequest) -> bool: + """Whether the caller requested SORT_SEMVER (semver) ordering of names.""" + return request.order_by.sort == TraceItemAttributeNamesRequest.OrderBy.SORT_SEMVER + + def _order_by_name_descending(request: TraceItemAttributeNamesRequest) -> bool: - """Whether the caller explicitly requested name ordering in descending order. + """Whether the caller requested name ordering in descending order. - Only ``COLUMN_NAME`` + ``descending`` flips the default; unset ordering stays - name-ascending for backwards compatibility. + Both an explicit ``COLUMN_NAME`` and ``SORT_SEMVER`` (which orders by the + semver key of the name, typically with ``column`` left unset) select name + ordering, so ``descending`` flips either. Unset ordering stays name-ascending + for backwards compatibility. """ - return ( + return request.order_by.descending and ( request.order_by.column == TraceItemAttributeNamesRequest.OrderBy.Column.COLUMN_NAME - and request.order_by.descending + or _order_by_semver(request) + ) + + +_SEMVER_NUMERIC_RE = re.compile(r"^[0-9]+(\.[0-9]+)*$") + + +def _semver_sort_key_py(name: str) -> tuple[tuple[int, int, int, int], int, str]: + """Python mirror of common.semver_sort_key, used to re-sort names in Python so + the merged (ClickHouse + synthetic) result matches the ClickHouse ORDER BY.""" + non_null = name or "" + version_no_prefix = non_null.split("@")[-1] + # Drop SemVer build metadata ("1.2.3+build") before parsing; it must not + # affect precedence (mirrors common.semver_sort_key). + version_no_build = version_no_prefix.split("+")[0] + release_part = version_no_build.split("-")[0] + # Mirror ClickHouse toUInt32OrZero: only ASCII decimal parses, anything else + # (including Unicode digits like "²", where str.isdigit() is True but int() + # raises) maps to 0. + components = [int(c) if (c.isascii() and c.isdigit()) else 0 for c in release_part.split(".")] + components = (components + [0, 0, 0, 0])[:4] + is_stable = 1 if _SEMVER_NUMERIC_RE.match(version_no_build) else 0 + return ( + (components[0], components[1], components[2], components[3]), + is_stable, + non_null, ) +def _name_order_by_expression(semver: bool) -> Expression: + """ClickHouse ORDER BY expression for the attribute name. + + Default orders by the raw (type, name) tuple; SORT_SEMVER orders by the + semver key of the name part so versions sort numerically. + """ + if semver: + return semver_sort_key(f.tupleElement(column("attr_key"), 2)) + return column("attr_key") + + class AttributeKeyCollector(ProtoVisitor): def __init__(self) -> None: self.keys: set[str] = set() @@ -344,6 +391,7 @@ def get_co_occurring_attributes( alias="attr_key", ) + semver = _order_by_semver(request) if _order_by_count(request): # Opt-in frequency ordering: group by key and count how many rows # (co-occurring attribute sets) contain each key. @@ -359,8 +407,9 @@ def get_co_occurring_attributes( ), expression=column("count"), ), - # stable tiebreak for keys with the same frequency - OrderBy(direction=OrderByDirection.ASC, expression=column("attr_key")), + # stable tiebreak for keys with the same frequency (semver key when + # SORT_SEMVER was requested) + OrderBy(direction=OrderByDirection.ASC, expression=_name_order_by_expression(semver)), ] else: # Default (order_by unset or COLUMN_NAME): distinct keys ordered by name. @@ -374,7 +423,7 @@ def get_co_occurring_attributes( order_by = [ OrderBy( direction=OrderByDirection.DESC if name_descending else OrderByDirection.ASC, - expression=column("attr_key"), + expression=_name_order_by_expression(semver), ), ] @@ -428,6 +477,17 @@ def t(row: Row) -> TraceItemAttributeNamesResponse.Attribute: attribute.count = int(count) return attribute + # Name-ordering key that mirrors the ClickHouse ORDER BY: the raw (type, name) + # tuple by default, or the semver key of the name under SORT_SEMVER. + semver = _order_by_semver(request) + + def _name_key(row: Mapping[str, Any]) -> Any: + attr_key = row.get("attr_key", ("TYPE_STRING", "")) + attr_type, attr_name = attr_key[0], attr_key[1] + if semver: + return _semver_sort_key_py(attr_name) + return (attr_type, attr_name) + data = query_res.result.get("data", []) if request.type in (AttributeKey.TYPE_UNSPECIFIED, AttributeKey.TYPE_STRING): non_stored = [ @@ -435,22 +495,20 @@ def t(row: Row) -> TraceItemAttributeNamesResponse.Attribute: for key_name in NON_STORED_ATTRIBUTE_KEYS if request.value_substring_match in key_name ] - non_stored.sort(key=lambda row: tuple(row["attr_key"])) + non_stored.sort(key=_name_key) if _order_by_count(request): - # Order the real (counted) rows to match ClickHouse: count in the - # requested direction, then name ASC (two stable passes). The synthetic - # non-stored attributes have no real count, so pin them first regardless - # of sort direction rather than relying on a sentinel value. - data.sort(key=lambda row: tuple(row.get("attr_key", ("TYPE_STRING", "")))) + # Match ClickHouse: count in the requested direction, then name ASC + # (two stable passes). Synthetic non-stored keys have no count, so + # pin them first rather than relying on a sentinel. + data.sort(key=_name_key) data.sort(key=lambda row: row.get("count", 0), reverse=request.order_by.descending) data = non_stored + data else: - # Default name ordering: merge the synthetic non-stored keys in and re-sort - # by name, honoring the requested direction so it matches the ClickHouse - # ORDER BY (a COLUMN_NAME descending request must stay descending here too). + # Merge synthetic non-stored keys in and re-sort by name in the + # requested direction, matching the ClickHouse ORDER BY. data.extend(non_stored) data.sort( - key=lambda row: tuple(row.get("attr_key", ("TYPE_STRING", ""))), + key=_name_key, reverse=_order_by_name_descending(request), ) diff --git a/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py b/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py index 15139f9b9c..238597b7d1 100644 --- a/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py +++ b/snuba/web/rpc/v1/resolvers/R_eap_items/resolver_trace_item_table.py @@ -62,6 +62,7 @@ attribute_key_to_expression, base_conditions_and, get_field_existence_expression, + semver_sort_key, timestamp_in_range_condition, trace_item_filters_to_expression, treeify_or_and_conditions, @@ -283,6 +284,10 @@ def _groupby_order_by_expression(attr_key: AttributeKey) -> Expression: while keeping the cast valid as a function of the grouped column. TYPE_ARRAY keys are rejected upstream (see _validate_select_and_groupby / _validate_order_by), so they never reach here. + + The SORT_SEMVER (semver) ordering is applied by `_convert_order_by`, not here, so + GROUP BY keeps the raw expression while ORDER BY can wrap it in the semver key. + ClickHouse accepts ORDER BY on a function of a GROUP BY key, so the two can differ. """ if attr_key.name == "sentry.timestamp": return snuba_column("timestamp") @@ -338,10 +343,18 @@ def _convert_order_by( # covers `sentry.timestamp` ordering anywhere else.) GROUP BY uses the same # expression so an aggregation query that orders by `sentry.timestamp` stays # valid. + expression = _groupby_order_by_expression(x.column.key) + # SORT_SEMVER: client-driven semver ordering, string columns only + # (numeric/timestamp columns already sort numerically). + if ( + x.sort == TraceItemTableRequest.OrderBy.SORT_SEMVER + and x.column.key.type == AttributeKey.TYPE_STRING + ): + expression = semver_sort_key(expression) res.append( OrderBy( direction=direction, - expression=_groupby_order_by_expression(x.column.key), + expression=expression, ) ) elif x.column.HasField("conditional_aggregation"): diff --git a/snuba/web/rpc/v1/trace_item_attribute_values.py b/snuba/web/rpc/v1/trace_item_attribute_values.py index c57f166625..69d46c26f9 100644 --- a/snuba/web/rpc/v1/trace_item_attribute_values.py +++ b/snuba/web/rpc/v1/trace_item_attribute_values.py @@ -30,6 +30,7 @@ add_existence_check_to_map_attribute_reads, attribute_key_to_expression, base_conditions_and, + semver_sort_key, treeify_or_and_conditions, ) from snuba.web.rpc.common.exceptions import BadSnubaRPCRequestException @@ -136,6 +137,18 @@ def _build_query( ) treeify_or_and_conditions(inner_query) add_existence_check_to_map_attribute_reads(inner_query) + # Under SORT_SEMVER, tiebreak equally-frequent values by the semver key + # instead of lexicographically (count() stays the primary ordering). Only + # string values: semver_sort_key uses string functions, so boolean keys + # (also enumerable here) keep plain ordering, like the table resolver's guard. + if ( + request.order_by.sort == TraceItemAttributeValuesRequest.OrderBy.SORT_SEMVER + and request.key.type == AttributeKey.TYPE_STRING + ): + value_order_expression: Expression = semver_sort_key(column("attr_value")) + else: + value_order_expression = column("attr_value") + res = CompositeQuery( from_clause=inner_query, selected_columns=[ @@ -154,7 +167,7 @@ def _build_query( ], order_by=[ OrderBy(direction=OrderByDirection.DESC, expression=column("count()")), - OrderBy(direction=OrderByDirection.ASC, expression=column("attr_value")), + OrderBy(direction=OrderByDirection.ASC, expression=value_order_expression), ], groupby=[column("attr_value")], limit=request.limit, diff --git a/tests/web/rpc/test_common.py b/tests/web/rpc/test_common.py index f5399f15a8..878003bdc1 100644 --- a/tests/web/rpc/test_common.py +++ b/tests/web/rpc/test_common.py @@ -11,6 +11,7 @@ ) from sentry_protos.snuba.v1.error_pb2 import Error as ErrorProto from sentry_protos.snuba.v1.request_common_pb2 import ( + PageToken, RequestMeta, TraceItemType, ) @@ -59,6 +60,7 @@ dedupe_and_conditions, next_monday, prev_monday, + semver_sort_key, trace_item_filters_to_expression, treeify_or_and_conditions, use_sampling_factor, @@ -68,6 +70,7 @@ RPCAllocationPolicyException, convert_rpc_exception_to_proto, ) +from snuba.web.rpc.common.pagination import FlexibleTimeWindowPageWithFilters from snuba.web.rpc.v1.endpoint_trace_item_table import EndpointTraceItemTable from tests.conftest import SnubaSetConfig from tests.helpers import write_raw_unprocessed_events @@ -1458,6 +1461,70 @@ def test_not_like_wildcard_matches_only_absent(self) -> None: assert self._execute(ComparisonFilter.OP_NOT_LIKE, value="%") == ["green"] +class TestSemverSortKey: + def test_expression_structure(self) -> None: + expr = semver_sort_key(column("release")) + assert isinstance(expr, FunctionCall) + assert expr.function_name == "tuple" + assert len(expr.parameters) == 3 + numeric_key_expr, is_stable_expr, raw_str_expr = expr.parameters + assert isinstance(numeric_key_expr, FunctionCall) + assert numeric_key_expr.function_name == "arrayResize" + # Stability flag: match(version, '^[0-9]+(\\.[0-9]+)*$') — 1 for a plain + # dotted-numeric (stable) version, 0 for any prerelease form. + assert isinstance(is_stable_expr, FunctionCall) + assert is_stable_expr.function_name == "match" + # Third element is the raw (non-null) string tiebreaker: ifNull(expr, ''). + assert isinstance(raw_str_expr, FunctionCall) + assert raw_str_expr.function_name == "ifNull" + + def test_alias_is_forwarded(self) -> None: + expr = semver_sort_key(column("release"), alias="semver_key") + assert isinstance(expr, FunctionCall) + assert expr.alias == "semver_key" + + def test_no_alias_by_default(self) -> None: + expr = semver_sort_key(column("release")) + assert expr.alias is None + + +class TestFlexibleTimeWindowPageFilters: + def _timestamp_page_token(self, value: AttributeValue) -> PageToken: + prefix = FlexibleTimeWindowPageWithFilters._FILTER_PREFIX + return PageToken( + filter_offset=TraceItemFilter( + and_filter=AndFilter( + filters=[ + TraceItemFilter( + comparison_filter=ComparisonFilter( + key=AttributeKey(name=f"{prefix}.timestamp"), + op=ComparisonFilter.OP_LESS_THAN, + value=value, + ) + ) + ] + ) + ) + ) + + def test_get_filters_rejects_unsupported_timestamp_type(self) -> None: + # A client-supplied page token whose timestamp filter carries an + # unsupported value type must raise a clear error rather than desync the + # parallel column lists and crash the strict zip() with an opaque error. + pager = FlexibleTimeWindowPageWithFilters( + self._timestamp_page_token(AttributeValue(val_bool=True)) + ) + with pytest.raises(ValueError, match="not supported"): + pager.get_filters() + + def test_get_filters_accepts_int_timestamp(self) -> None: + pager = FlexibleTimeWindowPageWithFilters( + self._timestamp_page_token(AttributeValue(val_int=1741910400)) + ) + # A supported value type builds the boundary filter without raising. + assert pager.get_filters() is not None + + class TestAnyAttributeFilterOption: """The `enable_any_attribute_filter` sentry-option gates whether any_attribute_filter is translated into a predicate or treated as diff --git a/tests/web/rpc/v1/test_endpoint_trace_item_attribute_names.py b/tests/web/rpc/v1/test_endpoint_trace_item_attribute_names.py index d29345f6a2..35c8043fa3 100644 --- a/tests/web/rpc/v1/test_endpoint_trace_item_attribute_names.py +++ b/tests/web/rpc/v1/test_endpoint_trace_item_attribute_names.py @@ -99,6 +99,73 @@ def test_basic(self) -> None: ) assert res.attributes == expected + def test_semver_sort(self) -> None: + # Version-like attribute names: lexicographically "1.2.10" < "1.2.2" < + # "1.2.9"; with SORT_SEMVER (semver) they order "1.2.2" < "1.2.9" < "1.2.10". + items_storage = get_writable_storage(StorageKey("eap_items")) + write_raw_unprocessed_events( + items_storage, + [ + gen_item_message( + start_timestamp=BASE_TIME, + attributes={name: AnyValue(string_value="x")}, + ) + for name in ("1.2.9", "1.2.10", "1.2.2") + ], + ) + req = TraceItemAttributeNamesRequest( + meta=RequestMeta( + project_ids=[1, 2, 3], + organization_id=1, + cogs_category="something", + referrer="something", + start_timestamp=Timestamp(seconds=int((BASE_TIME - timedelta(days=1)).timestamp())), + end_timestamp=Timestamp(seconds=int((BASE_TIME + timedelta(days=1)).timestamp())), + ), + limit=100, + type=AttributeKey.Type.TYPE_STRING, + value_substring_match="1.2", + order_by=TraceItemAttributeNamesRequest.OrderBy( + sort=TraceItemAttributeNamesRequest.OrderBy.SORT_SEMVER, + ), + ) + res = EndpointTraceItemAttributeNames().execute(req) + assert [a.name for a in res.attributes] == ["1.2.2", "1.2.9", "1.2.10"] + + def test_semver_sort_descending(self) -> None: + # descending applies to SORT_SEMVER name ordering even with column unset, + # so the result is the exact reverse of the ascending semver order. + items_storage = get_writable_storage(StorageKey("eap_items")) + write_raw_unprocessed_events( + items_storage, + [ + gen_item_message( + start_timestamp=BASE_TIME, + attributes={name: AnyValue(string_value="x")}, + ) + for name in ("1.2.9", "1.2.10", "1.2.2") + ], + ) + req = TraceItemAttributeNamesRequest( + meta=RequestMeta( + project_ids=[1, 2, 3], + organization_id=1, + cogs_category="something", + referrer="something", + start_timestamp=Timestamp(seconds=int((BASE_TIME - timedelta(days=1)).timestamp())), + end_timestamp=Timestamp(seconds=int((BASE_TIME + timedelta(days=1)).timestamp())), + ), + limit=100, + type=AttributeKey.Type.TYPE_STRING, + value_substring_match="1.2", + order_by=TraceItemAttributeNamesRequest.OrderBy( + sort=TraceItemAttributeNamesRequest.OrderBy.SORT_SEMVER, + descending=True, + ), + ) + res = EndpointTraceItemAttributeNames().execute(req) + assert [a.name for a in res.attributes] == ["1.2.10", "1.2.9", "1.2.2"] + def test_simple_float_backward_compat(self) -> None: req = TraceItemAttributeNamesRequest( meta=RequestMeta( diff --git a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py index a6d29223b6..93225fa21c 100644 --- a/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py +++ b/tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py @@ -4497,6 +4497,150 @@ def test_order_by_bug() -> None: _validate_order_by(message) +@pytest.mark.clickhouse_db +@pytest.mark.redis_db +class TestSemverSorting: + """ORDER BY with the SORT_SEMVER option applies the semver key so versions + sort numerically (1.2.9 before 1.2.10) with pre-releases before their + corresponding stable release. Without SORT_SEMVER the ordering stays + lexicographic (there is no hardcoded per-attribute behavior). + """ + + _RELEASES = [ + "1.2.9", + "1.2.10", + "1.2.3", + "1.2.3-beta.1", + "1.2.2", + "my-pkg@2.0.0", + "1.2", + "1.2.0", + # PEP 440 dot-style dev build (Sentry's own sentry.release format) and + # its GA release, to verify the dev build sorts before GA. + "24.7.0.dev0+abc123", + "24.7.0", + # SemVer build metadata must not affect precedence: "1.2.3+build456" + # sorts as the stable 1.2.3, not as a prerelease of 1.2.0. + "1.2.3+build456", + ] + + @pytest.fixture(autouse=True) + def _setup(self, clickhouse_db: None, redis_db: None) -> None: + for i, release in enumerate(self._RELEASES): + write_eap_item( + start_timestamp=BASE_TIME + timedelta(minutes=i), + raw_attributes={"sentry.release": release, "semver_test_marker": "1"}, + ) + + def _query_releases(self, descending: bool = False, semver: bool = True) -> list[str]: + sort = ( + TraceItemTableRequest.OrderBy.SORT_SEMVER + if semver + else TraceItemTableRequest.OrderBy.SORT_UNSPECIFIED + ) + message = TraceItemTableRequest( + meta=RequestMeta( + project_ids=[1], + organization_id=1, + cogs_category="something", + referrer="something", + start_timestamp=START_TIMESTAMP, + end_timestamp=END_TIMESTAMP, + trace_item_type=TraceItemType.TRACE_ITEM_TYPE_SPAN, + ), + filter=TraceItemFilter( + exists_filter=ExistsFilter( + key=AttributeKey(type=AttributeKey.TYPE_STRING, name="semver_test_marker") + ) + ), + columns=[ + Column(key=AttributeKey(type=AttributeKey.TYPE_STRING, name="sentry.release")) + ], + order_by=[ + TraceItemTableRequest.OrderBy( + column=Column( + key=AttributeKey(type=AttributeKey.TYPE_STRING, name="sentry.release") + ), + descending=descending, + sort=sort, + ) + ], + limit=len(self._RELEASES) + 10, + ) + response = EndpointTraceItemTable().execute(message) + return [v.val_str for v in response.column_values[0].results] + + def test_numeric_ordering(self) -> None: + releases = self._query_releases() + assert releases.index("1.2.9") < releases.index("1.2.10"), ( + "1.2.9 must sort before 1.2.10 (numeric, not lexicographic)" + ) + + def test_default_sort_is_lexicographic(self) -> None: + # Without SORT_SEMVER there is no semver behavior (no hardcoded + # attributes), so plain lexicographic order applies: "1.2.10" < "1.2.9". + releases = self._query_releases(semver=False) + assert releases.index("1.2.10") < releases.index("1.2.9"), ( + "without SORT_SEMVER, ordering is lexicographic" + ) + + def test_prerelease_before_stable(self) -> None: + releases = self._query_releases() + assert releases.index("1.2.3-beta.1") < releases.index("1.2.3"), ( + "prerelease 1.2.3-beta.1 must sort before stable 1.2.3" + ) + + def test_dot_dev_prerelease_before_stable(self) -> None: + releases = self._query_releases() + assert releases.index("24.7.0.dev0+abc123") < releases.index("24.7.0"), ( + "PEP 440 dot-style dev build must sort before its GA release" + ) + + def test_prerelease_of_newer_after_older_stable(self) -> None: + releases = self._query_releases() + assert releases.index("1.2.3-beta.1") > releases.index("1.2.2"), ( + "prerelease 1.2.3-beta.1 must sort after older stable 1.2.2" + ) + + def test_package_prefix_stripped(self) -> None: + releases = self._query_releases() + assert releases.index("my-pkg@2.0.0") > releases.index("1.2.10"), ( + "my-pkg@2.0.0 should sort as version 2.0.0 (after 1.x)" + ) + + def test_build_metadata_ignored(self) -> None: + # Build metadata does not affect precedence: "1.2.3+build456" must sort + # as stable 1.2.3 (after 1.2.2 and after the 1.2.3-beta.1 prerelease), + # not as a prerelease of 1.2.0 (which would land before 1.2.2). + releases = self._query_releases() + assert releases.index("1.2.3+build456") > releases.index("1.2.2"), ( + "1.2.3+build456 must sort as 1.2.3 (stable), after 1.2.2" + ) + assert releases.index("1.2.3+build456") > releases.index("1.2.3-beta.1"), ( + "build metadata is not a prerelease: 1.2.3+build456 sorts after 1.2.3-beta.1" + ) + + def test_length_normalisation(self) -> None: + releases = self._query_releases() + idx_12 = releases.index("1.2") + idx_120 = releases.index("1.2.0") + # Both normalise to [1,2,0,0], so they are adjacent. The raw-string + # tiebreaker then breaks the tie deterministically: "1.2" < "1.2.0". + assert idx_12 < idx_120, ( + "1.2 and 1.2.0 normalise equally and must be adjacent, with '1.2' " + "first via the raw-string tiebreaker" + ) + + def test_desc_is_reverse_of_asc(self) -> None: + # The raw-string tiebreaker in semver_sort_key gives distinct release + # strings a deterministic total order (e.g. "1.2" < "1.2.0"), so DESC is + # the exact reverse of ASC even for versions that share the same numeric + # key. + asc = self._query_releases(descending=False) + desc = self._query_releases(descending=True) + assert asc == list(reversed(desc)) + + def test_convert_results_empty_typed_array_is_null() -> None: """An absent/empty element-typed array reads as an empty native list; convert_results surfaces it as NULL (like a missing scalar), not an empty val_array. A populated array 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 3467e698bd..7600f8d802 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 @@ -148,6 +148,88 @@ def test_simple_case(self, setup_teardown: Any) -> None: assert response.values == ["derpderp", "blah", "durp", "herp", "herpderp"] assert response.counts == [2, 1, 1, 1, 1] + def _write_version_values(self) -> None: + # Distinct version-like values (one occurrence each). Lexicographically + # "1.2.10" < "1.2.2" < "1.2.9"; naturally "1.2.2" < "1.2.9" < "1.2.10". + # Written per-test (not in the shared fixture) so the extra items don't + # perturb the count-based assertions in other tests. + items_storage = get_writable_storage(StorageKey("eap_items")) + write_raw_unprocessed_events( + items_storage, + [ + gen_item_message( + start_timestamp=BASE_TIME, + attributes={"natural_ver": AnyValue(string_value=v)}, + ) + for v in ("1.2.9", "1.2.10", "1.2.2") + ], + ) + + def test_semver_sort(self, setup_teardown: Any) -> None: + # SORT_SEMVER applies the semver key, so version components sort + # numerically (1.2.2 < 1.2.9 < 1.2.10) regardless of the attribute. + self._write_version_values() + message = TraceItemAttributeValuesRequest( + meta=COMMON_META, + limit=10, + key=AttributeKey(name="natural_ver", type=AttributeKey.TYPE_STRING), + order_by=TraceItemAttributeValuesRequest.OrderBy( + column=TraceItemAttributeValuesRequest.OrderBy.COLUMN_VALUE, + sort=TraceItemAttributeValuesRequest.OrderBy.SORT_SEMVER, + ), + ) + response = AttributeValuesRequest().execute(message) + assert response.values == ["1.2.2", "1.2.9", "1.2.10"] + + def test_default_sort_is_lexicographic(self, setup_teardown: Any) -> None: + # Unset sort keeps the historical lexicographic ordering, where "1.2.10" + # sorts before "1.2.2" and "1.2.9". + self._write_version_values() + message = TraceItemAttributeValuesRequest( + meta=COMMON_META, + limit=10, + key=AttributeKey(name="natural_ver", type=AttributeKey.TYPE_STRING), + ) + response = AttributeValuesRequest().execute(message) + assert response.values == ["1.2.10", "1.2.2", "1.2.9"] + + def test_release_sort_is_semver_aware(self, setup_teardown: Any) -> None: + # SORT_SEMVER is the semver sort, so prerelease sorts before its stable + # release and the "pkg@" prefix is stripped. + items_storage = get_writable_storage(StorageKey("eap_items")) + releases = ["1.2.3-beta.1", "1.2.3", "1.2.9", "1.2.10", "my-pkg@2.0.0"] + write_raw_unprocessed_events( + items_storage, + [ + gen_item_message( + start_timestamp=BASE_TIME, + attributes={"sentry.release": AnyValue(string_value=r)}, + ) + for r in releases + ], + ) + message = TraceItemAttributeValuesRequest( + meta=COMMON_META, + limit=10, + key=AttributeKey(name="sentry.release", type=AttributeKey.TYPE_STRING), + order_by=TraceItemAttributeValuesRequest.OrderBy( + column=TraceItemAttributeValuesRequest.OrderBy.COLUMN_VALUE, + sort=TraceItemAttributeValuesRequest.OrderBy.SORT_SEMVER, + ), + ) + response = AttributeValuesRequest().execute(message) + # gen_item_message stamps a default sentry.release on every fixture item, + # so other release values may appear; assert only the relative order of + # the releases we wrote, which must follow semver ordering. + ordered = [v for v in response.values if v in set(releases)] + assert ordered == [ + "1.2.3-beta.1", + "1.2.3", + "1.2.9", + "1.2.10", + "my-pkg@2.0.0", + ] + def test_with_value_substring_match(self, setup_teardown: Any) -> None: message = TraceItemAttributeValuesRequest( meta=COMMON_META, @@ -265,6 +347,22 @@ def test_boolean_case(self, setup_teardown: Any) -> None: assert response.values == ["true", "false"] assert response.counts == [8, 1] + def test_boolean_case_with_semver_sort_does_not_crash(self, setup_teardown: Any) -> None: + # SORT_SEMVER only applies the semver key to string values; a boolean key + # falls back to plain ordering instead of feeding a bool column into the + # string-only semver functions (which would fail at ClickHouse). + message = TraceItemAttributeValuesRequest( + meta=COMMON_META, + limit=5, + key=AttributeKey(name="sentry.is_segment", type=AttributeKey.TYPE_BOOLEAN), + order_by=TraceItemAttributeValuesRequest.OrderBy( + sort=TraceItemAttributeValuesRequest.OrderBy.SORT_SEMVER, + ), + ) + response = AttributeValuesRequest().execute(message) + assert response.values == ["true", "false"] + assert response.counts == [8, 1] + def test_boolean_existence_check(self, setup_teardown: Any) -> None: # `custom_flag` is set on exactly two items (one True, one False); the # other items do not have the key. The existence check must exclude the diff --git a/uv.lock b/uv.lock index 159f5e1e31..3ced8f7ee8 100644 --- a/uv.lock +++ b/uv.lock @@ -1023,7 +1023,7 @@ wheels = [ [[package]] name = "sentry-protos" -version = "0.46.0" +version = "0.47.0" source = { registry = "https://pypi.devinfra.sentry.io/simple" } dependencies = [ { name = "grpc-stubs", marker = "sys_platform == 'darwin' or sys_platform == 'linux'" }, @@ -1031,7 +1031,7 @@ dependencies = [ { name = "protobuf", marker = "sys_platform == 'darwin' or sys_platform == 'linux'" }, ] wheels = [ - { url = "https://pypi.devinfra.sentry.io/wheels/sentry_protos-0.46.0-py3-none-any.whl", hash = "sha256:87c3f50ee223a1f3901eb0e084c54f50f4de772dedd314e20763994480258575" }, + { url = "https://pypi.devinfra.sentry.io/wheels/sentry_protos-0.47.0-py3-none-any.whl", hash = "sha256:4ec26ce7da25266f488987a42313c5c8ceff9252c7ffc5f59a84c4fd15941b99" }, ] [[package]] @@ -1234,7 +1234,7 @@ requires-dist = [ { name = "sentry-conventions", specifier = ">=0.17.0" }, { name = "sentry-kafka-schemas", specifier = ">=2.1.39" }, { name = "sentry-options", specifier = ">=1.2.1" }, - { name = "sentry-protos", specifier = ">=0.46.0" }, + { name = "sentry-protos", specifier = ">=0.47.0" }, { name = "sentry-redis-tools", specifier = ">=0.5.1" }, { name = "sentry-relay", specifier = ">=0.9.25" }, { name = "sentry-sdk", specifier = ">=2.35.0" },