feat(metrics_extraction): Always include the environment in the query hash #63774

armenzg · 2024-01-24T20:19:16Z

Alerts and widgets are colliding since widgets only include the environment in the tags rather than in the query hash.

This change will require 90 days of baking and then we will switch customers over and drop the old spec version.

All widgets will double in number.

tests/sentry/relay/config/test_metric_extraction.py

codecov · 2024-01-24T20:52:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (15e26c8) 81.25% compared to head (c1eb2db) 81.25%.
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #63774   +/-   ##
=======================================
  Coverage   81.25%   81.25%           
=======================================
  Files        5234     5234           
  Lines      230469   230477    +8     
  Branches    45232    45234    +2     
=======================================
+ Hits       187268   187280   +12     
+ Misses      37362    37358    -4     
  Partials     5839     5839

Files	Coverage Δ
src/sentry/conf/server.py	`88.11% <ø> (ø)`
src/sentry/features/__init__.py	`100.00% <100.00%> (ø)`
src/sentry/search/events/builder/metrics.py	`88.58% <100.00%> (-0.02%)`	⬇️
src/sentry/snuba/metrics/extraction.py	`91.05% <100.00%> (+0.10%)`	⬆️

... and 11 files with indirect coverage changes

src/sentry/snuba/metrics/extraction.py

tests/sentry/relay/config/test_metric_extraction.py

k-fish

Looks good, once you get tests passing then 👍

src/sentry/snuba/metrics/extraction.py

iambriccardo

I am lacking context of why we are adding the environment now, but functionally speaking the PR LGTM!

…action/armenzg

