Skip to content

ref(flags): Remove organizations:on-demand-metrics-query-spec-version-two #114808

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wedamija wants to merge 1 commit into
base: master
Choose a base branch
from
+9 −328
Open

ref(flags): Remove organizations:on-demand-metrics-query-spec-version-two #114808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/sentry/features/temporary.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
33 changes: 4 additions & 29 deletions src/sentry/snuba/metrics/extraction.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 (
Expand Down
37 changes: 4 additions & 33 deletions tests/sentry/relay/config/test_metric_extraction.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}",
Expand Down Expand Up @@ -2109,29 +2104,20 @@ 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"
assert spec._query_str_for_hash == expected_query_str_hash
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})
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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}",
Expand Down
132 changes: 1 addition & 131 deletions tests/sentry/search/events/builder/test_metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,16 @@
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,
)
from sentry.snuba.metrics.naming_layer import TransactionMetricKey
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

Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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:
Expand Down
Loading
Loading