feat(anomaly detection): add anomaly chart preview to new alert #78238

natemoo-re · 2024-09-26T22:03:24Z

Adds anomaly chart visualization to new alert form (ruleForm.tsx + <TriggersChart />).

Related PRs:

Closes https://getsentry.atlassian.net/browse/ALRT-289

codecov · 2024-09-26T22:21:26Z

❌ 7 Tests Failed:

Tests completed	Failed	Passed	Skipped
7945	7	7938	0

View the top 3 failed tests by shortest run time

Incident Rules Create queries insights metrics using the metricsEnhanced dataset and without the metrics layer Incident Rules Create queries insights metrics using the metricsEnhanced dataset and without the metrics layer

Stack Traces | 0.064s run time

Error: expect(jest.fn()).toHaveBeenCalledWith(...expected)

Expected: Anything, ObjectContaining {&quot;query&quot;: {&quot;dataset&quot;: &quot;metricsEnhanced&quot;, &quot;forceMetricsLayer&quot;: undefined, &quot;interval&quot;: &quot;1m&quot;, &quot;project&quot;: [2], &quot;query&quot;: &quot;span.module:db&quot;, &quot;referrer&quot;: &quot;api.organization-event-stats&quot;, &quot;statsPeriod&quot;: &quot;9998m&quot;, &quot;yAxis&quot;: &quot;spm()&quot;}}
Received: &quot;.../organizations/org-slug/events-stats/&quot;, {&quot;error&quot;: [Function error], &quot;query&quot;: {&quot;dataset&quot;: &quot;metricsEnhanced&quot;, &quot;interval&quot;: &quot;1m&quot;, &quot;project&quot;: [2], &quot;query&quot;: &quot;span.module:db&quot;, &quot;referrer&quot;: &quot;api.organization-event-stats&quot;, &quot;statsPeriod&quot;: &quot;7d&quot;, &quot;yAxis&quot;: &quot;spm()&quot;}, &quot;success&quot;: [Function success]}

Number of calls: 1
    at Object.&lt;anonymous&gt; (.../triggers/chart/index.spec.tsx:247:28)

Incident Rules Create does not show &amp; query total count if showTotalCount === false Incident Rules Create does not show &amp; query total count if showTotalCount === false

Stack Traces | 0.073s run time

Error: expect(jest.fn()).toHaveBeenCalledWith(...expected)

Expected: Anything, ObjectContaining {&quot;query&quot;: {&quot;interval&quot;: &quot;1m&quot;, &quot;project&quot;: [2], &quot;query&quot;: &quot;event.type:error&quot;, &quot;referrer&quot;: &quot;api.organization-event-stats&quot;, &quot;statsPeriod&quot;: &quot;9998m&quot;, &quot;yAxis&quot;: &quot;count()&quot;}}
Received: &quot;.../organizations/org-slug/events-stats/&quot;, {&quot;error&quot;: [Function error], &quot;query&quot;: {&quot;interval&quot;: &quot;1m&quot;, &quot;project&quot;: [2], &quot;query&quot;: &quot;event.type:error&quot;, &quot;referrer&quot;: &quot;api.organization-event-stats&quot;, &quot;statsPeriod&quot;: &quot;7d&quot;, &quot;yAxis&quot;: &quot;count()&quot;}, &quot;success&quot;: [Function success]}

Number of calls: 1
    at Object.&lt;anonymous&gt; (.../triggers/chart/index.spec.tsx:101:28)

Incident Rules Create queries custom metrics using the metricsEnhanced dataset and metrics layer Incident Rules Create queries custom metrics using the metricsEnhanced dataset and metrics layer

Stack Traces | 0.084s run time

Error: expect(jest.fn()).toHaveBeenCalledWith(...expected)

Expected: Anything, ObjectContaining {&quot;query&quot;: {&quot;dataset&quot;: &quot;metricsEnhanced&quot;, &quot;forceMetricsLayer&quot;: &quot;true&quot;, &quot;interval&quot;: &quot;1m&quot;, &quot;project&quot;: [2], &quot;query&quot;: &quot;&quot;, &quot;referrer&quot;: &quot;api.organization-event-stats&quot;, &quot;statsPeriod&quot;: &quot;9998m&quot;, &quot;yAxis&quot;: &quot;count(d:custom/my_metric@seconds)&quot;}}
Received: &quot;.../organizations/org-slug/events-stats/&quot;, {&quot;error&quot;: [Function error], &quot;query&quot;: {&quot;dataset&quot;: &quot;metricsEnhanced&quot;, &quot;forceMetricsLayer&quot;: &quot;true&quot;, &quot;interval&quot;: &quot;1m&quot;, &quot;project&quot;: [2], &quot;query&quot;: &quot;&quot;, &quot;referrer&quot;: &quot;api.organization-event-stats&quot;, &quot;statsPeriod&quot;: &quot;7d&quot;, &quot;yAxis&quot;: &quot;count(d:custom/my_metric@seconds)&quot;}, &quot;success&quot;: [Function success]}