…ollision (#64071) When merging specs, iterate over the widgets first. This will make sure that when an alert and widget specs collide, we will pick the spec of a widget which contains the environment in the tags. The next spec version in #63774 does not have this bug since the widget will always include the environment in the query hash, thus, it will not be possible for the alert spec to share the same query hash. This duplication of specs is the main reason that we still have some duplicated specs in [SENTRY-2EAS](https://sentry.sentry.io/issues/4888069767/).

…action/armenzg

armenzg · 2024-01-30T14:26:02Z

src/sentry/search/events/builder/metrics.py

-            # save us to look where to add this logic again
-            spec_version = OnDemandMetricSpecVersioning.get_query_spec_version(self.organization_id)
-            return OnDemandMetricSpec(
+            return fetch_on_demand_metric_spec(


This change will make sure that the tests have the same function to get an OnDemandMetricSpec from a Relay metric spec.

armenzg · 2024-01-30T14:26:48Z

src/sentry/snuba/metrics/extraction.py

@@ -1430,6 +1444,27 @@ def _parse_query(value: str) -> QueryParsingResult:
            raise Exception(f"Invalid search query '{value}' in on demand spec: {e}")


+def fetch_on_demand_metric_spec(


This can now be used by the tests to have a single entry point to querying the right spec based on feature flags.

Jump to the end of tests/sentry/relay/config/test_metric_extraction.py to see.

armenzg · 2024-01-30T14:28:00Z

tests/sentry/relay/config/test_metric_extraction.py

    field = widget.aggregates[0] if widget.aggregates else ""
-    return OnDemandMetricSpec(
+    return fetch_on_demand_metric_spec(


I can now use this shared function to reduce differences between the code and the tests.

armenzg · 2024-01-30T14:29:11Z

tests/sentry/relay/config/test_metric_extraction.py

+            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"}


Compare it without having the feature flag (just above):

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()

k-fish

Looks good!

k-fish · 2024-01-30T15:00:49Z

src/sentry/snuba/metrics/extraction.py

+        if features.has("organizations:on-demand-metrics-query-spec-version-two", org):
+            return cls.spec_versions[1]
        return cls.spec_versions[0]



Not blocking, but doing this by array lookup is a little strange since that feature flag should only be for SpecVersion(2), I think this can be hardcoded, which will make cleanup more obvious as well.

Yeah. I used to have a different approach that did not require any changes.

k-fish · 2024-01-30T15:02:32Z

src/sentry/snuba/metrics/extraction.py

        if (
            self.spec_type == MetricSpecType.DYNAMIC_QUERY
+            and self.spec_version.flags == set()


Not blocking; I don't know that we'll need this flag matching, it's pretty complicated, I can't see us needing a lot of individual behaviours and every new ability to a spec version will essentially reset the spec anyway.

k-fish · 2024-01-30T15:10:20Z

tests/sentry/relay/config/test_metric_extraction.py

            exception = capture_exception.call_args.args[0]
            assert (
                exception.args[0]
-                == "Spec version 1: Too many (2) on demand metric widgets for org baz"
+                == "Spec version 2: Too many (2) on demand metric widgets for org baz"


Okay so both spec version fail here and we're just checking the second one now?

Both of them fail. I couldn't make up my mind. Perhaps I should not even take the version into consideration.

k-fish · 2024-01-30T15:14:32Z

tests/sentry/relay/config/test_metric_extraction.py

+
+        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']"


This is specifically what we want for the new spec version, so 👍

…ollision (#64071) When merging specs, iterate over the widgets first. This will make sure that when an alert and widget specs collide, we will pick the spec of a widget which contains the environment in the tags. The next spec version in #63774 does not have this bug since the widget will always include the environment in the query hash, thus, it will not be possible for the alert spec to share the same query hash. This duplication of specs is the main reason that we still have some duplicated specs in [SENTRY-2EAS](https://sentry.sentry.io/issues/4888069767/).

… hash (#63774) Alerts and widgets are colliding since widgets only include the environment in the tags rather than in the query hash. This change will require 90 days of baking and then we will switch customers over and drop the old spec version. All widget extraction specs will double in number.

armenzg self-assigned this Jan 24, 2024

vercel bot deployed to Preview January 24, 2024 20:19 View deployment

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 24, 2024

armenzg commented Jan 24, 2024

View reviewed changes

tests/sentry/relay/config/test_metric_extraction.py Show resolved Hide resolved

tests/sentry/relay/config/test_metric_extraction.py Outdated Show resolved Hide resolved

k-fish reviewed Jan 24, 2024

View reviewed changes

src/sentry/snuba/metrics/extraction.py Outdated Show resolved Hide resolved

tests/sentry/relay/config/test_metric_extraction.py Outdated Show resolved Hide resolved

vercel bot deployed to Preview January 24, 2024 21:16 View deployment

vercel bot deployed to Preview January 25, 2024 14:10 View deployment

armenzg commented Jan 25, 2024

View reviewed changes

tests/sentry/relay/config/test_metric_extraction.py Outdated Show resolved Hide resolved

armenzg force-pushed the feat/environment_spec_version/metrics_extraction/armenzg branch from df644bc to ce7ebb2 Compare January 25, 2024 20:45

vercel bot deployed to Preview January 25, 2024 20:48 View deployment

vercel bot deployed to Preview January 25, 2024 21:07 View deployment

k-fish approved these changes Jan 25, 2024

View reviewed changes

src/sentry/snuba/metrics/extraction.py Outdated Show resolved Hide resolved

src/sentry/snuba/metrics/extraction.py Outdated Show resolved Hide resolved

armenzg added 11 commits January 26, 2024 07:28

WIP - Initial work

a39e168

Improve the tests

14e52f6

Update the tests

e3d3fb3

Fix test + remove ANY usage

56a5ced

Some of the feedback

ae244cd

Debugging

b564067

Some more changes

4d78c31

Fix double env

2d25cd5

Reset tests

deb648e

Add test back

06e974e

Passing tests

1a10a32

armenzg changed the title ~~feat(metrics_extraction): New spec version to include environment~~ feat(metrics_extraction): Include environment in query hash for widgets Jan 26, 2024

Adjust comments

b6b36ed

armenzg force-pushed the feat/environment_spec_version/metrics_extraction/armenzg branch from 8f2c9db to b6b36ed Compare January 26, 2024 13:48

vercel bot deployed to Preview January 26, 2024 13:51 View deployment

The env should be added to tags in both cases

75d2ae1

vercel bot deployed to Preview January 26, 2024 14:25 View deployment

armenzg requested a review from a team as a code owner January 26, 2024 19:20

armenzg requested review from k-fish, iambriccardo and obostjancic and removed request for iambriccardo and obostjancic January 26, 2024 19:20

iambriccardo approved these changes Jan 29, 2024

View reviewed changes

armenzg added 2 commits January 29, 2024 08:16

Merge branch 'master' into feat/environment_spec_version/metrics_extr…

b80e43b

…action/armenzg

Merge branch 'master' into feat/environment_spec_version/metrics_extr…

dc9f184

…action/armenzg

armenzg mentioned this pull request Jan 29, 2024

fix(metrics_extraction): Pick widget spec when there's a query hash collision #64071

Merged

armenzg added 2 commits January 29, 2024 16:07

Merge branch 'master' into feat/environment_spec_version/metrics_extr…

0ca4e8b

…action/armenzg

Fixes after the merge

594d952

vercel bot deployed to Preview January 29, 2024 21:21 View deployment

Right indent

90d3345

vercel bot deployed to Preview January 29, 2024 21:24 View deployment

Fix typing issues

3934621

vercel bot deployed to Preview January 29, 2024 21:39 View deployment

armenzg added 3 commits January 30, 2024 08:07

Merge branch 'master' into feat/environment_spec_version/metrics_extr…

e41ab4d

…action/armenzg

Adjust the tests

53fc680

Test the query str hash and the spec version

c1eb2db

armenzg requested a review from a team January 30, 2024 14:23

vercel bot deployed to Preview January 30, 2024 14:25 View deployment

armenzg commented Jan 30, 2024

View reviewed changes

k-fish approved these changes Jan 30, 2024

View reviewed changes

armenzg merged commit 04f031f into master Jan 30, 2024
50 checks passed

armenzg deleted the feat/environment_spec_version/metrics_extraction/armenzg branch January 30, 2024 15:38

github-actions bot locked and limited conversation to collaborators Feb 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(metrics_extraction): Always include the environment in the query hash #63774

feat(metrics_extraction): Always include the environment in the query hash #63774

armenzg commented Jan 24, 2024 •

edited

Loading

codecov bot commented Jan 24, 2024 •

edited

Loading

k-fish left a comment

iambriccardo left a comment

armenzg Jan 30, 2024

armenzg Jan 30, 2024

armenzg Jan 30, 2024

armenzg Jan 30, 2024

k-fish left a comment

k-fish Jan 30, 2024

armenzg Jan 30, 2024

k-fish Jan 30, 2024

k-fish Jan 30, 2024

armenzg Jan 30, 2024

k-fish Jan 30, 2024

		@@ -1430,6 +1444,27 @@ def _parse_query(value: str) -> QueryParsingResult:
		raise Exception(f"Invalid search query '{value}' in on demand spec: {e}")


		def fetch_on_demand_metric_spec(

feat(metrics_extraction): Always include the environment in the query hash #63774

feat(metrics_extraction): Always include the environment in the query hash #63774

Conversation

armenzg commented Jan 24, 2024 • edited Loading

codecov bot commented Jan 24, 2024 • edited Loading

Codecov Report

k-fish left a comment

Choose a reason for hiding this comment

iambriccardo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-fish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armenzg commented Jan 24, 2024 •

edited

Loading

codecov bot commented Jan 24, 2024 •

edited

Loading