From f364897388144440d365fbe28122dcd3c19f1b91 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 17 Apr 2026 17:43:43 -0700 Subject: [PATCH] ref(flags): Remove organizations:on-demand-metrics-query-spec-version-two MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The flag scanner classified this as flagpole-disabled. The flag gated two things: (1) choosing OnDemandMetricSpec version 2 at query time and (2) allowing count_unique and user_misery as on-demand functions. With the flag always disabled, get_query_spec_version always returns v1 and count_unique/user_misery are always rejected — promotes the disallow list to OPS_DISALLOWED and drops the feature check. Spec version 2 storage-side generation is preserved so existing write-side infrastructure remains intact. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/sentry/features/temporary.py | 1 - src/sentry/snuba/metrics/extraction.py | 33 +---- .../relay/config/test_metric_extraction.py | 37 +---- .../search/events/builder/test_metrics.py | 132 +---------------- .../endpoints/test_organization_events_mep.py | 134 ------------------ 5 files changed, 9 insertions(+), 328 deletions(-) diff --git a/src/sentry/features/temporary.py b/src/sentry/features/temporary.py index 83c6cedf3fe8c0..a7ca3eb92d5225 100644 --- a/src/sentry/features/temporary.py +++ b/src/sentry/features/temporary.py @@ -192,7 +192,6 @@ def register_temporary_features(manager: FeatureManager) -> None: # Extract on demand metrics (widget extraction) manager.add("organizations:on-demand-metrics-extraction-widgets", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) # This spec version includes the environment in the query hash - manager.add("organizations:on-demand-metrics-query-spec-version-two", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) # Use the new OrganizationMemberInvite endpoints manager.add("organizations:new-organization-member-invite", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True) # Display on demand metrics related UI elements diff --git a/src/sentry/snuba/metrics/extraction.py b/src/sentry/snuba/metrics/extraction.py index 853f4b62925355..929cd05fa8dee1 100644 --- a/src/sentry/snuba/metrics/extraction.py +++ b/src/sentry/snuba/metrics/extraction.py @@ -2,14 +2,12 @@ import hashlib import logging -import os from collections import defaultdict from collections.abc import Callable, Sequence from dataclasses import dataclass from enum import Enum from typing import Any, Literal, NamedTuple, NotRequired, Optional, Self, TypedDict, TypeVar, cast -import sentry_sdk from django.utils.functional import cached_property from sentry import features @@ -43,12 +41,8 @@ logger = logging.getLogger(__name__) -SPEC_VERSION_TWO_FLAG = "organizations:on-demand-metrics-query-spec-version-two" -# Certain functions will only be supported with certain feature flags -OPS_REQUIRE_FEAT_FLAG = { - "count_unique": SPEC_VERSION_TWO_FLAG, - "user_misery": SPEC_VERSION_TWO_FLAG, -} +# Functions that are not allowed for on-demand metric querying. +OPS_DISALLOWED: set[str] = {"count_unique", "user_misery"} # Splits the bulk cache for on-demand resolution into N chunks WIDGET_QUERY_CACHE_MAX_CHUNKS = 6 @@ -87,10 +81,6 @@ class OnDemandMetricSpecVersioning: @classmethod def get_query_spec_version(cls: Any, organization_id: int) -> SpecVersion: - """Return spec version based on feature flag enabled for an organization.""" - org = Organization.objects.get_from_cache(id=organization_id) - if features.has(SPEC_VERSION_TWO_FLAG, org): - return cls.spec_versions[1] return cls.spec_versions[0] @classmethod @@ -572,23 +562,8 @@ def should_use_on_demand_metrics_for_querying(organization: Organization, **kwar return False function, _ = components - # This helps us control which functions are allowed to use the new spec version. - if function in OPS_REQUIRE_FEAT_FLAG: - if not organization: - # We need to let devs writting tests that if they intend to use a function that requires a feature flag - # that the organization needs to be included in the test. - if os.environ.get("PYTEST_CURRENT_TEST"): - logger.error("Pass the organization to create the spec for this function.") - sentry_sdk.capture_message( - f"Organization is required for {function} on-demand metrics." - ) - return False - feat_flag = OPS_REQUIRE_FEAT_FLAG[function] - if not features.has(feat_flag, organization): - if os.environ.get("PYTEST_CURRENT_TEST"): - # This will show up in the logs and help the developer understand why the test is failing - logger.error("Add the feature flag to create the spec for this function.") - return False + if function in OPS_DISALLOWED: + return False supported_by = _query_supported_by(**kwargs) if ( diff --git a/tests/sentry/relay/config/test_metric_extraction.py b/tests/sentry/relay/config/test_metric_extraction.py index 2f0bfdadb880cd..1491ce8839b606 100644 --- a/tests/sentry/relay/config/test_metric_extraction.py +++ b/tests/sentry/relay/config/test_metric_extraction.py @@ -1760,12 +1760,7 @@ def test_stateful_get_metric_extraction_config_enabled_with_multiple_versions( widget_type: int, ) -> None: duration = 1000 - with Feature( - { - ON_DEMAND_METRICS_WIDGETS: True, - "organizations:on-demand-metrics-query-spec-version-two": True, - } - ): + with Feature({ON_DEMAND_METRICS_WIDGETS: True}): widget_query, _, _ = create_widget( ["epm()"], f"transaction.duration:>={duration}", @@ -2109,15 +2104,13 @@ def test_include_environment_for_widgets(default_project: Project, widget_type: with Feature([ON_DEMAND_METRICS, ON_DEMAND_METRICS_WIDGETS]): widget, _, _ = create_widget([aggr], query, default_project, widget_type=widget_type) config = get_metric_extraction_config(default_project) - # Because we have two specs we will have two metrics. - # The second spec includes the environment tag as part of the query hash. + # Two spec versions are still generated at write time; query-time always + # uses spec version 1. assert config and config["metrics"] == [ widget_to_metric_spec("f1353b0f", condition), widget_to_metric_spec("4fb5a472", condition), ] - # We now verify that the string used for hashing is what we expect - # Since we're using the current spec it will not include the environment tag expected_query_str_hash = f"None;{condition}" spec = _on_demand_spec_from_widget(default_project, widget) assert spec.query_hash == "f1353b0f" @@ -2125,13 +2118,6 @@ def test_include_environment_for_widgets(default_project: Project, widget_type: assert spec.spec_version.version == 1 assert spec.spec_version.flags == set() - with Feature("organizations:on-demand-metrics-query-spec-version-two"): - spec = _on_demand_spec_from_widget(default_project, widget) - assert spec._query_str_for_hash == f"{expected_query_str_hash};['environment']" - assert spec.query_hash == "4fb5a472" - assert spec.spec_version.version == 2 - assert spec.spec_version.flags == {"include_environment_tag"} - @django_db_all @override_options({"on_demand_metrics.check_widgets.enable": True}) @@ -2249,16 +2235,6 @@ def test_alert_and_widget_colliding(default_project: Project, widget_type: int) assert widget_spec._query_str_for_hash == expected_query_str_hash assert alert_spec._query_str_for_hash == expected_query_str_hash - with Feature("organizations:on-demand-metrics-query-spec-version-two"): - widget_spec = _on_demand_spec_from_widget(default_project, widget) - assert widget_spec._query_str_for_hash == f"{expected_query_str_hash};['environment']" - assert widget_spec.query_hash == "4fb5a472" - assert widget_spec.spec_version.version == 2 - assert widget_spec.spec_version.flags == {"include_environment_tag"} - - # With the new spec version they will not collide anymore - assert widget_spec.query_hash != alert_spec.query_hash - foo_bar_condition = {"name": "event.tags.foo", "op": "eq", "value": "bar"} not_event_type_cond = { @@ -2329,12 +2305,7 @@ def test_level_field(default_project: Project, widget_type: int) -> None: ) def test_widget_modifed_after_on_demand(default_project: Project, widget_type: int) -> None: duration = 1000 - with Feature( - { - ON_DEMAND_METRICS_WIDGETS: True, - "organizations:on-demand-metrics-query-spec-version-two": True, - } - ): + with Feature({ON_DEMAND_METRICS_WIDGETS: True}): widget_query, _, _ = create_widget( ["epm()"], f"transaction.duration:>={duration}", diff --git a/tests/sentry/search/events/builder/test_metrics.py b/tests/sentry/search/events/builder/test_metrics.py index c332229037115c..2a61872340c1f6 100644 --- a/tests/sentry/search/events/builder/test_metrics.py +++ b/tests/sentry/search/events/builder/test_metrics.py @@ -22,10 +22,9 @@ from sentry.sentry_metrics.aggregation_option_registry import AggregationOption from sentry.sentry_metrics.use_case_id_registry import UseCaseID from sentry.sentry_metrics.utils import resolve_tag_value -from sentry.snuba.dataset import Dataset, EntityKey +from sentry.snuba.dataset import Dataset from sentry.snuba.metrics.extraction import ( QUERY_HASH_KEY, - SPEC_VERSION_TWO_FLAG, MetricSpecType, OnDemandMetricSpec, ) @@ -33,7 +32,6 @@ from sentry.snuba.metrics.naming_layer.mri import TransactionMRI from sentry.testutils.cases import MetricsEnhancedPerformanceTestCase from sentry.testutils.helpers import Feature -from sentry.testutils.helpers.discover import user_misery_formula pytestmark = pytest.mark.sentry_metrics @@ -2108,39 +2106,6 @@ def test_run_query_with_on_demand_count(self) -> None: ], ) - # Once we delete the current spec version this test will fail and we can delete it - def test_on_demand_builder_with_new_spec(self) -> None: - field = "count()" - query = "transaction.duration:>0" - spec = OnDemandMetricSpec(field=field, query=query, spec_type=MetricSpecType.DYNAMIC_QUERY) - # As expected, it does not include the environment tag - expected_str_hash = "None;{'name': 'event.duration', 'op': 'gt', 'value': 0.0}" - assert spec._query_str_for_hash == expected_str_hash - - # Because we call the builder with the feature flag we will get the environment to be included - with Feature(SPEC_VERSION_TWO_FLAG): - query_builder = TimeseriesMetricQueryBuilder( - self.params, - dataset=Dataset.PerformanceMetrics, - interval=3600, - query=query, - selected_columns=[field], - config=QueryBuilderConfig( - on_demand_metrics_enabled=True, - on_demand_metrics_type=MetricSpecType.DYNAMIC_QUERY, - ), - ) - spec_in_use: OnDemandMetricSpec | None = ( - query_builder._on_demand_metric_spec_map[field] - if query_builder._on_demand_metric_spec_map - else None - ) - assert spec_in_use - # It does include the environment tag - assert spec_in_use._query_str_for_hash == f"{expected_str_hash};['environment']" - # This proves that we're picking up the new spec version - assert spec_in_use.spec_version.flags == {"include_environment_tag"} - def test_on_demand_builder_with_not_event_type_error(self) -> None: field = "count()" query = "!event.type:error" @@ -2935,101 +2900,6 @@ def test_on_demand_map_with_multiple_percentiles(self) -> None: }, ] - def _test_user_misery( - self, user_to_frustration: list[tuple[str, bool]], expected_user_misery: float - ) -> None: - threshold = 300 - field = f"user_misery({threshold})" - query_s = "transaction.duration:>=10" - # You can only query this function if you have the feature flag for the org - spec_type = MetricSpecType.SIMPLE_QUERY - spec = OnDemandMetricSpec(field=field, query=query_s, spec_type=spec_type) - - for hour in range(0, 2): - for name, frustrated in user_to_frustration: - tags = ( - {"query_hash": spec.query_hash, "satisfaction": "frustrated"} - if frustrated - else {"query_hash": spec.query_hash} - ) - self.store_transaction_metric( - value=name, - metric=TransactionMetricKey.COUNT_ON_DEMAND.value, - # It's a set on demand because of using the users field - internal_metric=TransactionMRI.SET_ON_DEMAND.value, - entity=EntityKey.MetricsSets.value, - tags=tags, - timestamp=self.start + datetime.timedelta(hours=hour), - ) - - def create_query_builder(): - return TimeseriesMetricQueryBuilder( - self.params, - dataset=Dataset.PerformanceMetrics, - interval=3600, - query=query_s, - selected_columns=[field], - config=QueryBuilderConfig( - on_demand_metrics_enabled=True, on_demand_metrics_type=spec_type - ), - ) - - with Feature({SPEC_VERSION_TWO_FLAG: False}): - # user_misery was added in spec version 2, querying without it results in fallback. - with pytest.raises(IncompatibleMetricsQuery): - create_query_builder() - - with Feature(SPEC_VERSION_TWO_FLAG): - query = create_query_builder() - assert query._on_demand_metric_spec_map - selected_spec = query._on_demand_metric_spec_map[field] - metrics_query = query._get_metrics_query_from_on_demand_spec( - spec=selected_spec, require_time_range=True - ) - - assert len(metrics_query.select) == 1 - assert metrics_query.select[0].op == "on_demand_user_misery" - assert metrics_query.where - assert metrics_query.where[0].lhs.name == "query_hash" - # hashed "on_demand_user_misery:300;{'name': 'event.duration', 'op': 'gte', 'value': 10.0}" - assert metrics_query.where[0].rhs == "f9a20ff3" - assert metrics_query.where[0].rhs == spec.query_hash - - result = query.run_query("test_query") - assert result["data"][:3] == [ - { - "time": self.start.isoformat(), - "user_misery_300": expected_user_misery, - }, - { - "time": (self.start + datetime.timedelta(hours=1)).isoformat(), - "user_misery_300": expected_user_misery, - }, - { - "time": (self.start + datetime.timedelta(hours=2)).isoformat(), - "user_misery_300": 0, - }, - ] - self.assertCountEqual( - result["meta"], - [ - {"name": "time", "type": "DateTime('Universal')"}, - {"name": "user_misery_300", "type": "Float64"}, - ], - ) - - def test_run_query_with_on_demand_user_misery(self) -> None: - self._test_user_misery( - [("happy user", False), ("sad user", True)], - user_misery_formula(1, 2), - ) - - def test_run_query_with_on_demand_user_misery_no_miserable_users(self) -> None: - self._test_user_misery( - [("happy user", False), ("ok user", False)], - user_misery_formula(0, 2), - ) - class HistogramMetricQueryBuilderTest(MetricBuilderBaseTest): def test_histogram_columns_set_on_builder(self) -> None: diff --git a/tests/snuba/api/endpoints/test_organization_events_mep.py b/tests/snuba/api/endpoints/test_organization_events_mep.py index c33c7e72292408..741b737a26de6f 100644 --- a/tests/snuba/api/endpoints/test_organization_events_mep.py +++ b/tests/snuba/api/endpoints/test_organization_events_mep.py @@ -16,17 +16,14 @@ from sentry.search.events import constants from sentry.search.utils import map_device_class_level from sentry.snuba.metrics.extraction import ( - SPEC_VERSION_TWO_FLAG, MetricSpecType, OnDemandMetricSpec, - OnDemandMetricSpecVersioning, ) from sentry.snuba.metrics.naming_layer.mri import TransactionMRI from sentry.snuba.metrics.naming_layer.public import TransactionMetricKey from sentry.snuba.utils import DATASET_OPTIONS from sentry.testutils.cases import MetricsEnhancedPerformanceTestCase from sentry.testutils.helpers.datetime import before_now -from sentry.testutils.helpers.discover import user_misery_formula from sentry.testutils.helpers.on_demand import create_widget from sentry.utils.samples import load_data @@ -3735,137 +3732,6 @@ def test_is_metrics_extracted_data_is_included(self) -> None: response = self._make_on_demand_request(params) self._assert_on_demand_response(response) - def test_on_demand_user_misery(self) -> None: - user_misery_field = "user_misery(300)" - query = "transaction.duration:>=100" - - # We store data for both specs, however, when the query builders try to query - # for the data it will not query on-demand data - for spec_version in OnDemandMetricSpecVersioning.get_spec_versions(): - spec = OnDemandMetricSpec( - field=user_misery_field, - query=query, - spec_type=MetricSpecType.DYNAMIC_QUERY, - # We only allow querying the function in the latest spec version, - # otherwise, the data returned by the endpoint would be 0.05 - spec_version=spec_version, - ) - tags = {"satisfaction": "miserable"} - self.store_on_demand_metric(1, spec=spec, additional_tags=tags, timestamp=self.min_ago) - self.store_on_demand_metric(2, spec=spec, timestamp=self.min_ago) - - params = {"field": [user_misery_field], "project": self.project.id, "query": query} - self._create_specs(params) - - # We expect it to be False because we're not using the extra feature flag - response = self._make_on_demand_request(params) - self._assert_on_demand_response(response, expected_on_demand_query=False) - - # Since we're using the extra feature flag we expect user_misery to be an on-demand metric - response = self._make_on_demand_request(params, {SPEC_VERSION_TWO_FLAG: True}) - self._assert_on_demand_response(response, expected_on_demand_query=True) - assert response.data["data"] == [{user_misery_field: user_misery_formula(1, 2)}] - - def test_on_demand_user_misery_discover_split_with_widget_id_unsaved(self) -> None: - user_misery_field = "user_misery(300)" - query = "transaction.duration:>=100" - - _, widget, __ = create_widget(["count()"], "", self.project, discover_widget_split=None) - - # We store data for both specs, however, when the query builders try to query - # for the data it will not query on-demand data - for spec_version in OnDemandMetricSpecVersioning.get_spec_versions(): - spec = OnDemandMetricSpec( - field=user_misery_field, - query=query, - spec_type=MetricSpecType.DYNAMIC_QUERY, - # We only allow querying the function in the latest spec version, - # otherwise, the data returned by the endpoint would be 0.05 - spec_version=spec_version, - ) - tags = {"satisfaction": "miserable"} - self.store_on_demand_metric(1, spec=spec, additional_tags=tags, timestamp=self.min_ago) - self.store_on_demand_metric(2, spec=spec, timestamp=self.min_ago) - - params = {"field": [user_misery_field], "project": self.project.id, "query": query} - self._create_specs(params) - - params["dashboardWidgetId"] = widget.id - - # Since we're using the extra feature flag we expect user_misery to be an on-demand metric - with mock.patch.object(widget, "save") as mock_widget_save: - response = self._make_on_demand_request(params, {SPEC_VERSION_TWO_FLAG: True}) - assert bool(mock_widget_save.assert_called_once) - - self._assert_on_demand_response(response, expected_on_demand_query=True) - assert response.data["data"] == [{user_misery_field: user_misery_formula(1, 2)}] - - def test_on_demand_user_misery_discover_split_with_widget_id_saved(self) -> None: - user_misery_field = "user_misery(300)" - query = "transaction.duration:>=100" - - _, widget, __ = create_widget( - ["count()"], - "", - self.project, - discover_widget_split=DashboardWidgetTypes.TRANSACTION_LIKE, # Transactions like uses on-demand - ) - - # We store data for both specs, however, when the query builders try to query - # for the data it will not query on-demand data - for spec_version in OnDemandMetricSpecVersioning.get_spec_versions(): - spec = OnDemandMetricSpec( - field=user_misery_field, - query=query, - spec_type=MetricSpecType.DYNAMIC_QUERY, - # We only allow querying the function in the latest spec version, - # otherwise, the data returned by the endpoint would be 0.05 - spec_version=spec_version, - ) - tags = {"satisfaction": "miserable"} - self.store_on_demand_metric(1, spec=spec, additional_tags=tags, timestamp=self.min_ago) - self.store_on_demand_metric(2, spec=spec, timestamp=self.min_ago) - - params = {"field": [user_misery_field], "project": self.project.id, "query": query} - self._create_specs(params) - - params["dashboardWidgetId"] = widget.id - - # Since we're using the extra feature flag we expect user_misery to be an on-demand metric - with mock.patch.object(widget, "save") as mock_widget_save: - response = self._make_on_demand_request(params, {SPEC_VERSION_TWO_FLAG: True}) - assert bool(mock_widget_save.assert_not_called) - - self._assert_on_demand_response(response, expected_on_demand_query=True) - assert response.data["data"] == [{user_misery_field: user_misery_formula(1, 2)}] - - def test_on_demand_count_unique(self) -> None: - field = "count_unique(user)" - query = "transaction.duration:>0" - params = {"field": [field], "query": query} - # We do not really have to create the metrics for both specs since - # the first API call will not query any on-demand metric - for spec_version in OnDemandMetricSpecVersioning.get_spec_versions(): - spec = OnDemandMetricSpec( - field=field, - query=query, - spec_type=MetricSpecType.DYNAMIC_QUERY, - spec_version=spec_version, - ) - self.store_on_demand_metric(1, spec=spec, timestamp=self.min_ago) - self.store_on_demand_metric(2, spec=spec, timestamp=self.min_ago) - - # The first call will not be on-demand - response = self._make_on_demand_request(params) - self._assert_on_demand_response(response, expected_on_demand_query=False) - - # This second call will be on-demand - response = self._make_on_demand_request( - params, extra_features={SPEC_VERSION_TWO_FLAG: True} - ) - self._assert_on_demand_response(response, expected_on_demand_query=True) - assert response.data["data"] == [{"count_unique(user)": 2}] - def test_on_demand_for_metrics_deprecation(self) -> None: params = {"field": ["count()"], "query": "", "yAxis": "count()"} specs = self._create_specs(params)