Number of calls: 1
    at Object.&lt;anonymous&gt; (.../triggers/chart/index.spec.tsx:203:28)

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

saponifi3d

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.

saponifi3d · 2024-10-08T20:26:04Z

static/app/views/alerts/rules/metric/details/metricChartOption.tsx

-  const firstPoint = new Date(dataArr[0]?.name).getTime();
-  const lastPoint = new Date(dataArr[dataArr.length - 1]?.name).getTime();
+  const startDate = new Date(dataArr[0]?.name);
+  const endDate = new Date(dataArr[dataArr.length - 1]?.name);


need to do any status checks to ensure that dataArr[dataArr.length - 1]?.name isn't the same value as dataArr[0]?.name?

saponifi3d · 2024-10-08T20:26:36Z

static/app/views/alerts/rules/metric/details/metricChartOption.tsx

-        silent: true, // potentially don't make this silent if we want to render the `anomaly detected` in the tooltip
-        data: anomalyBlocks,
-      },
-    });


🔴 - nice.

saponifi3d · 2024-10-08T20:27:55Z

static/app/views/alerts/rules/metric/ruleForm.tsx

@@ -144,6 +149,8 @@ type State = {
  triggerErrors: Map<number, {[fieldName: string]: string}>;
  triggers: Trigger[];
  activationCondition?: ActivationConditionType;
+  chartError?: boolean;
+  chartErrorMessage?: string;


🤔 should we add a context to maintain more of these props? (it's also cool if we don't, and plan to refactor this stuff when we get to ACI work)

saponifi3d · 2024-10-08T20:28:57Z

static/app/views/alerts/rules/metric/ruleForm.tsx

@@ -931,6 +956,7 @@ class RuleFormContainer extends DeprecatedAsyncComponent<Props, State> {
          seasonality: AlertRuleSeasonality.AUTO,
        };
        break;
+      }


nit: omit the {}s for consistency with the other switches - we should probably ensure there's a lint rule one way or the other.

saponifi3d · 2024-10-08T21:21:25Z

static/app/views/alerts/rules/metric/triggers/chart/index.tsx

+        includeTimeAggregation: false,
+        includeTransformedData: false,
+        limit: 15,
+        children: noop,


are the components using these props, using children as a renderProp? (could we just pass null instead?)

Yeah this is decidedly not clean—I think the ideal end state here (after ACI) is that these declarative *Request components go away.

They're already deprecated so I don't want to invest too much in cleaning this up.

saponifi3d · 2024-10-08T21:25:49Z

static/app/views/alerts/rules/metric/triggers/chart/index.tsx

