From 8d4ef7acad04ab9a4e32d5409b27cc4964b12767 Mon Sep 17 00:00:00 2001 From: George Gritsouk <989898+gggritso@users.noreply.github.com> Date: Thu, 16 Jan 2025 09:04:43 -0500 Subject: [PATCH] feat(visibility): Return meta with top N timeseries results (#83446) Right now, a timeseries request with a `groupBy` does not attach any `"meta"` to the response! That's a bummer because for the new standard widgets, we rely on that meta to figure out types and units. This PR adds the meta to the responses, including support for the `transformAliasToInputFormat` URL parameter. Note that a `groupBy` splits the results by group but the _meta_ is the same everywhere. This is why I attach `results["meta"]` to the response directly without having to split anything up. ## Changes - Attach meta to results - Pass through alias input parameter --------- Co-authored-by: Tony Xiao --- .../endpoints/organization_events_stats.py | 1 + src/sentry/snuba/discover.py | 5 ++ src/sentry/snuba/errors.py | 6 ++ src/sentry/snuba/functions.py | 4 +- .../snuba/metrics_enhanced_performance.py | 3 + src/sentry/snuba/metrics_performance.py | 11 +++- src/sentry/snuba/spans_eap.py | 3 + src/sentry/snuba/spans_indexed.py | 3 + src/sentry/snuba/spans_metrics.py | 3 + src/sentry/snuba/transactions.py | 2 + .../test_organization_events_stats.py | 65 +++++++++++++++++++ .../test_organization_events_stats_mep.py | 24 ++++--- 12 files changed, 118 insertions(+), 12 deletions(-) diff --git a/src/sentry/api/endpoints/organization_events_stats.py b/src/sentry/api/endpoints/organization_events_stats.py index a52ce6e5b12c69..5bc1feb81a707e 100644 --- a/src/sentry/api/endpoints/organization_events_stats.py +++ b/src/sentry/api/endpoints/organization_events_stats.py @@ -321,6 +321,7 @@ def _get_event_stats( on_demand_metrics_type=on_demand_metrics_type, include_other=include_other, query_source=query_source, + transform_alias_to_input_format=transform_alias_to_input_format, fallback_to_transactions=features.has( "organizations:performance-discover-dataset-selector", organization, diff --git a/src/sentry/snuba/discover.py b/src/sentry/snuba/discover.py index 42e31b128ab73f..a0f876d74f1233 100644 --- a/src/sentry/snuba/discover.py +++ b/src/sentry/snuba/discover.py @@ -440,6 +440,7 @@ def top_events_timeseries( dataset: Dataset = Dataset.Discover, query_source: QuerySource | None = None, fallback_to_transactions: bool = False, + transform_alias_to_input_format: bool = False, ) -> dict[str, SnubaTSResult] | SnubaTSResult: """ High-level API for doing arbitrary user timeseries queries for a limited number of top events @@ -462,6 +463,8 @@ def top_events_timeseries( top_events - A dictionary with a 'data' key containing a list of dictionaries that represent the top events matching the query. Useful when you have found the top events earlier and want to save a query. + transform_alias_to_input_format - Whether aggregate columns should be returned in the originally + requested function format. """ assert dataset in [ Dataset.Discover, @@ -500,6 +503,7 @@ def top_events_timeseries( config=QueryBuilderConfig( functions_acl=functions_acl, skip_tag_resolution=True, + transform_alias_to_input_format=transform_alias_to_input_format, ), ) if len(top_events["data"]) == limit and include_other: @@ -587,6 +591,7 @@ def top_events_timeseries( if zerofill_results else item["data"] ), + "meta": result["meta"], "order": item["order"], }, snuba_params.start_date, diff --git a/src/sentry/snuba/errors.py b/src/sentry/snuba/errors.py index b1dde08afa19c5..fec34d0a35e235 100644 --- a/src/sentry/snuba/errors.py +++ b/src/sentry/snuba/errors.py @@ -215,6 +215,7 @@ def top_events_timeseries( dataset: Dataset = Dataset.Discover, query_source: QuerySource | None = None, fallback_to_transactions: bool = False, + transform_alias_to_input_format: bool = False, ) -> dict[str, SnubaTSResult] | SnubaTSResult: """ High-level API for doing arbitrary user timeseries queries for a limited number of top events @@ -237,6 +238,9 @@ def top_events_timeseries( top_events - A dictionary with a 'data' key containing a list of dictionaries that represent the top events matching the query. Useful when you have found the top events earlier and want to save a query. + transform_alias_to_input_format - Whether aggregate columns should be returned in the originally + requested function format. + """ if top_events is None: with sentry_sdk.start_span(op="discover.errors", name="top_events.fetch_events"): @@ -269,6 +273,7 @@ def top_events_timeseries( config=QueryBuilderConfig( functions_acl=functions_acl, skip_tag_resolution=True, + transform_alias_to_input_format=transform_alias_to_input_format, ), ) if len(top_events["data"]) == limit and include_other: @@ -356,6 +361,7 @@ def top_events_timeseries( if zerofill_results else item["data"] ), + "meta": result["meta"], "order": item["order"], }, snuba_params.start_date, diff --git a/src/sentry/snuba/functions.py b/src/sentry/snuba/functions.py index c5641706f9f449..22e476c632fa21 100644 --- a/src/sentry/snuba/functions.py +++ b/src/sentry/snuba/functions.py @@ -151,6 +151,7 @@ def top_events_timeseries( on_demand_metrics_type: MetricSpecType | None = None, query_source: QuerySource | None = None, fallback_to_transactions: bool = False, + transform_alias_to_input_format: bool = False, ): assert not include_other, "Other is not supported" # TODO: support other @@ -183,6 +184,7 @@ def top_events_timeseries( config=QueryBuilderConfig( functions_acl=functions_acl, skip_tag_resolution=True, + transform_alias_to_input_format=transform_alias_to_input_format, ), ) @@ -270,7 +272,7 @@ def format_top_events_timeseries_results( else item["data"] ), "order": item["order"], - "meta": result["meta"], + "meta": processed_result["meta"], }, snuba_params.start_date, snuba_params.end_date, diff --git a/src/sentry/snuba/metrics_enhanced_performance.py b/src/sentry/snuba/metrics_enhanced_performance.py index 0fcdd654a2f19c..637e1cc37c4b94 100644 --- a/src/sentry/snuba/metrics_enhanced_performance.py +++ b/src/sentry/snuba/metrics_enhanced_performance.py @@ -235,6 +235,7 @@ def top_events_timeseries( on_demand_metrics_type: MetricSpecType | None = None, query_source: QuerySource | None = None, fallback_to_transactions: bool = False, + transform_alias_to_input_format: bool = False, ) -> SnubaTSResult | dict[str, Any]: metrics_compatible = False equations, _ = categorize_columns(selected_columns) @@ -262,6 +263,7 @@ def top_events_timeseries( on_demand_metrics_enabled=on_demand_metrics_enabled, on_demand_metrics_type=on_demand_metrics_type, query_source=query_source, + transform_alias_to_input_format=transform_alias_to_input_format, ) # raise Invalid Queries since the same thing will happen with discover except InvalidSearchQuery: @@ -296,6 +298,7 @@ def top_events_timeseries( include_other, functions_acl, query_source=query_source, + transform_alias_to_input_format=transform_alias_to_input_format, ) return SnubaTSResult( { diff --git a/src/sentry/snuba/metrics_performance.py b/src/sentry/snuba/metrics_performance.py index 2acd67232bc7bd..1a7e79ebc6255b 100644 --- a/src/sentry/snuba/metrics_performance.py +++ b/src/sentry/snuba/metrics_performance.py @@ -413,6 +413,7 @@ def top_events_timeseries( on_demand_metrics_type: MetricSpecType | None = None, query_source: QuerySource | None = None, fallback_to_transactions: bool = False, + transform_alias_to_input_format: bool = False, ) -> SnubaTSResult | dict[str, Any]: if top_events is None: @@ -445,6 +446,7 @@ def top_events_timeseries( functions_acl=functions_acl, on_demand_metrics_enabled=on_demand_metrics_enabled, on_demand_metrics_type=on_demand_metrics_type, + transform_alias_to_input_format=transform_alias_to_input_format, ), ) if len(top_events["data"]) == limit and include_other: @@ -527,9 +529,12 @@ def top_events_timeseries( else item["data"] ), "order": item["order"], - # One of the queries in the builder has required, thus, we mark all of them - # This could mislead downstream consumers of the meta data - "meta": {"isMetricsExtractedData": top_events_builder.use_on_demand}, + "meta": { + # One of the queries in the builder has required, thus, we mark all of them + # This could mislead downstream consumers of the meta data + "isMetricsExtractedData": top_events_builder.use_on_demand, + **result["meta"], + }, }, snuba_params.start_date, snuba_params.end_date, diff --git a/src/sentry/snuba/spans_eap.py b/src/sentry/snuba/spans_eap.py index 35ebede9dc6d7e..fff4317e8e934b 100644 --- a/src/sentry/snuba/spans_eap.py +++ b/src/sentry/snuba/spans_eap.py @@ -165,6 +165,7 @@ def top_events_timeseries( dataset: Dataset = Dataset.Discover, query_source: QuerySource | None = None, fallback_to_transactions: bool = False, + transform_alias_to_input_format: bool = False, ): """ High-level API for doing arbitrary user timeseries queries for a limited number of top events @@ -202,6 +203,7 @@ def top_events_timeseries( config=QueryBuilderConfig( functions_acl=functions_acl, skip_tag_resolution=True, + transform_alias_to_input_format=transform_alias_to_input_format, ), ) if len(top_events["data"]) == limit and include_other: @@ -286,6 +288,7 @@ def top_events_timeseries( if zerofill_results else result_item["data"] ), + "meta": result["meta"], "order": result_item["order"], }, snuba_params.start_date, diff --git a/src/sentry/snuba/spans_indexed.py b/src/sentry/snuba/spans_indexed.py index 2ef001caecbd67..4e09e747ddcf7d 100644 --- a/src/sentry/snuba/spans_indexed.py +++ b/src/sentry/snuba/spans_indexed.py @@ -159,6 +159,7 @@ def top_events_timeseries( on_demand_metrics_type: MetricSpecType | None = None, query_source: QuerySource | None = None, fallback_to_transactions: bool = False, + transform_alias_to_input_format: bool = False, ): """ High-level API for doing arbitrary user timeseries queries for a limited number of top events @@ -197,6 +198,7 @@ def top_events_timeseries( config=QueryBuilderConfig( functions_acl=functions_acl, skip_tag_resolution=True, + transform_alias_to_input_format=transform_alias_to_input_format, ), ) if len(top_events["data"]) == limit and include_other: @@ -279,6 +281,7 @@ def top_events_timeseries( if zerofill_results else item["data"] ), + "meta": result["meta"], "order": item["order"], }, snuba_params.start_date, diff --git a/src/sentry/snuba/spans_metrics.py b/src/sentry/snuba/spans_metrics.py index d2f41948938910..12467575b97de2 100644 --- a/src/sentry/snuba/spans_metrics.py +++ b/src/sentry/snuba/spans_metrics.py @@ -164,6 +164,7 @@ def top_events_timeseries( on_demand_metrics_type: MetricSpecType | None = None, query_source: QuerySource | None = None, fallback_to_transactions: bool = False, + transform_alias_to_input_format: bool = False, ): """ High-level API for doing arbitrary user timeseries queries for a limited number of top events @@ -217,6 +218,7 @@ def top_events_timeseries( config=QueryBuilderConfig( functions_acl=functions_acl, skip_tag_resolution=True, + transform_alias_to_input_format=transform_alias_to_input_format, ), ) if len(top_events["data"]) == limit and include_other: @@ -290,6 +292,7 @@ def top_events_timeseries( if zerofill_results else item["data"] ), + "meta": result["meta"], "order": item["order"], }, snuba_params.start_date, diff --git a/src/sentry/snuba/transactions.py b/src/sentry/snuba/transactions.py index 2f646b25a014c0..2f79b36508715f 100644 --- a/src/sentry/snuba/transactions.py +++ b/src/sentry/snuba/transactions.py @@ -135,6 +135,7 @@ def top_events_timeseries( on_demand_metrics_type: MetricSpecType | None = None, query_source: QuerySource | None = None, fallback_to_transactions: bool = False, + transform_alias_to_input_format: bool = False, ) -> dict[str, SnubaTSResult] | SnubaTSResult: return discover.top_events_timeseries( timeseries_columns, @@ -156,6 +157,7 @@ def top_events_timeseries( on_demand_metrics_type=on_demand_metrics_type, dataset=Dataset.Transactions, query_source=query_source, + transform_alias_to_input_format=transform_alias_to_input_format, ) diff --git a/tests/snuba/api/endpoints/test_organization_events_stats.py b/tests/snuba/api/endpoints/test_organization_events_stats.py index 535a185a6a4f70..b958af9cf145b9 100644 --- a/tests/snuba/api/endpoints/test_organization_events_stats.py +++ b/tests/snuba/api/endpoints/test_organization_events_stats.py @@ -1463,6 +1463,71 @@ def test_simple_top_events(self): assert other["order"] == 5 assert [{"count": 3}] in [attrs for _, attrs in other["data"]] + def test_simple_top_events_meta(self): + with self.feature(self.enabled_features): + response = self.client.get( + self.url, + data={ + "start": self.day_ago.isoformat(), + "end": (self.day_ago + timedelta(hours=2)).isoformat(), + "interval": "1h", + "yAxis": "sum(transaction.duration)", + "orderby": ["-sum(transaction.duration)"], + "field": ["transaction", "sum(transaction.duration)"], + "topEvents": "5", + }, + format="json", + ) + + data = response.data + assert response.status_code == 200, response.content + + for transaction, transaction_data in data.items(): + assert transaction_data["meta"]["fields"] == { + "time": "date", + "transaction": "string", + "sum_transaction_duration": "duration", + } + + assert transaction_data["meta"]["units"] == { + "time": None, + "transaction": None, + "sum_transaction_duration": "millisecond", + } + + def test_simple_top_events_meta_no_alias(self): + with self.feature(self.enabled_features): + response = self.client.get( + self.url, + data={ + "transformAliasToInputFormat": "1", + "start": self.day_ago.isoformat(), + "end": (self.day_ago + timedelta(hours=2)).isoformat(), + "interval": "1h", + "yAxis": "sum(transaction.duration)", + "orderby": ["-sum(transaction.duration)"], + "field": ["transaction", "sum(transaction.duration)"], + "topEvents": "5", + }, + format="json", + ) + + data = response.data + assert response.status_code == 200, response.content + + for transaction, transaction_data in data.items(): + assert transaction_data["meta"]["fields"] == { + "time": "date", + "transaction": "string", + "sum(transaction.duration)": "duration", + } + + assert transaction_data["meta"]["units"] == { + "time": None, + "transaction": None, + "sum(transaction.duration)": "millisecond", + } + def test_top_events_with_projects_other(self): with self.feature(self.enabled_features): response = self.client.get( 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 128bf673654bdd..20ae8c2f4af072 100644 --- a/tests/snuba/api/endpoints/test_organization_events_stats_mep.py +++ b/tests/snuba/api/endpoints/test_organization_events_stats_mep.py @@ -1996,11 +1996,11 @@ def test_glob_http_referer_on_demand(self): assert datum["meta"] == { "dataset": "metricsEnhanced", "datasetReason": "unchanged", - "fields": {}, + "fields": {"count": "integer", "networkId": "string", "time": "date"}, "isMetricsData": False, "isMetricsExtractedData": True, "tips": {}, - "units": {}, + "units": {"count": None, "networkId": None, "time": None}, } def _test_is_metrics_extracted_data( @@ -2184,11 +2184,11 @@ def test_order_by_aggregate_top_events_desc(self): assert datum["meta"] == { "dataset": "metricsEnhanced", "datasetReason": "unchanged", - "fields": {}, + "fields": {"count": "integer", "networkId": "string", "time": "date"}, "isMetricsData": False, "isMetricsExtractedData": True, "tips": {}, - "units": {}, + "units": {"count": None, "networkId": None, "time": None}, } def test_order_by_aggregate_top_events_asc(self): @@ -2226,11 +2226,11 @@ def test_order_by_aggregate_top_events_asc(self): assert datum["meta"] == { "dataset": "metricsEnhanced", "datasetReason": "unchanged", - "fields": {}, + "fields": {"count": "integer", "networkId": "string", "time": "date"}, "isMetricsData": False, "isMetricsExtractedData": True, "tips": {}, - "units": {}, + "units": {"count": None, "networkId": None, "time": None}, } def test_order_by_aggregate_top_events_graph_different_aggregate(self): @@ -2268,11 +2268,19 @@ def test_order_by_aggregate_top_events_graph_different_aggregate(self): assert datum["meta"] == { "dataset": "metricsEnhanced", "datasetReason": "unchanged", - "fields": {}, + "fields": { + "networkId": "string", + "p95_transaction_duration": "duration", + "time": "date", + }, "isMetricsData": False, "isMetricsExtractedData": True, "tips": {}, - "units": {}, + "units": { + "networkId": None, + "p95_transaction_duration": "millisecond", + "time": None, + }, } def test_cannot_order_by_tag(self):