Skip to content

feat(on-demand): Add feature flag check before cardinality check #98726

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

Merged
merged 2 commits into from
Sep 15, 2025
Merged

feat(on-demand): Add feature flag check before cardinality check #98726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e6ccf6
feat(on-demand): Add feature flag check before cardinality check
narsaynorath Sep 3, 2025
d1ba9ef
Copy old test for coverage with feature flag
narsaynorath Sep 15, 2025
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
5 changes: 4 additions & 1 deletion src/sentry/tasks/on_demand_metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from celery.exceptions import SoftTimeLimitExceeded
from django.utils import timezone

from sentry import options
from sentry import features, options
from sentry.models.dashboard_widget import (
DashboardWidgetQuery,
DashboardWidgetQueryOnDemand,
Expand Down Expand Up @@ -423,6 +423,9 @@ def check_field_cardinality(
is_task: bool = False,
widget_query: DashboardWidgetQuery | None = None,
) -> dict[str, str]:
if not features.has("organizations:on-demand-metrics-extraction-widgets", organization):
Copy link
Member Author

@narsaynorath narsaynorath Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@k-fish would you be able to advise if this is okay for us to do? I suppose when we migrate fully to EAP then we can get rid of this, but in an effort to make some easy progress for other users, I figured we could just feature flag this check

It seems like we wanted to evaluate it even without the feature flag so when we rolled out, people's queries were good to go

Copy link
Member

@k-fish k-fish Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can get rid of the flag if we confirm if any of the orgs are not still on am1 / their widgets work on EAP

In theory yes, we were supposed to check so that we could roll it out for every org, but am3 came along. We're okay skipping it for extraction widgets.

Can you copy the test with the flag to assert the old behaviour? otherwise it's fine I think

return {}

if not query_columns:
return {}
if is_task:
Expand Down
48 changes: 46 additions & 2 deletions tests/sentry/dashboards/endpoints/test_organization_dashboard_details.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3132,7 +3132,7 @@ def test_ondemand_updates_new_widget(self, mock_query: mock.MagicMock) -> None:
assert current_version.extraction_state == "enabled:creation"

@mock.patch("sentry.tasks.on_demand_metrics._query_cardinality")
def test_cardinality_precedence_over_feature_checks(self, mock_query: mock.MagicMock) -> None:
def test_cardinality_check_with_feature_flag(self, mock_query: mock.MagicMock) -> None:
mock_query.return_value = {"data": [{"count_unique(sometag)": 1_000_000}]}, [
"sometag",
]
Expand All @@ -3156,7 +3156,9 @@ def test_cardinality_precedence_over_feature_checks(self, mock_query: mock.Magic
},
],
}
response = self.do_request("put", self.url(self.dashboard.id), data=data)

with self.feature(["organizations:on-demand-metrics-extraction-widgets"]):
response = self.do_request("put", self.url(self.dashboard.id), data=data)
assert response.status_code == 200, response.data
widgets = self.get_widgets(self.dashboard.id)
assert len(widgets) == 1
Expand All @@ -3171,6 +3173,48 @@ def test_cardinality_precedence_over_feature_checks(self, mock_query: mock.Magic
assert current_version is not None
assert current_version.extraction_state == "disabled:high-cardinality"

@mock.patch("sentry.tasks.on_demand_metrics._query_cardinality")
def test_feature_check_takes_precedence_over_cardinality(
self, mock_query: mock.MagicMock
) -> None:
mock_query.return_value = {"data": [{"count_unique(sometag)": 1_000_000}]}, [
"sometag",
]
data: dict[str, Any] = {
"title": "first dashboard",
"widgets": [
{
"title": "errors per project",
"displayType": "table",
"interval": "5m",
"widgetType": DashboardWidgetTypes.get_type_name(self.widget_type),
"queries": [
{
"name": "errors",
"fields": ["count()", "sometag"],
"columns": ["sometag"],
"aggregates": ["count()"],
"conditions": "event.type:transaction",
}
],
},
],
}
response = self.do_request("put", self.url(self.dashboard.id), data=data)
assert response.status_code == 200, response.data
widgets = self.get_widgets(self.dashboard.id)
assert len(widgets) == 1
last = list(widgets).pop()
queries = last.dashboardwidgetquery_set.all()

ondemand_objects = DashboardWidgetQueryOnDemand.objects.filter(
dashboard_widget_query=queries[0]
)
for version in OnDemandMetricSpecVersioning.get_spec_versions():
current_version = ondemand_objects.filter(spec_version=version.version).first()
assert current_version is not None
assert current_version.extraction_state == "disabled:pre-rollout"

@mock.patch("sentry.api.serializers.rest_framework.dashboard.get_current_widget_specs")
def test_cardinality_skips_non_discover_widget_types(
self, mock_get_specs: mock.MagicMock
Expand Down
Loading