feat(anomaly detection): warn users if less than one week of historical data is available (backend) #75214

mifu67 · 2024-07-30T00:03:56Z

If snuba returns fewer than 7 days worth of historical data, then surface a warning.

mifu67 · 2024-07-30T00:08:03Z

ack accidentally branched off of a branch ;_;

codecov · 2024-07-30T06:30:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.18%. Comparing base (97bd9c7) to head (777553f).
Report is 5 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #75214   +/-   ##
=======================================
  Coverage   78.18%   78.18%           
=======================================
  Files        6808     6808           
  Lines      302899   302924   +25     
  Branches    52128    52134    +6     
=======================================
+ Hits       236818   236850   +32     
+ Misses      59692    59685    -7     
  Partials     6389     6389

Files	Coverage Δ
...cidents/endpoints/organization_alert_rule_index.py	`98.01% <100.00%> (+0.06%)`	⬆️
src/sentry/seer/anomaly_detection/store_data.py	`83.33% <100.00%> (+4.95%)`	⬆️

... and 11 files with indirect coverage changes

tests/sentry/incidents/endpoints/test_organization_alert_rule_index.py

nhsiehgit

few comments

src/sentry/incidents/endpoints/organization_alert_rule_index.py

tests/sentry/incidents/endpoints/test_organization_alert_rule_index.py

src/sentry/incidents/endpoints/organization_alert_rule_index.py

nhsiehgit · 2024-08-01T15:53:47Z

src/sentry/seer/anomaly_detection/store_data.py

+    if not indices_with_results:
+        return start, end
+    else:
+        start = indices_with_results[0]


Ahhh grab the first and the last because the data is already ordered is that right?
Maybe we should add some quick validations in here for that?

Yeah, we're looping through the data in order, so start is guaranteed to be <= end

I wouldn't be able to test the validation—should I still add it?

maybe a quick assertion here so that it would raise an exception 🤷

could just be a quick assert start < end
This way it will raise in case this is ever false

src/sentry/seer/anomaly_detection/store_data.py

nhsiehgit · 2024-08-01T15:56:07Z

src/sentry/seer/anomaly_detection/store_data.py

+    MIN_DAYS = 7
+    data_start_index, data_end_index = get_start_and_end(historical_data)
+    if data_start_index == -1:
+        resp.status = 202


Ah ok - RE: comments above, lets just throw an exception if there's not enough data.
This should be a loud failure, rather than a seeming success

I think that we do create the alert rule—it just won't detect any anomalies until Seer has at least a week of data. Should we still have an exception in this case?

^^ Yes this is just a warning to the user that it can't fire for some period of time until we've collected the data. We still want to create it and then we'll be periodically sending updated data to Seer

got it, that makes sense!

Commented above - rather than modifying the response and returning the response,
Thoughts on simply returning a boolean for successful backfill or not?

we're not utilizing the rest of the response, and modifying the response is unexpected (for ex. mocking seer response w/ 200, but expecting a 202?)

I think we can do that

maybeeee? backend stuff smol change logic fixed (tests are gonna fail don't come for me) buggies fix stuff and add test cleanup test fixed (?) fix failing test and address some comments another test fix up more tests

nhsiehgit

few comments!

nhsiehgit · 2024-08-02T21:19:44Z

src/sentry/incidents/endpoints/organization_alert_rule_index.py

@@ -106,6 +106,7 @@ def create_metric_alert(self, request, organization, project=None):
            },
            data=data,
        )
+


src/sentry/incidents/logic.py

nhsiehgit · 2024-08-02T21:25:27Z

src/sentry/incidents/logic.py

@@ -652,7 +652,10 @@ def create_alert_rule(
                )

            try:


👌

So we've covered create_alert_rule,
Lets also be sure to capture update_alert_rule as well

I'll work in that in the next PR after Colleen's changes to update_alert_rule land

Can we leave a TODO in the update method so we don't forget it?

src/sentry/incidents/subscription_processor.py

nhsiehgit · 2024-08-02T21:30:46Z

src/sentry/seer/anomaly_detection/store_data.py

+    if not indices_with_results:
+        return start, end
+    else:
+        start = indices_with_results[0]


maybe a quick assertion here so that it would raise an exception 🤷

could just be a quick assert start < end
This way it will raise in case this is ever false

src/sentry/seer/anomaly_detection/store_data.py

