ref(alerts): Update Snuba queries to match events-stats more closely #77755

ceorourke · 2024-09-18T23:26:29Z

When a user creates an anomaly detection alert we need to query snuba for 28 days worth of historical data to send to Seer to calculate the anomalies. Originally (#74614) I'd tried to pull out the relevant parts of the events-stats endpoint to mimic the data we see populated in metric alert preview charts (but for a larger time period, and it's happening after the rule is saved so I can't use any of the request object stuff) but I think I missed some things, so this PR aims to make that data be the same.

Closes https://getsentry.atlassian.net/browse/ALRT-288 (hopefully)

ceorourke · 2024-09-18T23:46:57Z

src/sentry/api/bases/organization_events.py

@@ -42,6 +42,27 @@
 from sentry.utils.snuba import MAX_FIELDS, SnubaTSResult


+def get_query_columns(columns, rollup):


I moved this to be reused by anomaly detection

ceorourke · 2024-09-18T23:48:00Z

src/sentry/seer/anomaly_detection/utils.py

    """
+    serializer = SnubaTSResultSerializer(organization=organization, lookup=None, user=None)


I'm using the same serializer the events-stats endpoint uses and just pulling that data off to format into a list of TimeSeriesPoints for Seer's API. I clicked through every alert type and it always has the timestamp and count

ceorourke · 2024-09-18T23:49:56Z

src/sentry/seer/anomaly_detection/utils.py

+        data,
+        resolve_axis_column(query_columns[0]),
+        allow_partial_buckets=False,
+        zerofill_results=False,


I was getting strange results in tests with this set to True, and for our purposes I think it doesn't matter that much since we default to sending Seer a 0 if we don't find a count anyway

By "strange" I mean it was hitting this line and overwriting data with a count I had in a test as an empty array.

codecov · 2024-09-19T00:17:41Z

Codecov Report

Attention: Patch coverage is 82.35294% with 12 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/sentry/seer/anomaly_detection/utils.py	78.43%	6 Missing and 5 partials ⚠️
...seer/anomaly_detection/get_historical_anomalies.py	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #77755      +/-   ##
==========================================
+ Coverage   78.09%   78.10%   +0.01%     
==========================================
  Files        6979     6988       +9     
  Lines      309710   310197     +487     
  Branches    50695    50744      +49     
==========================================
+ Hits       241857   242291     +434     
- Misses      61378    61420      +42     
- Partials     6475     6486      +11

ceorourke · 2024-09-19T21:45:33Z

src/sentry/seer/anomaly_detection/utils.py

+        stats_period=None,
+        environments=environments,
+    )
+    snuba_query_string = get_snuba_query_string(snuba_query)


This is one of the key changes here - the front end constructs a stringified query based on snuba_query.query AND snuba_query.event_types. This adds a join to the table for things like errors count with the is:unresolved query, or when you're using the dropdown to select "errors", "default", or "errors OR default" event types

ceorourke · 2024-09-20T00:21:45Z

The users experiencing errors query is selecting data as a different name but it's otherwise the same, I don't know if that makes a difference to the outcome?
events-stats:

SELECT (events._snuba_events.time AS _snuba_events.time), (uniq((events._snuba_events.tags[sentry:user] AS _snuba_events.tags[sentry:user])) AS _snuba_count_unique_user)

anomaly detection:
SELECT (events._snuba_events.time AS _snuba_events.time), (uniq((events._snuba_events.tags[sentry:user] AS _snuba_events.tags[sentry:user])) AS _snuba_count_unique_tags_sentry_user)

ceorourke · 2024-09-20T21:27:05Z

src/sentry/seer/anomaly_detection/utils.py

    if dataset_label == "events":
        # DATASET_OPTIONS expects the name 'errors'
        dataset_label = "errors"
    elif dataset_label == "generic_metrics":
+        # XXX: performance alerts in prod
        dataset_label = "transactions"


not sure if I need to be using the discover dataset here, it's hard to know since it's different locally

I think we do want to use the discover dataset - unfortunately this differs in prod vs. locally so it's confusing but locally we make this request:

api/0/organizations/sentry/events-stats/?interval=60m&project=1&query=event.type%3Atransaction&referrer=api.organization-event-stats&statsPeriod=9998m&yAxis=count%28%29

and in prod we make this request:

api/0/organizations/sentry/events-stats/?dataset=metricsEnhanced&interval=60m&project=1&query=event.type%3Atransaction&referrer=api.organization-event-stats&statsPeriod=9998m&yAxis=count%28%29

the events-stats endpoint gets the dataset label from the request here and falls back to using discover if nothing is passed (like in production).

In our case we're not getting the dataset label from the request but rather the snuba query object, so instead of it being "discover" it's "generic_metrics" in production and "transactions" in my local db.

All that to say for performance metrics alerts we need to use the discover dataset for both generic_metrics and transaction dataset labels

tests/sentry/seer/anomaly_detection/test_store_data.py

saponifi3d

lgtm! I just had some nitpicks - may want to have someone with more insights into seer take another glance too.

src/sentry/seer/anomaly_detection/get_historical_anomalies.py

src/sentry/seer/anomaly_detection/utils.py

saponifi3d · 2024-09-23T21:37:29Z

src/sentry/seer/anomaly_detection/utils.py


    if dataset == metrics_performance:
+        nested_data = data.data.get("data", [])


so, this is... data.data.data? 😅 is there anywhere we have control over any of those that we could rename the variable? i think adding context like what kind of data it is would help a lot.

Sadly no, this is the format of SnubaTSResult which is typed in the function signature.

src/sentry/seer/anomaly_detection/utils.py

tests/sentry/incidents/endpoints/test_organization_alert_rule_anomalies.py

…77755) When a user creates an anomaly detection alert we need to query snuba for 28 days worth of historical data to send to Seer to calculate the anomalies. Originally (#74614) I'd tried to pull out the relevant parts of the `events-stats` endpoint to mimic the data we see populated in metric alert preview charts (but for a larger time period, and it's happening after the rule is saved so I can't use any of the `request` object stuff) but I think I missed some things, so this PR aims to make that data be the same. Closes https://getsentry.atlassian.net/browse/ALRT-288 (hopefully)

…tch events-stats more closely (#77755)" I just want to test if ci is green if this PR is reverted - seems like an issue there... This reverts commit 583a084.

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

vercel bot deployed to Preview September 18, 2024 23:27 View deployment

vercel bot deployed to Preview September 18, 2024 23:35 View deployment

vercel bot deployed to Preview September 18, 2024 23:44 View deployment

ceorourke commented Sep 18, 2024

View reviewed changes

vercel bot deployed to Preview September 18, 2024 23:55 View deployment

vercel bot deployed to Preview September 19, 2024 00:35 View deployment

ceorourke added 8 commits September 19, 2024 10:50

fix snuba query

0fc6cfa

use SnubaTSResultSerializer

bdab736

update some tests

0bf63c7

formatting and update a missing org param

a95aefb

typing

eea294f

be safer, don't need to put ts in a var

f17c46e

oops fix count

7e8170f

fix environments param

4dfe836

ceorourke force-pushed the ceorourke/anomaly-detection-no-none-values branch from 634ed2c to 4dfe836 Compare September 19, 2024 18:00

vercel bot deployed to Preview September 19, 2024 18:04 View deployment

add snuba query event type to query

a00fb6f

vercel bot deployed to Preview September 19, 2024 21:44 View deployment

ceorourke commented Sep 19, 2024

View reviewed changes

make sure it works with and w/o is:unresolved queries

6357580

vercel bot deployed to Preview September 19, 2024 22:30 View deployment

realize I only thought I needed that because the event type didn't match

136d058

vercel bot deployed to Preview September 20, 2024 00:21 View deployment

ceorourke requested a review from wedamija September 20, 2024 00:22

ceorourke mentioned this pull request Sep 20, 2024

feat(anomaly detection):preview chart proxy api endpoint #77813

Merged

put back crash free stuff

21ce07c

vercel bot deployed to Preview September 20, 2024 21:08 View deployment

ceorourke commented Sep 20, 2024

View reviewed changes

ceorourke marked this pull request as ready for review September 20, 2024 21:28

ceorourke requested a review from a team as a code owner September 20, 2024 21:28

update perf alert dataset

699798b

ceorourke requested a review from a team September 20, 2024 21:54

ceorourke commented Sep 20, 2024

View reviewed changes

tests/sentry/seer/anomaly_detection/test_store_data.py Outdated Show resolved Hide resolved

actually make error_type = error ... make the error type be error

0c8ca4c

vercel bot deployed to Preview September 21, 2024 00:01 View deployment

fix tests

43fea40

vercel bot deployed to Preview September 23, 2024 19:40 View deployment

update test

a60dccf

vercel bot deployed to Preview September 23, 2024 20:22 View deployment

saponifi3d approved these changes Sep 23, 2024

View reviewed changes

pr comments

41c04e8

vercel bot deployed to Preview September 23, 2024 22:39 View deployment

oops

d413f0b

vercel bot deployed to Preview September 23, 2024 23:52 View deployment

ceorourke merged commit 583a084 into master Sep 24, 2024
49 of 50 checks passed

ceorourke deleted the ceorourke/anomaly-detection-no-none-values branch September 24, 2024 16:29

github-actions bot locked and limited conversation to collaborators Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(alerts): Update Snuba queries to match events-stats more closely #77755

ref(alerts): Update Snuba queries to match events-stats more closely #77755

ceorourke commented Sep 18, 2024 •

edited

Loading

ceorourke Sep 18, 2024

ceorourke Sep 18, 2024 •

edited

Loading

ceorourke Sep 18, 2024 •

edited

Loading

ceorourke Sep 19, 2024

codecov bot commented Sep 19, 2024 •

edited

Loading

ceorourke Sep 19, 2024

ceorourke commented Sep 20, 2024

ceorourke Sep 20, 2024

ceorourke Sep 20, 2024

saponifi3d left a comment •

edited

Loading

saponifi3d Sep 23, 2024

ceorourke Sep 23, 2024

		@@ -42,6 +42,27 @@
		from sentry.utils.snuba import MAX_FIELDS, SnubaTSResult


		def get_query_columns(columns, rollup):

		"""
		serializer = SnubaTSResultSerializer(organization=organization, lookup=None, user=None)


		if dataset == metrics_performance:
		nested_data = data.data.get("data", [])

ref(alerts): Update Snuba queries to match events-stats more closely #77755

ref(alerts): Update Snuba queries to match events-stats more closely #77755

Conversation

ceorourke commented Sep 18, 2024 • edited Loading

ceorourke Sep 18, 2024

Choose a reason for hiding this comment

ceorourke Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

ceorourke Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

ceorourke Sep 19, 2024

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2024 • edited Loading

Codecov Report

ceorourke Sep 19, 2024

Choose a reason for hiding this comment

ceorourke commented Sep 20, 2024

ceorourke Sep 20, 2024

Choose a reason for hiding this comment

ceorourke Sep 20, 2024

Choose a reason for hiding this comment

saponifi3d left a comment • edited Loading

Choose a reason for hiding this comment

saponifi3d Sep 23, 2024

Choose a reason for hiding this comment

ceorourke Sep 23, 2024

Choose a reason for hiding this comment

ceorourke commented Sep 18, 2024 •

edited

Loading

ceorourke Sep 18, 2024 •

edited

Loading

ceorourke Sep 18, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

saponifi3d left a comment •

edited

Loading