@@ -447,32 +465,165 @@ class TriggersChart extends PureComponent<Props, State> {
    };

    if (isOnDemandMetricAlert) {
+      const {sampleRate} = this.state;
+      const baseProps: EventsRequestProps = {


could we change these baseProps to provide to a context? That way we could decompose all of these sub components fairly easily. I'm a bit nervous about the complexity of this component and being able to grok it.

saponifi3d · 2024-10-08T21:28:46Z

static/app/views/alerts/rules/metric/triggers/chart/index.tsx

@@ -63,6 +66,7 @@ import ThresholdsChart from './thresholdsChart';

 type Props = {
  aggregate: MetricRule['aggregate'];
+  anomalies: Anomaly[];


i'd kinda expect these to be loaded as something else into the Chart component (rather than anomalies). something more like a generic for data or something along those lines? 🤔

saponifi3d · 2024-10-08T21:29:11Z

static/app/views/alerts/rules/metric/triggers/chart/thresholdsChart.tsx

@@ -28,6 +30,7 @@ import type {MetricRule, Trigger} from '../../types';
 import {AlertRuleThresholdType, AlertRuleTriggerType} from '../../types';

 type DefaultProps = {
+  anomalies: Anomaly[];


might be nice to create a context just for anomalies so we can avoid prop drilling

saponifi3d · 2024-10-08T21:30:11Z

static/app/views/alerts/rules/metric/utils/anomalyChart.tsx

@@ -0,0 +1,154 @@
+import type {MarkAreaComponentOption} from 'echarts';
+import moment from 'moment-timezone';


idk how kosher this is, but it might be nice to use built in Intl / Datetime stuff rather than moment 😅

Yeah dropping moment would be great! Going to punt on that until frontend tsc makes some decisions on https://github.com/getsentry/frontend-tsc/issues/69

natemoo-re · 2024-10-08T22:02:05Z

static/app/views/alerts/rules/metric/types.tsx

-  // Seven days is actually 10080m but Snuba can only return up to 10000 entries, for this
-  // we approximate to 9998m which prevents rounding errors due to the minutes granularity
-  // limitations.
-  SEVEN_DAYS = '9998m',
+  SEVEN_DAYS = '7d',


This is what's breaking the tests currently. Can try to add more hacks to workaround the limitation, but probably worth asking if this is still needed?

+1 about asking, unfortunately I don't have context over it.

saponifi3d

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.

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Sep 26, 2024

vercel bot deployed to Preview September 26, 2024 22:06 View deployment

natemoo-re force-pushed the natemoo-re/anomaly-detection-charts branch from f9b0916 to 87ad8b7 Compare October 2, 2024 17:29

vercel bot deployed to Preview October 2, 2024 17:33 View deployment

natemoo-re force-pushed the natemoo-re/anomaly-detection-charts branch from 87ad8b7 to 5c52250 Compare October 4, 2024 22:06

vercel bot deployed to Preview October 4, 2024 22:11 View deployment

natemoo-re force-pushed the natemoo-re/anomaly-detection-charts branch from 5c52250 to 5c0ef18 Compare October 7, 2024 18:54

vercel bot deployed to Preview October 7, 2024 18:59 View deployment

natemoo-re force-pushed the natemoo-re/anomaly-detection-charts branch from 5c0ef18 to 98df95f Compare October 8, 2024 18:47

vercel bot deployed to Preview October 8, 2024 18:51 View deployment

natemoo-re force-pushed the natemoo-re/anomaly-detection-charts branch from bdd69de to d25ff37 Compare October 8, 2024 20:20

natemoo-re marked this pull request as ready for review October 8, 2024 20:21

natemoo-re requested a review from a team as a code owner October 8, 2024 20:21

vercel bot deployed to Preview October 8, 2024 20:26 View deployment

natemoo-re added 12 commits October 8, 2024 15:50

wip: add anomaly support to alert create form

6680c20

feat(anomaly detection): rule form anomaly chart wiring

3602664

fix(alert rule form): clear errors on update

7ff4a2d

fix(alert rule form): missing default state

b6bbf96

feat(anomaly detection): improve api error handling

87245c6

fix(anomaly detection): conform to seer expected values

f9efbe0

refactor(anomaly detection): improve anomaly data handling

cfbb09f

refactor(anomaly detection): extract shared logic to utils

d56f882

feat(anomaly detection): add anomaly support to threshold chart

473f814

feat(anomaly detection): pass anomalies through triggerschart

2a12f79

fix(anomaly detection): improve chart error message

73cfa3c

refactor(anomaly detection): improve chart error message

48c5e4d

natemoo-re force-pushed the natemoo-re/anomaly-detection-charts branch from 88f76fd to 48c5e4d Compare October 8, 2024 20:51

Merge branch 'master' into natemoo-re/anomaly-detection-charts

0533d60

vercel bot deployed to Preview October 8, 2024 21:00 View deployment

saponifi3d reviewed Oct 8, 2024

View reviewed changes

natemoo-re commented Oct 8, 2024

View reviewed changes

saponifi3d reviewed Oct 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(anomaly detection): add anomaly chart preview to new alert #78238

feat(anomaly detection): add anomaly chart preview to new alert #78238

natemoo-re commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading

saponifi3d left a comment

saponifi3d Oct 8, 2024

saponifi3d Oct 8, 2024

saponifi3d Oct 8, 2024

saponifi3d Oct 8, 2024

saponifi3d Oct 8, 2024

natemoo-re Oct 8, 2024

saponifi3d Oct 8, 2024

saponifi3d Oct 8, 2024

saponifi3d Oct 8, 2024

saponifi3d Oct 8, 2024

natemoo-re Oct 8, 2024

natemoo-re Oct 8, 2024

saponifi3d Oct 9, 2024

saponifi3d left a comment

		@@ -0,0 +1,154 @@
		import type {MarkAreaComponentOption} from 'echarts';
		import moment from 'moment-timezone';

feat(anomaly detection): add anomaly chart preview to new alert #78238

Are you sure you want to change the base?

feat(anomaly detection): add anomaly chart preview to new alert #78238

Conversation

natemoo-re commented Sep 26, 2024 • edited Loading

codecov bot commented Sep 26, 2024 • edited Loading

❌ 7 Tests Failed:

saponifi3d 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

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

saponifi3d left a comment

Choose a reason for hiding this comment

natemoo-re commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading