-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
feat(anomaly detection): add chart preview to new alert form #78238
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #78238 +/- ##
===========================================
+ Coverage 52.26% 78.30% +26.03%
===========================================
Files 7106 7132 +26
Lines 312658 314001 +1343
Branches 51071 51275 +204
===========================================
+ Hits 163416 245863 +82447
+ Misses 147648 61716 -85932
- Partials 1594 6422 +4828 |
f9b0916
to
87ad8b7
Compare
87ad8b7
to
5c52250
Compare
5c52250
to
5c0ef18
Compare
5c0ef18
to
98df95f
Compare
bdd69de
to
d25ff37
Compare
88f76fd
to
48c5e4d
Compare
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.
mostly just comments about the prop drilling for anomalies and hoping we can create a context. if that's out of scope and want to take it on with ACI, that's cool too.
static/app/views/alerts/rules/metric/details/metricChartOption.tsx
Outdated
Show resolved
Hide resolved
static/app/views/alerts/rules/metric/triggers/chart/thresholdsChart.tsx
Outdated
Show resolved
Hide resolved
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.
JS / reasoning behind implementation sounds good to me - i'd recommend having someone on anomaly detection do a quick pass to make sure the UI / features are working as expected.
0533d60
to
f2f8e66
Compare
0531e73
to
729154a
Compare
5ff096c
to
0f466cc
Compare
bbdcb4f
to
0163b78
Compare
53ae7e9
to
973076d
Compare
Adds anomaly chart visualization to new alert form (
ruleForm.tsx
+<TriggersChart />
).Related PRs:
Closes https://getsentry.atlassian.net/browse/ALRT-289
Context
In order to render the preview chart, we need to send the current dataset and 28 days of historical context to Seer for processing. In an ideal world, the backend would manage this for us, but there are technical constraints which make this unworkable.
I opted to implement this logic as unobtrusively as possible rather than taking on a large refactor. This PR extends the existing declarative
<EndpointRequest />
pattern to fetch any historical data, which is then passed to the/organization/org-slug/events/anomalies/
endpoint viathis.api
.Is this ideal? Absolutely not! I am looking forward to refactoring this form to FC +
useApiQuery
as part of the upcoming Alerts Create Issues effort.