From 980bdeb718cf4426a76d55d06ece786385cd1bee Mon Sep 17 00:00:00 2001 From: Edward Gou Date: Mon, 30 Dec 2024 19:54:08 -0500 Subject: [PATCH 1/3] Updates the performance score function to account for missing webvitals, instead of just considering them to be zero --- src/sentry/search/events/datasets/metrics.py | 71 +++++++++++++++++-- .../endpoints/test_organization_events_mep.py | 47 ++++++++++++ 2 files changed, 112 insertions(+), 6 deletions(-) diff --git a/src/sentry/search/events/datasets/metrics.py b/src/sentry/search/events/datasets/metrics.py index ee851f009946de..323cae81c31163 100644 --- a/src/sentry/search/events/datasets/metrics.py +++ b/src/sentry/search/events/datasets/metrics.py @@ -1848,9 +1848,38 @@ def _resolve_total_performance_score_function( for vital in vitals } + weights = { + vital: Function( + "if", + [ + Function( + "isZeroOrNull", + [ + Function( + "countIf", + [ + Column("value"), + Function( + "equals", + [ + Column("metric_id"), + self.resolve_metric(f"measurements.score.{vital}"), + ], + ), + ], + ), + ], + ), + 0, + constants.WEB_VITALS_PERFORMANCE_SCORE_WEIGHTS[vital], + ], + ) + for vital in vitals + } + # TODO: Is there a way to sum more than 2 values at once? return Function( - "plus", + "divide", [ Function( "plus", @@ -1861,17 +1890,47 @@ def _resolve_total_performance_score_function( Function( "plus", [ - scores["lcp"], - scores["fcp"], + Function( + "plus", + [ + scores["lcp"], + scores["fcp"], + ], + ), + scores["cls"], + ], + ), + scores["ttfb"], + ], + ), + scores["inp"], + ], + ), + Function( + "plus", + [ + Function( + "plus", + [ + Function( + "plus", + [ + Function( + "plus", + [ + weights["lcp"], + weights["fcp"], + ], + ), + weights["cls"], ], ), - scores["cls"], + weights["ttfb"], ], ), - scores["ttfb"], + weights["inp"], ], ), - scores["inp"], ], alias, ) diff --git a/tests/snuba/api/endpoints/test_organization_events_mep.py b/tests/snuba/api/endpoints/test_organization_events_mep.py index c87441e06a22dc..d3dd9aca6a77b9 100644 --- a/tests/snuba/api/endpoints/test_organization_events_mep.py +++ b/tests/snuba/api/endpoints/test_organization_events_mep.py @@ -3100,6 +3100,49 @@ def test_total_performance_score(self): assert data[0]["performance_score(measurements.score.total)"] == 0.48 assert meta["isMetricsData"] + def test_total_performance_score_with_missing_vitals(self): + self.store_transaction_metric( + 0.03, + metric="measurements.score.lcp", + tags={"transaction": "foo_transaction", "transaction.op": "pageload"}, + timestamp=self.min_ago, + ) + self.store_transaction_metric( + 0.30, + metric="measurements.score.weight.lcp", + tags={"transaction": "foo_transaction", "transaction.op": "pageload"}, + timestamp=self.min_ago, + ) + self.store_transaction_metric( + 0.15, + metric="measurements.score.fcp", + tags={"transaction": "foo_transaction", "transaction.op": "pageload"}, + timestamp=self.min_ago, + ) + self.store_transaction_metric( + 0.15, + metric="measurements.score.weight.fcp", + tags={"transaction": "foo_transaction", "transaction.op": "pageload"}, + timestamp=self.min_ago, + ) + response = self.do_request( + { + "field": [ + "transaction", + "performance_score(measurements.score.total)", + ], + "query": "", + "dataset": "metrics", + "per_page": 50, + } + ) + assert response.status_code == 200, response.content + assert len(response.data["data"]) == 1 + data = response.data["data"] + meta = response.data["meta"] + assert data[0]["performance_score(measurements.score.total)"] == 0.4 + assert meta["isMetricsData"] + def test_count_scores(self): self.store_transaction_metric( 0.1, @@ -4030,6 +4073,10 @@ def test_performance_score_boundaries(self): def test_total_performance_score(self): super().test_total_performance_score() + @pytest.mark.xfail(reason="Not implemented") + def test_total_performance_score_with_missing_vitals(self): + super().test_total_performance_score_with_missing_vitals() + @pytest.mark.xfail(reason="Not implemented") def test_invalid_performance_score_column(self): super().test_invalid_performance_score_column() From 5615ad4e31e1ebe4e6f38a1e3bdb6127187c1e82 Mon Sep 17 00:00:00 2001 From: Edward Gou Date: Tue, 31 Dec 2024 11:30:57 -0500 Subject: [PATCH 2/3] feature flag check --- src/sentry/search/events/datasets/metrics.py | 56 +++++++++++-------- .../endpoints/test_organization_events_mep.py | 35 ++++++------ 2 files changed, 50 insertions(+), 41 deletions(-) diff --git a/src/sentry/search/events/datasets/metrics.py b/src/sentry/search/events/datasets/metrics.py index 323cae81c31163..e408bab4202295 100644 --- a/src/sentry/search/events/datasets/metrics.py +++ b/src/sentry/search/events/datasets/metrics.py @@ -5,6 +5,7 @@ from django.utils.functional import cached_property from snuba_sdk import Column, Condition, Function, Op, OrderBy +from sentry import features from sentry.api.event_search import SearchFilter from sentry.exceptions import IncompatibleMetricsQuery, InvalidSearchQuery from sentry.search.events import constants, fields @@ -1906,30 +1907,37 @@ def _resolve_total_performance_score_function( scores["inp"], ], ), - Function( - "plus", - [ - Function( - "plus", - [ - Function( - "plus", - [ - Function( - "plus", - [ - weights["lcp"], - weights["fcp"], - ], - ), - weights["cls"], - ], - ), - weights["ttfb"], - ], - ), - weights["inp"], - ], + ( + Function( + "plus", + [ + Function( + "plus", + [ + Function( + "plus", + [ + Function( + "plus", + [ + weights["lcp"], + weights["fcp"], + ], + ), + weights["cls"], + ], + ), + weights["ttfb"], + ], + ), + weights["inp"], + ], + ) + if features.has( + "organizations:performance-vitals-handle-missing-webvitals", + self.builder.params.organization, + ) + else 1 ), ], alias, diff --git a/tests/snuba/api/endpoints/test_organization_events_mep.py b/tests/snuba/api/endpoints/test_organization_events_mep.py index d3dd9aca6a77b9..5ea16a2954cacf 100644 --- a/tests/snuba/api/endpoints/test_organization_events_mep.py +++ b/tests/snuba/api/endpoints/test_organization_events_mep.py @@ -3125,23 +3125,24 @@ def test_total_performance_score_with_missing_vitals(self): tags={"transaction": "foo_transaction", "transaction.op": "pageload"}, timestamp=self.min_ago, ) - response = self.do_request( - { - "field": [ - "transaction", - "performance_score(measurements.score.total)", - ], - "query": "", - "dataset": "metrics", - "per_page": 50, - } - ) - assert response.status_code == 200, response.content - assert len(response.data["data"]) == 1 - data = response.data["data"] - meta = response.data["meta"] - assert data[0]["performance_score(measurements.score.total)"] == 0.4 - assert meta["isMetricsData"] + with self.feature({"organizations:performance-vitals-handle-missing-webvitals": True}): + response = self.do_request( + { + "field": [ + "transaction", + "performance_score(measurements.score.total)", + ], + "query": "", + "dataset": "metrics", + "per_page": 50, + } + ) + assert response.status_code == 200, response.content + assert len(response.data["data"]) == 1 + data = response.data["data"] + meta = response.data["meta"] + assert data[0]["performance_score(measurements.score.total)"] == 0.4 + assert meta["isMetricsData"] def test_count_scores(self): self.store_transaction_metric( From 2cd5a0c85cd6b546563715a029b484c71db0113d Mon Sep 17 00:00:00 2001 From: Edward Gou Date: Thu, 2 Jan 2025 14:20:44 -0500 Subject: [PATCH 3/3] add doc strings --- src/sentry/search/events/datasets/metrics.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/sentry/search/events/datasets/metrics.py b/src/sentry/search/events/datasets/metrics.py index ef343b9781378b..faf147d5a023a9 100644 --- a/src/sentry/search/events/datasets/metrics.py +++ b/src/sentry/search/events/datasets/metrics.py @@ -1532,6 +1532,12 @@ def _resolve_web_vital_score_function( args: Mapping[str, str | Column | SelectType | int | float], alias: str | None, ) -> SelectType: + """Returns the normalized score (0.0-1.0) for a given web vital. + This function exists because we don't store a metric for the normalized score. + The normalized score is calculated by dividing the sum of measurements.score.* by the sum of measurements.score.weight.* + + To calculate the total performance score, see _resolve_total_performance_score_function. + """ column = args["column"] metric_id = args["metric_id"] @@ -1836,6 +1842,14 @@ def _resolve_total_performance_score_function( _: Mapping[str, str | Column | SelectType | int | float], alias: str | None, ) -> SelectType: + """Returns the total performance score based on a page/site's web vitals. + This function is calculated by: + the summation of (normalized_vital_score * weight) for each vital, divided by the sum of all weights + - normalized_vital_score is the 0.0-1.0 score for each individual vital + - weight is the 0.0-1.0 weight for each individual vital (this is a constant value stored in constants.WEB_VITALS_PERFORMANCE_SCORE_WEIGHTS) + - if all webvitals have data, then the sum of all weights is 1 + - normalized_vital_score is obtained through _resolve_web_vital_score_function (see docstring on that function for more details) + """ vitals = ["lcp", "fcp", "cls", "ttfb", "inp"] scores = { vital: Function(