diff --git a/sentry-options/schemas/snuba/schema.json b/sentry-options/schemas/snuba/schema.json index 1a46b64c20c..8f977ba752a 100644 --- a/sentry-options/schemas/snuba/schema.json +++ b/sentry-options/schemas/snuba/schema.json @@ -688,16 +688,6 @@ "default": "jittered", "description": "TaskBuilderMode for the subscription scheduler: one of immediate, jittered, transition_jitter, transition_immediate." }, - "use_co_occurring_attrs_v2": { - "type": "boolean", - "default": false, - "description": "When true, the TraceItemAttributeNames RPC may read the v2 co-occurring-attributes storage (eap_item_co_occurring_attrs_v2) instead of v1. v2 is a SummingMergeTree carrying an occurrence count per attribute set (so count ordering sums that column rather than counting rows), a last_seen timestamp (which v1 lacks entirely, so ordering by COLUMN_LAST_SEEN is only honoured on v2 and degrades to COLUMN_COUNT otherwise), and one attribute-key array per type, which lets int and array-typed keys be returned with their real AttributeKey type. Enabling this is safe at any time: a request whose time range reaches back before co_occurring_attrs_v2_start_timestamp still reads v1, since v2 has no data there." - }, - "co_occurring_attrs_v2_start_timestamp": { - "type": "integer", - "default": 1785110400, - "description": "Unix timestamp of the earliest weekly date bucket the v2 co-occurring-attributes tables hold data for; TraceItemAttributeNames only reads v2 for requests whose range starts at or after it, and reads v1 otherwise. Defaults to 2026-07-27 00:00 UTC, the Monday of the week the v2 tables and their materialized view were created. Must be a Monday: the date column is bucketed weekly with toMonday() and the query rounds its lower bound down to the previous Monday, so a mid-week value would let a request read a bucket v2 never populated. Lower it only to a bucket v2 has been backfilled to." - }, "eap_items_use_indexed_name_organization_ids": { "type": "array", "items": { 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 166940621ff..55b8ff5d44f 100644 --- a/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py +++ b/snuba/web/rpc/v1/endpoint_trace_item_attribute_names.py @@ -39,8 +39,10 @@ ) from snuba.web.rpc.common.debug_info import extract_response_meta from snuba.web.rpc.proto_visitor import ProtoVisitor, TraceItemFilterWrapper -from snuba.web.rpc.v1.resolvers.R_eap_items import co_occurring_attrs -from snuba.web.rpc.v1.resolvers.R_eap_items.co_occurring_attrs import CoOccurringAttrsSource +from snuba.web.rpc.v1.resolvers.R_eap_items.co_occurring_attrs import ( + V2, + CoOccurringAttrsSource, +) # max value the user can provide for 'limit' in their request MAX_REQUEST_LIMIT = 1000 @@ -71,29 +73,6 @@ def _order_by_count(request: TraceItemAttributeNamesRequest) -> bool: return request.order_by.column == TraceItemAttributeNamesRequest.OrderBy.Column.COLUMN_COUNT -def _effective_order_by_column( - request: TraceItemAttributeNamesRequest, - source: CoOccurringAttrsSource, -) -> TraceItemAttributeNamesRequest.OrderBy.Column.ValueType: - """The ordering the query will apply, which may differ from the one requested. - - Only v2 records ``last_seen``, so a recency request that lands on v1 degrades to frequency - ordering rather than failing: both rank "attributes worth showing first", so an - autocomplete caller still gets a useful answer. It stays detectable because ``last_seen`` - is then absent from the response, and via a metric. - - Everything downstream keys off this rather than ``request.order_by.column``, so the - ClickHouse ORDER BY and the Python re-sort cannot disagree about which ordering was used. - """ - column = request.order_by.column - if ( - column == TraceItemAttributeNamesRequest.OrderBy.Column.COLUMN_LAST_SEEN - and not source.has_last_seen - ): - return TraceItemAttributeNamesRequest.OrderBy.Column.COLUMN_COUNT - return column - - def _aggregates_attributes( order_by_column: TraceItemAttributeNamesRequest.OrderBy.Column.ValueType, ) -> bool: @@ -254,7 +233,6 @@ def _add_substring_match_optimization( def get_co_occurring_attributes( request: TraceItemAttributeNamesRequest, - source: CoOccurringAttrsSource | None = None, ) -> SnubaRequest: """Constructs the clickhouse query for co-occurring attributes: @@ -263,7 +241,7 @@ def get_co_occurring_attributes( -- Default ordering (order_by unset or COLUMN_NAME): distinct keys by name SELECT distinct(arrayJoin(arrayFilter(attr -> ((NOT has(['test_tag_1_0'], attr.2)) AND startsWith(attr.2, 'test_')), arrayMap(x -> ('TYPE_STRING', x), attributes_string)))) AS attr_key - FROM eap_item_co_occurring_attrs_1_local + FROM eap_item_co_occurring_attrs_2_local WHERE (item_type = 1) AND (project_id IN [1]) AND (organization_id = 1) AND (date < toDateTime(toDate('2025-03-17', 'Universal'))) AND (date >= toDateTime(toDate('2025-03-10', 'Universal'))) -- This is a faster way of looking up whether all attributes co-exist, it uses an array of hashes. This avoids string equality comparisons @@ -279,9 +257,8 @@ def get_co_occurring_attributes( -- max(last_seen) AS last_seen -- ... GROUP BY attr_key ORDER BY DESC, attr_key ASC - **Storage:** the roll-up this reads and the parts of the query shape that differ between - the two (per-type key arrays, the aggregates) come from the `CoOccurringAttrsSource` - returned by `resolvers.R_eap_items.co_occurring_attrs.for_request`. + **Storage:** always ``eap_item_co_occurring_attrs_v2``. Per-type key arrays and + aggregates come from ``CoOccurringAttrsV2``. **Explanation:** @@ -318,12 +295,8 @@ def get_co_occurring_attributes( - The attribute keys are deduplicated, resulting in less data to scan (~95% row reduction rate) - there is a bloom filter index on all key values """ - # Callers that need the source themselves must pass the one they resolved: resolving reads - # runtime options, and an option flipping between two reads would build the query for one - # storage while the response is processed as if it were the other. - if source is None: - source = co_occurring_attrs.for_request(request) - order_by_column = _effective_order_by_column(request, source) + source = V2 + order_by_column = request.order_by.column # get all attribute keys from the filter collector = AttributeKeyCollector() @@ -497,9 +470,8 @@ def convert_co_occurring_results_to_attributes( ) -> list[TraceItemAttributeNamesResponse.Attribute]: """Build the response attributes, re-sorting to match the ClickHouse ORDER BY. - ``order_by_column`` must be the value the query was built with (see - ``_effective_order_by_column``), or the merge below re-sorts into a different order than - ClickHouse used. Defaults to the requested column. + ``order_by_column`` must match the query's ORDER BY, or the merge re-sorts into a + different order than ClickHouse used. Defaults to the requested column. """ if order_by_column is None: order_by_column = request.order_by.column @@ -607,24 +579,8 @@ def _build_response( ) def _execute(self, in_msg: TraceItemAttributeNamesRequest) -> TraceItemAttributeNamesResponse: - # Resolved once and shared, so the query and the response re-sort cannot disagree. - source = co_occurring_attrs.for_request(in_msg) - order_by_column = _effective_order_by_column(in_msg, source) - if order_by_column != in_msg.order_by.column: - # Keeps an otherwise silent downgrade visible during the v2 rollout. - self.metrics.increment( - "attribute_names_order_by_degraded", - 1, - tags={ - "requested": TraceItemAttributeNamesRequest.OrderBy.Column.Name( - in_msg.order_by.column - ), - "applied": TraceItemAttributeNamesRequest.OrderBy.Column.Name(order_by_column), - "storage": source.storage_key.value, - }, - ) - - snuba_request = get_co_occurring_attributes(in_msg, source) + order_by_column = in_msg.order_by.column + snuba_request = get_co_occurring_attributes(in_msg) res = run_query( dataset=PluggableDataset(name="eap", all_entities=[]), request=snuba_request, diff --git a/snuba/web/rpc/v1/resolvers/R_eap_items/co_occurring_attrs/__init__.py b/snuba/web/rpc/v1/resolvers/R_eap_items/co_occurring_attrs/__init__.py index 9664993ac3a..2f6667daeac 100644 --- a/snuba/web/rpc/v1/resolvers/R_eap_items/co_occurring_attrs/__init__.py +++ b/snuba/web/rpc/v1/resolvers/R_eap_items/co_occurring_attrs/__init__.py @@ -1,23 +1,16 @@ """Pre-aggregated roll-ups of ``eap_items`` that ``TraceItemAttributeNames`` reads from. -The two roll-ups have different schemas, so choosing one also decides part of the query's -shape. ``CoOccurringAttrsSource`` is that choice, one implementation per table: +``TraceItemAttributeNames`` reads ``eap_item_co_occurring_attrs_v2`` exclusively. +``CoOccurringAttrsV1`` remains as the query shape for the original table, which is +still populated but no longer served. v1.py eap_item_co_occurring_attrs ReplacingMergeTree, scalar key arrays only v2.py eap_item_co_occurring_attrs_v2 SummingMergeTree, one key array per type - -``for_request`` picks the source for a request. """ from snuba.web.rpc.v1.resolvers.R_eap_items.co_occurring_attrs.base import ( CoOccurringAttrsSource, ) -from snuba.web.rpc.v1.resolvers.R_eap_items.co_occurring_attrs.selection import ( - CO_OCCURRING_ATTRS_V2_OPTION, - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_DEFAULT, - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_OPTION, - for_request, -) from snuba.web.rpc.v1.resolvers.R_eap_items.co_occurring_attrs.v1 import ( CO_OCCURRING_ATTRS_STORAGE_KEY, V1, @@ -31,14 +24,10 @@ __all__ = [ "CO_OCCURRING_ATTRS_STORAGE_KEY", - "CO_OCCURRING_ATTRS_V2_OPTION", - "CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_DEFAULT", - "CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_OPTION", "CO_OCCURRING_ATTRS_V2_STORAGE_KEY", "V1", "V2", "CoOccurringAttrsSource", "CoOccurringAttrsV1", "CoOccurringAttrsV2", - "for_request", ] diff --git a/snuba/web/rpc/v1/resolvers/R_eap_items/co_occurring_attrs/selection.py b/snuba/web/rpc/v1/resolvers/R_eap_items/co_occurring_attrs/selection.py deleted file mode 100644 index 4c77b9e9fd5..00000000000 --- a/snuba/web/rpc/v1/resolvers/R_eap_items/co_occurring_attrs/selection.py +++ /dev/null @@ -1,55 +0,0 @@ -"""Which co-occurring-attributes source a request reads, and how the v2 rollout is gated.""" - -from __future__ import annotations - -from datetime import UTC, datetime - -from sentry_protos.snuba.v1.endpoint_trace_item_attributes_pb2 import ( - TraceItemAttributeNamesRequest, -) - -from snuba.state.sentry_options import get_option -from snuba.web.rpc.common.common import prev_monday -from snuba.web.rpc.v1.resolvers.R_eap_items.co_occurring_attrs.base import ( - CoOccurringAttrsSource, -) -from snuba.web.rpc.v1.resolvers.R_eap_items.co_occurring_attrs.v1 import V1 -from snuba.web.rpc.v1.resolvers.R_eap_items.co_occurring_attrs.v2 import V2 - -# Rollout flag. Not sufficient on its own: a request must also fall inside the window v2 has -# data for, see for_request. -CO_OCCURRING_ATTRS_V2_OPTION = "use_co_occurring_attrs_v2" - -# First `date` bucket v2 holds data for. Its materialized view was created 2026-07-29 and -# only appends from then on, so v2 has nothing before the Monday of that week. Must stay a -# Monday: `date` is bucketed with toMonday() and queries round down to the previous Monday, -# so a mid-week cutoff would admit requests that then read a bucket only v1 has. No backfill -# is planned, so this stays until v1 is retired. -CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_OPTION = "co_occurring_attrs_v2_start_timestamp" -CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_DEFAULT = 1785110400 # 2026-07-27 00:00:00 UTC - - -def _v2_covers_request_window(request: TraceItemAttributeNamesRequest) -> bool: - """Whether v2 has data for the whole time range the request asks about. - - Compares the *rounded* lower bound, since that is the bucket the query actually reads (see - ``get_co_occurring_attributes_date_condition``). Comparing the raw timestamp would let a - request starting just after the cutoff read the preceding, non-existent bucket. - """ - start_timestamp = get_option( - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_OPTION, - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_DEFAULT, - ) - earliest_bucket = prev_monday( - request.meta.start_timestamp.ToDatetime().replace(hour=0, minute=0, second=0) - ) - # ToDatetime() is naive UTC, so drop the tzinfo to compare like-for-like - v2_start = datetime.fromtimestamp(start_timestamp, UTC).replace(tzinfo=None) - return earliest_bucket >= v2_start - - -def for_request(request: TraceItemAttributeNamesRequest) -> CoOccurringAttrsSource: - """The source a request should read, falling back to v1 outside v2's data window.""" - if not get_option(CO_OCCURRING_ATTRS_V2_OPTION, False): - return V1 - return V2 if _v2_covers_request_window(request) else V1 diff --git a/tests/web/rpc/v1/resolvers/R_eap_items/co_occurring_attrs/test_selection.py b/tests/web/rpc/v1/resolvers/R_eap_items/co_occurring_attrs/test_selection.py deleted file mode 100644 index f847cda1311..00000000000 --- a/tests/web/rpc/v1/resolvers/R_eap_items/co_occurring_attrs/test_selection.py +++ /dev/null @@ -1,86 +0,0 @@ -"""How the v2 rollout is gated: which source a given request is routed to.""" - -from datetime import UTC, datetime, timedelta - -import pytest -from sentry_options.testing import override_options -from sentry_protos.snuba.v1.endpoint_trace_item_attributes_pb2 import ( - TraceItemAttributeNamesRequest, -) -from sentry_protos.snuba.v1.request_common_pb2 import RequestMeta -from sentry_protos.snuba.v1.trace_item_attribute_pb2 import AttributeKey - -from snuba.web.rpc.v1.resolvers.R_eap_items import co_occurring_attrs -from snuba.web.rpc.v1.resolvers.R_eap_items.co_occurring_attrs import V1, V2 -from snuba.web.rpc.v1.resolvers.R_eap_items.co_occurring_attrs.selection import ( - CO_OCCURRING_ATTRS_V2_OPTION, - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_DEFAULT, - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_OPTION, -) - -V2_START = datetime.fromtimestamp(CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_DEFAULT, UTC) - - -def _request( - start: datetime, - attr_type: AttributeKey.Type.ValueType = AttributeKey.Type.TYPE_STRING, -) -> TraceItemAttributeNamesRequest: - req = TraceItemAttributeNamesRequest(meta=RequestMeta(project_ids=[1], organization_id=1)) - req.meta.start_timestamp.FromDatetime(start) - req.meta.end_timestamp.FromDatetime(start + timedelta(hours=1)) - req.type = attr_type - return req - - -@pytest.mark.redis_db -class TestForRequest: - def test_flag_off_reads_v1(self) -> None: - with override_options("snuba", {CO_OCCURRING_ATTRS_V2_OPTION: False}): - assert co_occurring_attrs.for_request(_request(V2_START)) is V1 - - def test_flag_off_reads_v1_even_well_inside_the_window(self) -> None: - with override_options("snuba", {CO_OCCURRING_ATTRS_V2_OPTION: False}): - assert co_occurring_attrs.for_request(_request(V2_START + timedelta(days=90))) is V1 - - def test_flag_on_inside_window_reads_v2(self) -> None: - with override_options("snuba", {CO_OCCURRING_ATTRS_V2_OPTION: True}): - assert co_occurring_attrs.for_request(_request(V2_START)) is V2 - - def test_flag_on_before_window_falls_back_to_v1(self) -> None: - with override_options("snuba", {CO_OCCURRING_ATTRS_V2_OPTION: True}): - assert co_occurring_attrs.for_request(_request(V2_START - timedelta(seconds=1))) is V1 - - def test_gate_compares_the_rounded_bucket(self) -> None: - """The gate must compare the bucket the query reads, not the raw start timestamp. - - With a mid-week cutoff, a request starting after that instant still reads from the - Monday before it — a bucket v2 never populated — so it must fall back to v1. - """ - wednesday = V2_START + timedelta(days=2) - with override_options( - "snuba", - { - CO_OCCURRING_ATTRS_V2_OPTION: True, - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_OPTION: int(wednesday.timestamp()), - }, - ): - assert co_occurring_attrs.for_request(_request(wednesday + timedelta(hours=1))) is V1 - - def test_start_timestamp_option_widens_the_window(self) -> None: - old = _request(V2_START - timedelta(days=365)) - with override_options("snuba", {CO_OCCURRING_ATTRS_V2_OPTION: True}): - assert co_occurring_attrs.for_request(old) is V1 - with override_options( - "snuba", - { - CO_OCCURRING_ATTRS_V2_OPTION: True, - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_OPTION: 0, - }, - ): - assert co_occurring_attrs.for_request(old) is V2 - - def test_default_cutoff_is_a_monday(self) -> None: - """`date` is bucketed with toMonday() and the query rounds down to the previous - Monday, so a mid-week cutoff would admit requests that read a v1-only bucket.""" - assert V2_START.weekday() == 0 - assert (V2_START.hour, V2_START.minute, V2_START.second) == (0, 0, 0) 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 a31292397cf..a9279b331b4 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 @@ -1,10 +1,8 @@ import uuid -from collections.abc import Generator from datetime import UTC, datetime, timedelta import pytest from google.protobuf.timestamp_pb2 import Timestamp -from sentry_options.testing import override_options from sentry_protos.snuba.v1.endpoint_trace_item_attributes_pb2 import ( TraceItemAttributeNamesRequest, TraceItemAttributeNamesResponse, @@ -24,9 +22,6 @@ get_co_occurring_attributes, ) from snuba.web.rpc.v1.resolvers.R_eap_items.co_occurring_attrs import ( - CO_OCCURRING_ATTRS_STORAGE_KEY, - CO_OCCURRING_ATTRS_V2_OPTION, - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_OPTION, CO_OCCURRING_ATTRS_V2_STORAGE_KEY, ) from tests.base import BaseApiTest @@ -81,25 +76,6 @@ def setup_teardown(eap: None, redis_db: None) -> None: populate_eap_spans_storage(num_rows=TOTAL_GENERATED_SPANS) -@pytest.fixture(autouse=True, params=[False, True], ids=["co_occurring_v1", "co_occurring_v2"]) -def co_occurring_storage(request: pytest.FixtureRequest) -> Generator[bool]: - """Run every test in this module against both co-occurring-attributes storages, so the - shared behaviour holds on either side of the rollout. - - The v2 start timestamp is pinned back so the date gate does not send the v2 leg to v1: - whether ``BASE_TIME`` clears the real cutoff depends on the day the suite runs. - """ - use_v2 = bool(request.param) - with override_options( - "snuba", - { - CO_OCCURRING_ATTRS_V2_OPTION: use_v2, - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_OPTION: 0, - }, - ): - yield use_v2 - - @pytest.mark.eap @pytest.mark.redis_db class TestTraceItemAttributeNames(BaseApiTest): @@ -305,10 +281,7 @@ def test_response_metadata(self) -> None: res = EndpointTraceItemAttributeNames().execute(req) assert res.meta.query_info != [] - def test_reads_the_storage_selected_by_the_rollout_flag( - self, co_occurring_storage: bool - ) -> None: - """The option picks which storage is read, in either direction.""" + def test_reads_v2_storage(self) -> None: req = TraceItemAttributeNamesRequest( meta=RequestMeta( project_ids=[1, 2, 3], @@ -322,14 +295,9 @@ def test_reads_the_storage_selected_by_the_rollout_flag( limit=10, type=AttributeKey.Type.TYPE_STRING, ) - expected = ( - CO_OCCURRING_ATTRS_V2_STORAGE_KEY - if co_occurring_storage - else CO_OCCURRING_ATTRS_STORAGE_KEY - ) from_clause = get_co_occurring_attributes(req).query.get_from_clause() assert isinstance(from_clause, StorageDataSource) - assert from_clause.key == expected + assert from_clause.key == CO_OCCURRING_ATTRS_V2_STORAGE_KEY def test_basic_co_occurring_attrs(self) -> None: req = TraceItemAttributeNamesRequest( diff --git a/tests/web/rpc/v1/test_endpoint_trace_item_attribute_names_v2.py b/tests/web/rpc/v1/test_endpoint_trace_item_attribute_names_v2.py index ba51c9a9c7c..90400d149cc 100644 --- a/tests/web/rpc/v1/test_endpoint_trace_item_attribute_names_v2.py +++ b/tests/web/rpc/v1/test_endpoint_trace_item_attribute_names_v2.py @@ -1,18 +1,14 @@ """Behaviour only v2 can provide: per-type attribute keys, summed item counts, last_seen. -Behaviour shared with v1 is covered against both storages by the parameterized suite in -``test_endpoint_trace_item_attribute_names.py``. +Shared behaviour is covered by ``test_endpoint_trace_item_attribute_names.py``. """ import uuid from collections.abc import Generator from datetime import UTC, datetime, timedelta -from unittest import mock import pytest from google.protobuf.timestamp_pb2 import Timestamp -from sentry_options import OptionValue -from sentry_options.testing import override_options from sentry_protos.snuba.v1.endpoint_trace_item_attributes_pb2 import ( TraceItemAttributeNamesRequest, TraceItemAttributeNamesResponse, @@ -27,20 +23,13 @@ from snuba.datasets.storages.storage_key import StorageKey from snuba.query.data_source.simple import Storage as StorageDataSource from snuba.query.expressions import Column, FunctionCall -from snuba.utils.metrics.backends.testing import get_recorded_metric_calls from snuba.web.rpc.v1.endpoint_trace_item_attribute_names import ( EndpointTraceItemAttributeNames, get_co_occurring_attributes, ) -from snuba.web.rpc.v1.resolvers.R_eap_items import co_occurring_attrs from snuba.web.rpc.v1.resolvers.R_eap_items.co_occurring_attrs import ( CO_OCCURRING_ATTRS_STORAGE_KEY, - CO_OCCURRING_ATTRS_V2_OPTION, - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_DEFAULT, - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_OPTION, CO_OCCURRING_ATTRS_V2_STORAGE_KEY, - V1, - V2, ) from tests.base import BaseApiTest from tests.helpers import write_raw_unprocessed_events @@ -48,18 +37,6 @@ BASE_TIME = datetime.now(UTC).replace(minute=0, second=0, microsecond=0) - timedelta(hours=3) -# The two ways a request ends up reading v1: the rollout flag is off, or the flag is on but -# the date gate routed an older time range there. Behaviour must be identical in both. -ROUTES_TO_V1: list[dict[str, OptionValue]] = [ - {CO_OCCURRING_ATTRS_V2_OPTION: False}, - { - CO_OCCURRING_ATTRS_V2_OPTION: True, - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_OPTION: int( - (BASE_TIME + timedelta(days=365)).timestamp() - ), - }, -] - # Number of items written, which is also the expected `count` for every attribute below # since each attribute is present on every item. NUM_ITEMS = 3 @@ -105,16 +82,7 @@ def setup_teardown(eap: None, redis_db: None) -> Generator[None]: for _ in range(NUM_ITEMS) ], ) - # Pin the start timestamp back so the date gate (covered separately below) stays out of - # the way. - with override_options( - "snuba", - { - CO_OCCURRING_ATTRS_V2_OPTION: True, - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_OPTION: 0, - }, - ): - yield + yield def _request( @@ -166,20 +134,12 @@ def _names_and_types( @pytest.mark.eap @pytest.mark.redis_db class TestTraceItemAttributeNamesV2(BaseApiTest): - def test_reads_v2_storage_when_enabled(self) -> None: + def test_reads_v2_storage(self) -> None: assert ( _queried_storage_key(_request(AttributeKey.Type.TYPE_STRING)) == CO_OCCURRING_ATTRS_V2_STORAGE_KEY ) - def test_reads_v1_storage_when_disabled(self) -> None: - """The option is a rollback switch: turning it off restores the v1 read.""" - with override_options("snuba", {CO_OCCURRING_ATTRS_V2_OPTION: False}): - assert ( - _queried_storage_key(_request(AttributeKey.Type.TYPE_STRING)) - == CO_OCCURRING_ATTRS_STORAGE_KEY - ) - def test_int_keys_typed_as_int(self) -> None: """Dedicated attributes_int array, so int keys keep their type (v1 folds them into the float array).""" @@ -241,21 +201,13 @@ def test_unspecified_type_includes_array_keys_without_duplicating_ints(self) -> ] def test_count_sums_occurrence_column(self) -> None: - """Rows carry an occurrence `count`, so this sums to the number of items. The same - request on v1 counts attribute sets, which here is 1.""" + """Rows carry an occurrence `count`, so this sums to the number of items.""" res = EndpointTraceItemAttributeNames().execute( _request(AttributeKey.Type.TYPE_STRING, order_by_count=True) ) counts = {attr.name: attr.count for attr in res.attributes} assert counts == {"probe_str": NUM_ITEMS} - with override_options("snuba", {CO_OCCURRING_ATTRS_V2_OPTION: False}): - v1_res = EndpointTraceItemAttributeNames().execute( - _request(AttributeKey.Type.TYPE_STRING, order_by_count=True) - ) - v1_counts = {attr.name: attr.count for attr in v1_res.attributes} - assert v1_counts == {"probe_str": 1} - def test_count_populated_for_int_and_array_keys(self) -> None: """Available for the types v1 cannot surface at all.""" for attr_type in ( @@ -285,108 +237,10 @@ def test_substring_match_prefilters_the_typed_array(self) -> None: assert array_exists_columns == {"attributes_array_int"} -@pytest.mark.eap -@pytest.mark.redis_db -class TestCoOccurringV2DateGate(BaseApiTest): - """A request reaching back before v2's materialized view existed must read v1, or the - attributes that only existed in the earlier part of its range vanish. - """ - - V2_START = datetime.fromtimestamp(CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_DEFAULT, UTC) - - @pytest.fixture(autouse=True) - def use_real_cutoff(self) -> Generator[None]: - """Undo the module fixture's pinned cutoff so the gate is exercised.""" - with override_options( - "snuba", - { - CO_OCCURRING_ATTRS_V2_OPTION: True, - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_OPTION: ( - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_DEFAULT - ), - }, - ): - yield - - def _storage_for_start(self, start: datetime) -> StorageKey: - req = _request(AttributeKey.Type.TYPE_STRING) - req.meta.start_timestamp.FromDatetime(start) - req.meta.end_timestamp.FromDatetime(start + timedelta(hours=1)) - return _queried_storage_key(req) - - def test_default_cutoff_is_a_monday(self) -> None: - """`date` is bucketed with toMonday() and queries round down to the previous Monday, so - a mid-week cutoff would let a request round *below* it into a v1-only bucket.""" - assert self.V2_START.weekday() == 0 - assert (self.V2_START.hour, self.V2_START.minute, self.V2_START.second) == (0, 0, 0) - - def test_request_starting_at_the_cutoff_reads_v2(self) -> None: - assert self._storage_for_start(self.V2_START) == CO_OCCURRING_ATTRS_V2_STORAGE_KEY - - def test_request_starting_after_the_cutoff_reads_v2(self) -> None: - """Later in the same week rounds down to exactly the cutoff bucket.""" - assert ( - self._storage_for_start(self.V2_START + timedelta(days=3)) - == CO_OCCURRING_ATTRS_V2_STORAGE_KEY - ) - - def test_request_starting_before_the_cutoff_falls_back_to_v1(self) -> None: - """One second earlier rounds to the previous Monday, which v2 never populated.""" - assert ( - self._storage_for_start(self.V2_START - timedelta(seconds=1)) - == CO_OCCURRING_ATTRS_STORAGE_KEY - ) - - def test_request_reaching_far_back_falls_back_to_v1(self) -> None: - assert ( - self._storage_for_start(self.V2_START - timedelta(days=30)) - == CO_OCCURRING_ATTRS_STORAGE_KEY - ) - - def test_gate_uses_rounded_lower_bound_not_raw_timestamp(self) -> None: - """The gate must compare the bucket the query reads, not the raw start timestamp. - - With a mid-week cutoff, a request starting just after it still reads from the Monday - before — a bucket only v1 has — so a raw-timestamp comparison would wrongly admit it. - """ - wednesday_cutoff = self.V2_START + timedelta(days=2) - with override_options( - "snuba", - { - CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_OPTION: int(wednesday_cutoff.timestamp()), - }, - ): - # Starts after the cutoff instant, but rounds down to the Monday before it. - assert ( - self._storage_for_start(wednesday_cutoff + timedelta(hours=1)) - == CO_OCCURRING_ATTRS_STORAGE_KEY - ) - - def test_start_timestamp_is_configurable(self) -> None: - """Lowering the option widens the v2 window.""" - before_cutoff = self.V2_START - timedelta(days=30) - assert self._storage_for_start(before_cutoff) == CO_OCCURRING_ATTRS_STORAGE_KEY - with override_options("snuba", {CO_OCCURRING_ATTRS_V2_START_TIMESTAMP_OPTION: 0}): - assert self._storage_for_start(before_cutoff) == CO_OCCURRING_ATTRS_V2_STORAGE_KEY - - def test_flag_off_reads_v1_even_inside_the_v2_window(self) -> None: - """The rollout flag remains an unconditional off switch.""" - with override_options("snuba", {CO_OCCURRING_ATTRS_V2_OPTION: False}): - assert self._storage_for_start(self.V2_START) == CO_OCCURRING_ATTRS_STORAGE_KEY - - def test_gated_fallback_still_returns_attributes(self) -> None: - """Served by v1, so it must still return attributes rather than an empty result.""" - req = _request(AttributeKey.Type.TYPE_STRING) - req.meta.start_timestamp.FromDatetime(self.V2_START - timedelta(days=30)) - assert _queried_storage_key(req) == CO_OCCURRING_ATTRS_STORAGE_KEY - res = EndpointTraceItemAttributeNames().execute(req) - assert [attr.name for attr in res.attributes] == ["probe_str"] - - @pytest.mark.eap @pytest.mark.redis_db class TestLastSeen(BaseApiTest): - """Reporting and ordering by `last_seen`, which only v2 records. + """Reporting and ordering by `last_seen`. The module fixture writes every probe attribute at one timestamp, which cannot tell a recency ordering from an arbitrary one, so this class writes its own at distinct times. @@ -406,9 +260,8 @@ class TestLastSeen(BaseApiTest): # Most recent first: the reverse of write order. BY_RECENCY_DESC = ["ls_newest", "ls_middle", "ls_oldest"] - # Most frequent first: the exact opposite of BY_RECENCY_DESC, which is what makes a - # degraded recency request distinguishable from an honoured one. Both storages agree on - # this order here (see the filler attribute below), though they count different things. + # Most frequent first: the exact opposite of BY_RECENCY_DESC, so neither ordering can + # be satisfied by accident by the other. BY_COUNT_DESC = ["ls_oldest", "ls_middle", "ls_newest"] @pytest.fixture(autouse=True) @@ -416,14 +269,8 @@ def staggered_items(self) -> None: items_storage = get_writable_storage(StorageKey("eap_items")) for name, hours_ago in self.OFFSETS.items(): # All of an attribute's items share a timestamp, so last_seen stays exact while - # the number of items drives its count. - # - # Each item also gets a unique filler attribute so that every item is a distinct - # attribute-key *set*. Without it the items for one attribute collapse into a - # single set, and v1 — which counts sets, not items — reports 1 for everything, - # making its count ordering degenerate into the name tiebreak. The filler names - # deliberately avoid the "ls_" substring the tests filter on, so they stay out of - # the results. + # the number of items drives its count. Unique filler attributes keep each item + # a distinct key set; their names avoid the "ls_" substring the tests filter on. write_raw_unprocessed_events( items_storage, [ @@ -488,69 +335,14 @@ def test_last_seen_is_unset_under_name_ordering(self) -> None: for attribute in attributes: assert not attribute.HasField("last_seen") - @pytest.mark.parametrize("route_index", [0, 1], ids=["flag_off", "date_gate"]) - def test_recency_ordering_degrades_to_count_on_v1(self, route_index: int) -> None: - """v1 has no last_seen, so a recency request falls back to frequency ordering rather - than failing, and stays detectable because last_seen is absent from the response. - - Covers both ways a request lands on v1: the flag being off, and the date gate. - """ - with override_options("snuba", ROUTES_TO_V1[route_index]): - attributes = self._run( - column=TraceItemAttributeNamesRequest.OrderBy.Column.COLUMN_LAST_SEEN - ) - # No error, and the attributes still come back — in count order, which over this - # data is the exact reverse of the recency order that was asked for. - assert [a.name for a in attributes] == self.BY_COUNT_DESC - # Counts are populated; last_seen cannot be, so it stays unset. - assert all(a.HasField("count") for a in attributes) - assert all(not a.HasField("last_seen") for a in attributes) - - @pytest.mark.parametrize("route_index", [0, 1], ids=["flag_off", "date_gate"]) - def test_degraded_ordering_matches_a_real_count_ordering(self, route_index: int) -> None: - """Catches the ordering being derived inconsistently between the ClickHouse ORDER BY - and the Python re-sort, which would make the two drift apart.""" - with override_options("snuba", ROUTES_TO_V1[route_index]): - degraded = self._run( - column=TraceItemAttributeNamesRequest.OrderBy.Column.COLUMN_LAST_SEEN - ) - by_count = self._run(column=TraceItemAttributeNamesRequest.OrderBy.Column.COLUMN_COUNT) - assert [a.name for a in degraded] == [a.name for a in by_count] - assert [a.count for a in degraded] == [a.count for a in by_count] - - def test_other_orderings_still_work_on_v1(self) -> None: - """Only recency ordering degrades.""" - with override_options("snuba", {CO_OCCURRING_ATTRS_V2_OPTION: False}): - by_count = self._run(column=TraceItemAttributeNamesRequest.OrderBy.Column.COLUMN_COUNT) - assert [a.name for a in by_count] == self.BY_COUNT_DESC - # v1 cannot report last_seen, so it must be left unset rather than zeroed. - assert all(not a.HasField("last_seen") for a in by_count) - - def test_recency_ordering_is_honoured_on_v2(self) -> None: - """Guard against the degrade firing when it should not.""" + def test_recency_ordering_is_honoured(self) -> None: + """Most recently used attributes first, with last_seen populated.""" by_recency = self._run( column=TraceItemAttributeNamesRequest.OrderBy.Column.COLUMN_LAST_SEEN ) assert [a.name for a in by_recency] == self.BY_RECENCY_DESC assert all(a.HasField("last_seen") for a in by_recency) - @pytest.mark.parametrize("route_index", [0, 1], ids=["flag_off", "date_gate"]) - def test_degrade_is_recorded_as_a_metric(self, route_index: int) -> None: - """Invisible to the caller, so it has to be visible to us during the rollout.""" - with override_options("snuba", ROUTES_TO_V1[route_index]): - self._run(column=TraceItemAttributeNamesRequest.OrderBy.Column.COLUMN_LAST_SEEN) - calls = get_recorded_metric_calls("increment", "rpc.attribute_names_order_by_degraded") - assert calls, "expected a degrade metric" - tags = calls[-1].tags or {} - assert tags.get("requested") == "COLUMN_LAST_SEEN" - assert tags.get("applied") == "COLUMN_COUNT" - assert tags.get("storage") == CO_OCCURRING_ATTRS_STORAGE_KEY.value - - def test_no_degrade_metric_when_honoured(self) -> None: - """Guard against the metric firing on v2.""" - self._run(column=TraceItemAttributeNamesRequest.OrderBy.Column.COLUMN_LAST_SEEN) - assert not get_recorded_metric_calls("increment", "rpc.attribute_names_order_by_degraded") - def test_recency_and_count_orderings_differ_on_v2(self) -> None: """Checks the fixture data as much as the code: the two aggregating orderings are exact opposites here, so neither can be satisfied by accident by the other.""" @@ -563,36 +355,3 @@ def test_recency_and_count_orderings_differ_on_v2(self) -> None: assert list(reversed(self.BY_COUNT_DESC)) == self.BY_RECENCY_DESC # v2 counts items, so the counts are the number written rather than a flat 1. assert {a.name: a.count for a in by_count} == self.ITEM_COUNTS - - def test_source_is_resolved_once_per_request(self) -> None: - """Resolving reads runtime options, so a second resolution could see a different value - if an option flips mid-request: the query would be built for one storage while the - response is re-sorted as if it were the other (regression guard: the count was 2). - """ - with mock.patch.object( - co_occurring_attrs, "for_request", wraps=co_occurring_attrs.for_request - ) as resolve: - self._run(column=TraceItemAttributeNamesRequest.OrderBy.Column.COLUMN_LAST_SEEN) - assert resolve.call_count == 1, ( - f"source resolved {resolve.call_count} times; the query and the response " - "converter must share a single resolution" - ) - - def test_ordering_survives_the_source_changing_mid_request(self) -> None: - """Simulates an option flipping between resolutions by returning v2 then v1. With a - single resolution the second value is never read, so the result stays coherent. - """ - sources = iter([V2, V1]) - - def flipping(_request: TraceItemAttributeNamesRequest) -> object: - return next(sources, V1) - - with mock.patch.object(co_occurring_attrs, "for_request", side_effect=flipping): - attributes = self._run( - column=TraceItemAttributeNamesRequest.OrderBy.Column.COLUMN_LAST_SEEN - ) - # v2 was resolved, so recency ordering is honoured end to end: the order is the - # requested one and last_seen is populated, not the misordered mix the double - # resolution produced. - assert [a.name for a in attributes] == self.BY_RECENCY_DESC - assert all(a.HasField("last_seen") for a in attributes)