-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
feat(default detector): create default detector for new projects #104208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #104208 +/- ##
===========================================
+ Coverage 80.50% 80.61% +0.11%
===========================================
Files 9342 9344 +2
Lines 399240 400731 +1491
Branches 25560 25560
===========================================
+ Hits 321408 323056 +1648
+ Misses 77383 77226 -157
Partials 449 449 |
23d77c0 to
2d38c85
Compare
roggenkemper
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a test we can add for this?
| detector = _ensure_metric_detector( | ||
| project, owner_team_id=owner_team.id if owner_team else None, enabled=enabled | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this so that an org on a plan w/o anomaly detection would have the default detector in the case that they do upgrade? FYI I have a task in my team's backlog to address an edge case I think you're likely to hit here - Seer won't keep data on detectors they're not receiving updates for over 90 days for, so when they upgrade the alert might not work. Here is the task: https://getsentry.atlassian.net/browse/ACI-528
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will follow up
| assert detector.enabled is True | ||
|
|
||
| def test_send_new_detector_data_failure_does_not_block_creation(self): | ||
| """Test that detector is still created even if sending data to Seer fails.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you'd actually want this to happen - if sending the data to Seer fails then it will not function. However, it might be worth checking with the Seer team since as I mentioned in a previous comment a new project isn't likely to have historical data anyway so maybe it is ok since we wouldn't expect this monitor to function for 28 days - I can't remember if it'd be ok on their end if they suddenly start getting data on a monitor they don't have any previous data for though, I think they might rely on having the previous historical data though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch - I will have it skip creating a detector if sending data to seer fails for now and follow up with seer team
Co-authored-by: Colleen O'Rourke <colleen@sentry.io>
Co-authored-by: Colleen O'Rourke <colleen@sentry.io>
Co-authored-by: Colleen O'Rourke <colleen@sentry.io>
create a default metric monitor for new projects
with these settings:

event.type is errorFeature Flag
organizations:default-anomaly-detector