From dc19d400b481cac81105aaa5447e16642a2c044d Mon Sep 17 00:00:00 2001 From: Nar Saynorath Date: Wed, 24 Jul 2024 10:25:37 -0400 Subject: [PATCH] feat(dashboards): Default widget split decision to errors (#74723) In the case that neither side has data, or both sides have data, force the dataset to errors and record the reason in the source field. This way we can ensure the split occurs and also warn the user that they may need to select a dataset. Note: To avoid changing behaviour for existing on-demand users by forcing the split to use errors, I feature flagged the default scenario so without the feature flag the default is still discover. --- src/sentry/api/bases/organization_events.py | 42 +++++++++++-------- .../api/endpoints/organization_events.py | 6 ++- .../endpoints/organization_events_stats.py | 4 +- .../endpoints/test_organization_events_mep.py | 23 ++++++---- .../test_organization_events_stats_mep.py | 15 +++++-- 5 files changed, 58 insertions(+), 32 deletions(-) diff --git a/src/sentry/api/bases/organization_events.py b/src/sentry/api/bases/organization_events.py index ee9302e6c304c4..b2608373a9a7ce 100644 --- a/src/sentry/api/bases/organization_events.py +++ b/src/sentry/api/bases/organization_events.py @@ -240,33 +240,39 @@ def handle_on_demand(self, request: Request) -> tuple[bool, MetricSpecType]: return use_on_demand_metrics, on_demand_metric_type - def get_split_decision(self, has_errors, has_transactions_data): + def save_split_decision(self, widget, has_errors, has_transactions_data, organization, user): """This can be removed once the discover dataset has been fully split""" + source = DashboardDatasetSourcesTypes.INFERRED.value if has_errors and not has_transactions_data: decision = DashboardWidgetTypes.ERROR_EVENTS + sentry_sdk.set_tag("discover.split_reason", "query_result") elif not has_errors and has_transactions_data: decision = DashboardWidgetTypes.TRANSACTION_LIKE - elif has_errors and has_transactions_data: - decision = DashboardWidgetTypes.DISCOVER + sentry_sdk.set_tag("discover.split_reason", "query_result") else: - # In the case that neither side has data, we do not need to split this yet and can make multiple queries to check each time. - # This will help newly created widgets or infrequent count widgets that shouldn't be prematurely assigned a side. - decision = None - sentry_sdk.set_tag("split_decision", decision) - return decision + if features.has( + "organizations:performance-discover-dataset-selector", organization, actor=user + ): + # In the case that neither side has data, or both sides have data, default to errors. + decision = DashboardWidgetTypes.ERROR_EVENTS + source = DashboardDatasetSourcesTypes.FORCED.value + sentry_sdk.set_tag("discover.split_reason", "default") + else: + # This branch can be deleted once the feature flag for the discover split is removed + if has_errors and has_transactions_data: + decision = DashboardWidgetTypes.DISCOVER + else: + # In the case that neither side has data, we do not need to split this yet and can make multiple queries to check each time. + # This will help newly created widgets or infrequent count widgets that shouldn't be prematurely assigned a side. + decision = None - def save_split_decision(self, widget, has_errors, has_transactions_data): - """This can be removed once the discover dataset has been fully split""" - new_discover_widget_split = self.get_split_decision(has_errors, has_transactions_data) - if ( - new_discover_widget_split is not None - and widget.discover_widget_split != new_discover_widget_split - ): - widget.discover_widget_split = new_discover_widget_split - widget.dataset_source = DashboardDatasetSourcesTypes.INFERRED.value + sentry_sdk.set_tag("discover.split_decision", decision) + if decision is not None and widget.discover_widget_split != decision: + widget.discover_widget_split = decision + widget.dataset_source = source widget.save() - return new_discover_widget_split + return decision def save_discover_saved_query_split_decision( self, query, dataset_inferred_from_query, has_errors, has_transactions_data diff --git a/src/sentry/api/endpoints/organization_events.py b/src/sentry/api/endpoints/organization_events.py index 6b14bbe8c1fe6e..c997f1021bd354 100644 --- a/src/sentry/api/endpoints/organization_events.py +++ b/src/sentry/api/endpoints/organization_events.py @@ -438,7 +438,7 @@ def _dashboards_data_fn(scoped_dataset, offset, limit, scoped_query, dashboard_w error_results = None original_results = _data_fn(scoped_dataset, offset, limit, scoped_query) - if original_results.get("data"): + if original_results.get("data") is not None: dataset_meta = original_results.get("meta", {}) else: dataset_meta = list(original_results.values())[0].get("data").get("meta", {}) @@ -460,7 +460,9 @@ def _dashboards_data_fn(scoped_dataset, offset, limit, scoped_query, dashboard_w transaction_results = _data_fn(discover, offset, limit, transactions_only_query) has_transactions = len(transaction_results["data"]) > 0 - decision = self.save_split_decision(widget, has_errors, has_transactions) + decision = self.save_split_decision( + widget, has_errors, has_transactions, organization, request.user + ) if decision == DashboardWidgetTypes.DISCOVER: return _data_fn(discover, offset, limit, scoped_query) diff --git a/src/sentry/api/endpoints/organization_events_stats.py b/src/sentry/api/endpoints/organization_events_stats.py index 5fb5fb247e3dc2..a9af5af442fb0f 100644 --- a/src/sentry/api/endpoints/organization_events_stats.py +++ b/src/sentry/api/endpoints/organization_events_stats.py @@ -421,7 +421,9 @@ def fn( ) has_transactions = self.check_if_results_have_data(transaction_results) - decision = self.save_split_decision(widget, has_errors, has_transactions) + decision = self.save_split_decision( + widget, has_errors, has_transactions, organization, request.user + ) if decision == DashboardWidgetTypes.DISCOVER: # The user needs to be warned to split in this case. diff --git a/tests/snuba/api/endpoints/test_organization_events_mep.py b/tests/snuba/api/endpoints/test_organization_events_mep.py index 398856c353d4aa..8e644620b2a776 100644 --- a/tests/snuba/api/endpoints/test_organization_events_mep.py +++ b/tests/snuba/api/endpoints/test_organization_events_mep.py @@ -3725,20 +3725,23 @@ def test_split_decision_for_ambiguous_widget_without_data(self): response = self.do_request( { - "field": ["count()", "transaction.name", "error.type"], + "field": ["count()", "transaction.op", "error.type"], "query": "", "dataset": "metricsEnhanced", "per_page": 50, "dashboardWidgetId": widget.id, - } + }, + features={"organizations:performance-discover-dataset-selector": True}, ) assert response.status_code == 200, response.content - assert response.data.get("meta").get("discoverSplitDecision") is None + assert response.data.get("meta").get( + "discoverSplitDecision" + ) == DashboardWidgetTypes.get_type_name(DashboardWidgetTypes.ERROR_EVENTS) widget.refresh_from_db() - assert widget.discover_widget_split is None - assert widget.dataset_source == DatasetSourcesTypes.UNKNOWN.value + assert widget.discover_widget_split == DashboardWidgetTypes.ERROR_EVENTS + assert widget.dataset_source == DatasetSourcesTypes.FORCED.value def test_split_decision_for_ambiguous_widget_with_data(self): # Store a transaction @@ -3775,14 +3778,18 @@ def test_split_decision_for_ambiguous_widget_with_data(self): "dataset": "metricsEnhanced", "per_page": 50, "dashboardWidgetId": widget.id, - } + }, + features={"organizations:performance-discover-dataset-selector": True}, ) assert response.status_code == 200, response.content - assert response.data.get("meta").get("discoverSplitDecision") is None + assert response.data.get("meta").get( + "discoverSplitDecision" + ) == DashboardWidgetTypes.get_type_name(DashboardWidgetTypes.ERROR_EVENTS) widget.refresh_from_db() - assert widget.discover_widget_split is DashboardWidgetTypes.DISCOVER + assert widget.discover_widget_split == DashboardWidgetTypes.ERROR_EVENTS + assert widget.dataset_source == DatasetSourcesTypes.FORCED.value class OrganizationEventsMetricsEnhancedPerformanceEndpointTestWithMetricLayer( diff --git a/tests/snuba/api/endpoints/test_organization_events_stats_mep.py b/tests/snuba/api/endpoints/test_organization_events_stats_mep.py index 2aab9c89017ce6..048cf1142a9a9c 100644 --- a/tests/snuba/api/endpoints/test_organization_events_stats_mep.py +++ b/tests/snuba/api/endpoints/test_organization_events_stats_mep.py @@ -8,6 +8,7 @@ from django.urls import reverse from rest_framework.response import Response +from sentry.discover.models import DatasetSourcesTypes from sentry.models.dashboard_widget import DashboardWidget, DashboardWidgetTypes from sentry.models.environment import Environment from sentry.sentry_metrics.use_case_id_registry import UseCaseID @@ -791,6 +792,7 @@ def test_split_decision_for_errors_widget(self): widget.refresh_from_db() assert widget.discover_widget_split == DashboardWidgetTypes.ERROR_EVENTS + assert widget.dataset_source == DatasetSourcesTypes.INFERRED.value def test_split_decision_for_transactions_widget(self): self.store_transaction_metric( @@ -820,6 +822,7 @@ def test_split_decision_for_transactions_widget(self): widget.refresh_from_db() assert widget.discover_widget_split == DashboardWidgetTypes.TRANSACTION_LIKE + assert widget.dataset_source == DatasetSourcesTypes.INFERRED.value def test_split_decision_for_top_events_errors_widget(self): error_data = load_data("python", timestamp=before_now(minutes=1)) @@ -860,6 +863,7 @@ def test_split_decision_for_top_events_errors_widget(self): widget.refresh_from_db() assert widget.discover_widget_split == DashboardWidgetTypes.ERROR_EVENTS + assert widget.dataset_source == DatasetSourcesTypes.INFERRED.value def test_split_decision_for_top_events_transactions_widget(self): self.store_transaction_metric( @@ -896,6 +900,7 @@ def test_split_decision_for_top_events_transactions_widget(self): widget.refresh_from_db() assert widget.discover_widget_split == DashboardWidgetTypes.TRANSACTION_LIKE + assert widget.dataset_source == DatasetSourcesTypes.INFERRED.value def test_split_decision_for_ambiguous_widget_without_data(self): _, widget, __ = create_widget( @@ -913,14 +918,18 @@ def test_split_decision_for_ambiguous_widget_without_data(self): "dataset": "metricsEnhanced", "per_page": 50, "dashboardWidgetId": widget.id, - } + }, + features={"organizations:performance-discover-dataset-selector": True}, ) assert response.status_code == 200, response.content - assert response.data.get("meta").get("discoverSplitDecision") is None + assert response.data.get("meta").get( + "discoverSplitDecision" + ) == DashboardWidgetTypes.get_type_name(DashboardWidgetTypes.ERROR_EVENTS) widget.refresh_from_db() - assert widget.discover_widget_split is None + assert widget.discover_widget_split == DashboardWidgetTypes.ERROR_EVENTS + assert widget.dataset_source == DatasetSourcesTypes.FORCED.value class OrganizationEventsStatsMetricsEnhancedPerformanceEndpointTestWithMetricLayer(