nhsiehgit · 2024-08-02T21:36:02Z

src/sentry/seer/anomaly_detection/store_data.py

+    MIN_DAYS = 7
+    data_start_index, data_end_index = get_start_and_end(historical_data)
+    if data_start_index == -1:
+        resp.status = 202


got it, that makes sense!

Commented above - rather than modifying the response and returning the response,
Thoughts on simply returning a boolean for successful backfill or not?

we're not utilizing the rest of the response, and modifying the response is unexpected (for ex. mocking seer response w/ 200, but expecting a 202?)

tests/sentry/incidents/test_subscription_processor.py

ceorourke · 2024-08-02T21:50:19Z

src/sentry/incidents/subscription_processor.py

+                                self.alert_rule.save()
+                            else:
+                                # we don't need to check if the alert should fire if the alert can't fire yet
+                                continue


Do we need to continue iterating through this - like would there be a situation in which the next potential_anomaly would change the result of self.has_enough_data()? If not, we can just break here (which I realize is also a little silly since we expect one anomaly, but 🤷‍♀️

I don't think so, but I also don't know, which is why I did continue instead of break. If only one anomaly is returned, like how it's spec'd in Seer, then they accomplish the same thing. I think continue is just a little safer.

🤔

Would Seer ever actually return any potential anomalies if we don't have enough data?
This check may actually be unnecessary? (but safe to have anyways)

src/sentry/seer/anomaly_detection/store_data.py

nhsiehgit

hell yea!

a few nit comments, but this is looking sharp!

nhsiehgit · 2024-08-05T15:48:08Z

src/sentry/incidents/logic.py

@@ -652,7 +652,10 @@ def create_alert_rule(
                )

            try:


Can we leave a TODO in the update method so we don't forget it?

src/sentry/incidents/logic.py

nhsiehgit · 2024-08-05T16:12:31Z

src/sentry/incidents/subscription_processor.py

+                                self.alert_rule.save()
+                            else:
+                                # we don't need to check if the alert should fire if the alert can't fire yet
+                                continue


🤔

Would Seer ever actually return any potential anomalies if we don't have enough data?
This check may actually be unnecessary? (but safe to have anyways)

src/sentry/seer/anomaly_detection/store_data.py

sentry-io · 2024-08-06T17:41:25Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ TimeoutError /api/0/organizations/{organization_id_or_slug}/... View Issue

_{Did you find this useful? React with a 👍 or 👎}

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

mifu67 force-pushed the mifu67/alert-creation-warning branch from ef6f4b2 to 051a28f Compare July 30, 2024 00:04

mifu67 closed this Jul 30, 2024

mifu67 deleted the mifu67/alert-creation-warning branch July 30, 2024 00:06

vercel bot deployed to Preview July 30, 2024 00:07 View deployment

mifu67 restored the mifu67/alert-creation-warning branch July 30, 2024 00:07

mifu67 reopened this Jul 30, 2024

mifu67 force-pushed the mifu67/alert-creation-warning branch from 051a28f to b1686bf Compare July 30, 2024 00:14

vercel bot deployed to Preview July 30, 2024 00:17 View deployment

vercel bot deployed to Preview July 30, 2024 19:07 View deployment

vercel bot deployed to Preview July 30, 2024 21:03 View deployment

vercel bot deployed to Preview July 30, 2024 21:16 View deployment

vercel bot deployed to Preview July 31, 2024 20:01 View deployment

mifu67 requested a review from ceorourke July 31, 2024 20:09

vercel bot deployed to Preview July 31, 2024 20:10 View deployment

vercel bot deployed to Preview July 31, 2024 21:22 View deployment

mifu67 changed the title ~~feat(anomaly detection): warn users if less than one week of historical data is available~~ feat(anomaly detection): warn users if less than one week of historical data is available (backend) Jul 31, 2024

vercel bot deployed to Preview July 31, 2024 21:41 View deployment

mifu67 marked this pull request as ready for review July 31, 2024 21:41

mifu67 requested a review from a team as a code owner July 31, 2024 21:41

mifu67 commented Jul 31, 2024

View reviewed changes

tests/sentry/incidents/endpoints/test_organization_alert_rule_index.py Outdated Show resolved Hide resolved

vercel bot deployed to Preview August 1, 2024 00:00 View deployment

nhsiehgit reviewed Aug 1, 2024

View reviewed changes

mifu67 requested a review from nhsiehgit August 1, 2024 17:58

vercel bot deployed to Preview August 1, 2024 17:59 View deployment

mifu67 force-pushed the mifu67/alert-creation-warning branch from 346e50c to 43d0fbd Compare August 1, 2024 18:45

vercel bot deployed to Preview August 1, 2024 18:48 View deployment

vercel bot deployed to Preview August 1, 2024 18:57 View deployment

old seven days detection implementation

9e2ebcf

maybeeee? backend stuff smol change logic fixed (tests are gonna fail don't come for me) buggies fix stuff and add test cleanup test fixed (?) fix failing test and address some comments another test fix up more tests

mifu67 force-pushed the mifu67/alert-creation-warning branch from 777553f to 9e2ebcf Compare August 1, 2024 22:42

refactor step 1

3c33428

vercel bot deployed to Preview August 1, 2024 22:48 View deployment

mifu67 force-pushed the mifu67/alert-creation-warning branch from 8a79a46 to 3c33428 Compare August 1, 2024 22:48

🛠️ apply pre-commit fixes

33e776d

vercel bot deployed to Preview August 1, 2024 22:53 View deployment

new logic

1d78546

mifu67 marked this pull request as draft August 2, 2024 18:10

Merge branch 'master' into mifu67/alert-creation-warning

54c870a

mifu67 marked this pull request as ready for review August 2, 2024 18:12

🛠️ apply pre-commit fixes

22bf283

vercel bot deployed to Preview August 2, 2024 18:15 View deployment

debug and add tests

54dd6f6

vercel bot deployed to Preview August 2, 2024 21:17 View deployment

nhsiehgit reviewed Aug 2, 2024

View reviewed changes

ceorourke reviewed Aug 2, 2024

View reviewed changes

src/sentry/seer/anomaly_detection/store_data.py Outdated Show resolved Hide resolved

ceorourke reviewed Aug 2, 2024

View reviewed changes

src/sentry/seer/anomaly_detection/store_data.py Outdated Show resolved Hide resolved

address PR comments

8e619fb

mifu67 requested review from nhsiehgit and ceorourke August 2, 2024 23:21

vercel bot deployed to Preview August 2, 2024 23:23 View deployment

nhsiehgit approved these changes Aug 5, 2024

View reviewed changes

nits

aa22c53

vercel bot deployed to Preview August 5, 2024 17:03 View deployment

mifu67 merged commit c8103fe into master Aug 5, 2024
48 checks passed

mifu67 deleted the mifu67/alert-creation-warning branch August 5, 2024 17:33

github-actions bot locked and limited conversation to collaborators Aug 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(anomaly detection): warn users if less than one week of historical data is available (backend) #75214

feat(anomaly detection): warn users if less than one week of historical data is available (backend) #75214

mifu67 commented Jul 30, 2024

mifu67 commented Jul 30, 2024

codecov bot commented Jul 30, 2024 •

edited

Loading

nhsiehgit left a comment

nhsiehgit Aug 1, 2024

mifu67 Aug 1, 2024

mifu67 Aug 1, 2024

nhsiehgit Aug 2, 2024

nhsiehgit Aug 1, 2024

mifu67 Aug 1, 2024

ceorourke Aug 1, 2024

nhsiehgit Aug 2, 2024

mifu67 Aug 2, 2024

nhsiehgit left a comment

nhsiehgit Aug 2, 2024

nhsiehgit Aug 2, 2024

mifu67 Aug 2, 2024

nhsiehgit Aug 5, 2024

nhsiehgit Aug 2, 2024

nhsiehgit Aug 2, 2024

ceorourke Aug 2, 2024

mifu67 Aug 2, 2024

nhsiehgit Aug 5, 2024

nhsiehgit left a comment

nhsiehgit Aug 5, 2024

nhsiehgit Aug 5, 2024

sentry-io bot commented Aug 6, 2024

feat(anomaly detection): warn users if less than one week of historical data is available (backend) #75214

feat(anomaly detection): warn users if less than one week of historical data is available (backend) #75214

Conversation

mifu67 commented Jul 30, 2024

mifu67 commented Jul 30, 2024

codecov bot commented Jul 30, 2024 • edited Loading

Codecov Report

nhsiehgit 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhsiehgit 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhsiehgit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sentry-io bot commented Aug 6, 2024

Suspect Issues

codecov bot commented Jul 30, 2024 •

edited

Loading