From 602e3c26fc8aa1a34f7199e187ee1f9879d29b53 Mon Sep 17 00:00:00 2001 From: Taylan Gocmen Date: Wed, 27 Oct 2021 16:20:12 -0700 Subject: [PATCH 1/8] use basechart in test too --- .../alerts/incidentRules/triggersChart.spec.jsx | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/js/spec/views/alerts/incidentRules/triggersChart.spec.jsx b/tests/js/spec/views/alerts/incidentRules/triggersChart.spec.jsx index 6294ae35f8f6a8..a4fd629df2b235 100644 --- a/tests/js/spec/views/alerts/incidentRules/triggersChart.spec.jsx +++ b/tests/js/spec/views/alerts/incidentRules/triggersChart.spec.jsx @@ -2,7 +2,7 @@ import {mountWithTheme} from 'sentry-test/enzyme'; import {initializeOrg} from 'sentry-test/initializeOrg'; import {Client} from 'app/api'; -import AreaChart from 'app/components/charts/areaChart'; +import BaseChart from 'app/components/charts/baseChart'; import TriggersChart from 'app/views/alerts/incidentRules/triggers/chart'; jest.mock('app/components/charts/areaChart'); @@ -14,7 +14,7 @@ describe('Incident Rules Create', () => { beforeEach(() => { MockApiClient.clearMockResponses(); - AreaChart.default = jest.fn(() => null); + BaseChart.default = jest.fn(() => null); api = new Client(); eventStatsMock = MockApiClient.addMockResponse({ url: '/organizations/org-slug/events-stats/', @@ -73,7 +73,7 @@ describe('Incident Rules Create', () => { }) ); - expect(AreaChart).toHaveBeenCalledWith( + expect(BaseChart).toHaveBeenCalledWith( expect.objectContaining({ series: [{data: expect.objectContaining({length: 1}), seriesName: 'count()'}], }), @@ -137,15 +137,15 @@ describe('Incident Rules Create', () => { ); // "series" accessed directly to assist with jest diff - expect(AreaChart.mock.calls[0][0].series).toEqual([ + expect(BaseChart.mock.calls[0][0].series).toEqual([ {data: expect.anything(), seriesName: 'count()'}, {data: expect.anything(), seriesName: 'Minimum'}, {data: expect.anything(), seriesName: 'Average'}, {data: expect.anything(), seriesName: 'Maximum'}, ]); - expect(AreaChart.mock.calls[0][0].series[0].data).toHaveLength(1199); - expect(AreaChart.mock.calls[0][0].series[1].data).toHaveLength(239); - expect(AreaChart.mock.calls[0][0].series[2].data).toHaveLength(239); - expect(AreaChart.mock.calls[0][0].series[3].data).toHaveLength(239); + expect(BaseChart.mock.calls[0][0].series[0].data).toHaveLength(1199); + expect(BaseChart.mock.calls[0][0].series[1].data).toHaveLength(239); + expect(BaseChart.mock.calls[0][0].series[2].data).toHaveLength(239); + expect(BaseChart.mock.calls[0][0].series[3].data).toHaveLength(239); }); }); From 0a708d964fa91a96abdcd16dfde6ba572752ea01 Mon Sep 17 00:00:00 2001 From: Taylan Gocmen Date: Wed, 27 Oct 2021 16:38:38 -0700 Subject: [PATCH 2/8] more mock --- tests/js/spec/views/alerts/incidentRules/triggersChart.spec.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/js/spec/views/alerts/incidentRules/triggersChart.spec.jsx b/tests/js/spec/views/alerts/incidentRules/triggersChart.spec.jsx index a4fd629df2b235..e2afe7da660914 100644 --- a/tests/js/spec/views/alerts/incidentRules/triggersChart.spec.jsx +++ b/tests/js/spec/views/alerts/incidentRules/triggersChart.spec.jsx @@ -5,7 +5,7 @@ import {Client} from 'app/api'; import BaseChart from 'app/components/charts/baseChart'; import TriggersChart from 'app/views/alerts/incidentRules/triggers/chart'; -jest.mock('app/components/charts/areaChart'); +jest.mock('app/components/charts/baseChart'); describe('Incident Rules Create', () => { let eventStatsMock; From 5de8116c62b1b775ce80328835933e0f07a57c2a Mon Sep 17 00:00:00 2001 From: Taylan Gocmen Date: Wed, 27 Oct 2021 17:45:38 -0700 Subject: [PATCH 3/8] revert back to area chart --- static/app/components/charts/areaChart.tsx | 46 ++++++++++--------- .../incidentRules/triggersChart.spec.jsx | 18 ++++---- 2 files changed, 34 insertions(+), 30 deletions(-) diff --git a/static/app/components/charts/areaChart.tsx b/static/app/components/charts/areaChart.tsx index 3c67484fe52018..2aaa58c9cf753e 100644 --- a/static/app/components/charts/areaChart.tsx +++ b/static/app/components/charts/areaChart.tsx @@ -12,37 +12,41 @@ export type AreaChartSeries = Series & Omit & { series: AreaChartSeries[]; + additionalSeries?: EChartOption.SeriesLine[]; stacked?: boolean; }; class AreaChart extends React.Component { render() { - const {series, stacked, colors, ...props} = this.props; + const {series, additionalSeries, stacked, colors, ...props} = this.props; return ( - AreaSeries({ - stack: stacked ? 'area' : undefined, - name: seriesName, - data: data.map(({name, value}) => [name, value]), - lineStyle: { - color: colors?.[i], - opacity: 1, - width: 0.4, - }, - areaStyle: { - color: colors?.[i], - opacity: 1.0, - }, - animation: false, - animationThreshold: 1, - animationDuration: 0, - ...otherSeriesProps, - }) - )} + series={[ + ...series.map(({seriesName, data, ...otherSeriesProps}, i) => + AreaSeries({ + stack: stacked ? 'area' : undefined, + name: seriesName, + data: data.map(({name, value}) => [name, value]), + lineStyle: { + color: colors?.[i], + opacity: 1, + width: 0.4, + }, + areaStyle: { + color: colors?.[i], + opacity: 1.0, + }, + animation: false, + animationThreshold: 1, + animationDuration: 0, + ...otherSeriesProps, + }) + ), + ...(additionalSeries || []), + ]} /> ); } diff --git a/tests/js/spec/views/alerts/incidentRules/triggersChart.spec.jsx b/tests/js/spec/views/alerts/incidentRules/triggersChart.spec.jsx index e2afe7da660914..6294ae35f8f6a8 100644 --- a/tests/js/spec/views/alerts/incidentRules/triggersChart.spec.jsx +++ b/tests/js/spec/views/alerts/incidentRules/triggersChart.spec.jsx @@ -2,10 +2,10 @@ import {mountWithTheme} from 'sentry-test/enzyme'; import {initializeOrg} from 'sentry-test/initializeOrg'; import {Client} from 'app/api'; -import BaseChart from 'app/components/charts/baseChart'; +import AreaChart from 'app/components/charts/areaChart'; import TriggersChart from 'app/views/alerts/incidentRules/triggers/chart'; -jest.mock('app/components/charts/baseChart'); +jest.mock('app/components/charts/areaChart'); describe('Incident Rules Create', () => { let eventStatsMock; @@ -14,7 +14,7 @@ describe('Incident Rules Create', () => { beforeEach(() => { MockApiClient.clearMockResponses(); - BaseChart.default = jest.fn(() => null); + AreaChart.default = jest.fn(() => null); api = new Client(); eventStatsMock = MockApiClient.addMockResponse({ url: '/organizations/org-slug/events-stats/', @@ -73,7 +73,7 @@ describe('Incident Rules Create', () => { }) ); - expect(BaseChart).toHaveBeenCalledWith( + expect(AreaChart).toHaveBeenCalledWith( expect.objectContaining({ series: [{data: expect.objectContaining({length: 1}), seriesName: 'count()'}], }), @@ -137,15 +137,15 @@ describe('Incident Rules Create', () => { ); // "series" accessed directly to assist with jest diff - expect(BaseChart.mock.calls[0][0].series).toEqual([ + expect(AreaChart.mock.calls[0][0].series).toEqual([ {data: expect.anything(), seriesName: 'count()'}, {data: expect.anything(), seriesName: 'Minimum'}, {data: expect.anything(), seriesName: 'Average'}, {data: expect.anything(), seriesName: 'Maximum'}, ]); - expect(BaseChart.mock.calls[0][0].series[0].data).toHaveLength(1199); - expect(BaseChart.mock.calls[0][0].series[1].data).toHaveLength(239); - expect(BaseChart.mock.calls[0][0].series[2].data).toHaveLength(239); - expect(BaseChart.mock.calls[0][0].series[3].data).toHaveLength(239); + expect(AreaChart.mock.calls[0][0].series[0].data).toHaveLength(1199); + expect(AreaChart.mock.calls[0][0].series[1].data).toHaveLength(239); + expect(AreaChart.mock.calls[0][0].series[2].data).toHaveLength(239); + expect(AreaChart.mock.calls[0][0].series[3].data).toHaveLength(239); }); }); From ce427ab371bc5bb48da3471e42a39667eaed0088 Mon Sep 17 00:00:00 2001 From: Taylan Gocmen Date: Wed, 27 Oct 2021 18:06:22 -0700 Subject: [PATCH 4/8] fix series --- static/app/components/charts/areaChart.tsx | 58 ++++++++++------------ 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/static/app/components/charts/areaChart.tsx b/static/app/components/charts/areaChart.tsx index 2aaa58c9cf753e..d5fe863e8165cf 100644 --- a/static/app/components/charts/areaChart.tsx +++ b/static/app/components/charts/areaChart.tsx @@ -18,37 +18,33 @@ type Props = Omit & { class AreaChart extends React.Component { render() { - const {series, additionalSeries, stacked, colors, ...props} = this.props; - - return ( - - AreaSeries({ - stack: stacked ? 'area' : undefined, - name: seriesName, - data: data.map(({name, value}) => [name, value]), - lineStyle: { - color: colors?.[i], - opacity: 1, - width: 0.4, - }, - areaStyle: { - color: colors?.[i], - opacity: 1.0, - }, - animation: false, - animationThreshold: 1, - animationDuration: 0, - ...otherSeriesProps, - }) - ), - ...(additionalSeries || []), - ]} - /> - ); + const {series: _series, additionalSeries, stacked, colors, ...props} = this.props; + + const series = [ + ..._series.map(({seriesName, data, ...otherSeriesProps}, i) => + AreaSeries({ + stack: stacked ? 'area' : undefined, + name: seriesName, + data: data.map(({name, value}) => [name, value]), + lineStyle: { + color: colors?.[i], + opacity: 1, + width: 0.4, + }, + areaStyle: { + color: colors?.[i], + opacity: 1.0, + }, + animation: false, + animationThreshold: 1, + animationDuration: 0, + ...otherSeriesProps, + }) + ), + ...(additionalSeries || []), + ]; + + return ; } } From d981bd1bba737b9fd443cfd0559dbf1bbd0bbda0 Mon Sep 17 00:00:00 2001 From: Taylan Gocmen Date: Wed, 27 Oct 2021 18:24:34 -0700 Subject: [PATCH 5/8] complete bypass area chart --- static/app/components/charts/areaChart.tsx | 56 +++++++++++----------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/static/app/components/charts/areaChart.tsx b/static/app/components/charts/areaChart.tsx index d5fe863e8165cf..3c67484fe52018 100644 --- a/static/app/components/charts/areaChart.tsx +++ b/static/app/components/charts/areaChart.tsx @@ -12,39 +12,39 @@ export type AreaChartSeries = Series & Omit & { series: AreaChartSeries[]; - additionalSeries?: EChartOption.SeriesLine[]; stacked?: boolean; }; class AreaChart extends React.Component { render() { - const {series: _series, additionalSeries, stacked, colors, ...props} = this.props; - - const series = [ - ..._series.map(({seriesName, data, ...otherSeriesProps}, i) => - AreaSeries({ - stack: stacked ? 'area' : undefined, - name: seriesName, - data: data.map(({name, value}) => [name, value]), - lineStyle: { - color: colors?.[i], - opacity: 1, - width: 0.4, - }, - areaStyle: { - color: colors?.[i], - opacity: 1.0, - }, - animation: false, - animationThreshold: 1, - animationDuration: 0, - ...otherSeriesProps, - }) - ), - ...(additionalSeries || []), - ]; - - return ; + const {series, stacked, colors, ...props} = this.props; + + return ( + + AreaSeries({ + stack: stacked ? 'area' : undefined, + name: seriesName, + data: data.map(({name, value}) => [name, value]), + lineStyle: { + color: colors?.[i], + opacity: 1, + width: 0.4, + }, + areaStyle: { + color: colors?.[i], + opacity: 1.0, + }, + animation: false, + animationThreshold: 1, + animationDuration: 0, + ...otherSeriesProps, + }) + )} + /> + ); } } From 1c13a2749f7c2cf67818e9c55b565e07be2606ba Mon Sep 17 00:00:00 2001 From: Taylan Gocmen Date: Wed, 27 Oct 2021 15:40:58 -0700 Subject: [PATCH 6/8] feat(cmp_alerts): Change alert data in dashed line in alert details --- static/app/components/charts/lineChart.tsx | 28 ++-- .../changeAlerts/comparisonMarklines.tsx | 133 ++++++++++++++++ .../constants.tsx} | 0 .../incidentRules/triggers/chart/index.tsx | 142 ++---------------- .../views/alerts/issueRuleEditor/index.tsx | 2 +- .../alerts/issueRuleEditor/ruleNodeList.tsx | 6 +- .../alerts/rules/details/metricChart.tsx | 95 ++++++++++-- 7 files changed, 249 insertions(+), 157 deletions(-) create mode 100644 static/app/views/alerts/changeAlerts/comparisonMarklines.tsx rename static/app/views/alerts/{issueRuleEditor/constants/changeAlerts.tsx => changeAlerts/constants.tsx} (100%) diff --git a/static/app/components/charts/lineChart.tsx b/static/app/components/charts/lineChart.tsx index a6f32a1f466aca..4ae3666e4b8c5b 100644 --- a/static/app/components/charts/lineChart.tsx +++ b/static/app/components/charts/lineChart.tsx @@ -15,27 +15,31 @@ export type LineChartSeries = Series & type Props = Omit & { series: LineChartSeries[]; + lineSeries?: EChartOption.SeriesLine[]; seriesOptions?: EChartOption.SeriesLine; }; export default class LineChart extends React.Component { render() { - const {series, seriesOptions, ...props} = this.props; + const {series, lineSeries, seriesOptions, ...props} = this.props; return ( - LineSeries({ - ...seriesOptions, - ...options, - name: seriesName, - data: dataArray || data.map(({value, name}) => [name, value]), - animation: false, - animationThreshold: 1, - animationDuration: 0, - }) - )} + series={[ + ...series.map(({seriesName, data, dataArray, ...options}) => + LineSeries({ + ...seriesOptions, + ...options, + name: seriesName, + data: dataArray || data.map(({value, name}) => [name, value]), + animation: false, + animationThreshold: 1, + animationDuration: 0, + }) + ), + ...(lineSeries ?? []), + ]} /> ); } diff --git a/static/app/views/alerts/changeAlerts/comparisonMarklines.tsx b/static/app/views/alerts/changeAlerts/comparisonMarklines.tsx new file mode 100644 index 00000000000000..50faebd4c4bb6b --- /dev/null +++ b/static/app/views/alerts/changeAlerts/comparisonMarklines.tsx @@ -0,0 +1,133 @@ +import MarkLine from 'app/components/charts/components/markLine'; +import {LineChartSeries} from 'app/components/charts/lineChart'; +import {t} from 'app/locale'; +import {Series} from 'app/types/echarts'; +import {MINUTE} from 'app/utils/formatters'; +import theme from 'app/utils/theme'; +import {AlertRuleThresholdType, Trigger} from 'app/views/alerts/incidentRules/types'; + +const checkChangeStatus = ( + value: number, + thresholdType: AlertRuleThresholdType, + triggers: Trigger[] +): string => { + const criticalTrigger = triggers?.find(trig => trig.label === 'critical'); + const warningTrigger = triggers?.find(trig => trig.label === 'warning'); + const criticalTriggerAlertThreshold = + typeof criticalTrigger?.alertThreshold === 'number' + ? criticalTrigger.alertThreshold + : undefined; + const warningTriggerAlertThreshold = + typeof warningTrigger?.alertThreshold === 'number' + ? warningTrigger.alertThreshold + : undefined; + + // Need to catch the critical threshold cases before warning threshold cases + if ( + thresholdType === AlertRuleThresholdType.ABOVE && + criticalTriggerAlertThreshold && + value >= criticalTriggerAlertThreshold + ) { + return 'critical'; + } + if ( + thresholdType === AlertRuleThresholdType.ABOVE && + warningTriggerAlertThreshold && + value >= warningTriggerAlertThreshold + ) { + return 'warning'; + } + // When threshold is below(lower than in comparison alerts) the % diff value is negative + // It crosses the threshold if its abs value is greater than threshold + // -80% change crosses below 60% threshold -1 * (-80) > 60 + if ( + thresholdType === AlertRuleThresholdType.BELOW && + criticalTriggerAlertThreshold && + -1 * value >= criticalTriggerAlertThreshold + ) { + return 'critical'; + } + if ( + thresholdType === AlertRuleThresholdType.BELOW && + warningTriggerAlertThreshold && + -1 * value >= warningTriggerAlertThreshold + ) { + return 'warning'; + } + + return ''; +}; + +export const getComparisonMarkLines = ( + timeseriesData: Series[] = [], + comparisonTimeseriesData: Series[] = [], + timeWindow: number, + triggers: Trigger[], + thresholdType: AlertRuleThresholdType +): LineChartSeries[] => { + const changeStatuses: {name: number | string; status: string}[] = []; + + if ( + timeseriesData?.[0]?.data !== undefined && + timeseriesData[0].data.length > 1 && + comparisonTimeseriesData?.[0]?.data !== undefined && + comparisonTimeseriesData[0].data.length > 1 + ) { + const changeData = comparisonTimeseriesData[0].data; + const baseData = timeseriesData[0].data; + + if (triggers.some(({alertThreshold}) => typeof alertThreshold === 'number')) { + const lastPointLimit = + (baseData[changeData.length - 1].name as number) - timeWindow * MINUTE; + changeData.forEach(({name, value: comparisonValue}, idx) => { + const baseValue = baseData[idx].value; + const comparisonPercentage = + comparisonValue === 0 + ? baseValue === 0 + ? 0 + : Infinity + : ((baseValue - comparisonValue) / comparisonValue) * 100; + const status = checkChangeStatus(comparisonPercentage, thresholdType, triggers); + if ( + idx === 0 || + idx === changeData.length - 1 || + status !== changeStatuses[changeStatuses.length - 1].status + ) { + changeStatuses.push({name, status}); + } + }); + + return changeStatuses.slice(0, -1).map(({name, status}, idx) => ({ + seriesName: t('status'), + type: 'line', + markLine: MarkLine({ + silent: true, + lineStyle: { + color: + status === 'critical' + ? theme.red300 + : status === 'warning' + ? theme.yellow300 + : theme.green300, + type: 'solid', + width: 4, + }, + data: [ + [ + {coord: [name, 0]}, + { + coord: [ + Math.min(changeStatuses[idx + 1].name as number, lastPointLimit), + 0, + ], + }, + ] as any, + ], + }), + data: [], + })); + } + } + + return []; +}; diff --git a/static/app/views/alerts/issueRuleEditor/constants/changeAlerts.tsx b/static/app/views/alerts/changeAlerts/constants.tsx similarity index 100% rename from static/app/views/alerts/issueRuleEditor/constants/changeAlerts.tsx rename to static/app/views/alerts/changeAlerts/constants.tsx diff --git a/static/app/views/alerts/incidentRules/triggers/chart/index.tsx b/static/app/views/alerts/incidentRules/triggers/chart/index.tsx index 93744e374a013d..bfa86fe26f0f9d 100644 --- a/static/app/views/alerts/incidentRules/triggers/chart/index.tsx +++ b/static/app/views/alerts/incidentRules/triggers/chart/index.tsx @@ -8,7 +8,6 @@ import minBy from 'lodash/minBy'; import {fetchTotalCount} from 'app/actionCreators/events'; import {Client} from 'app/api'; import Feature from 'app/components/acl/feature'; -import MarkLine from 'app/components/charts/components/markLine'; import EventsRequest from 'app/components/charts/eventsRequest'; import {LineChartSeries} from 'app/components/charts/lineChart'; import OptionSelector from 'app/components/charts/optionSelector'; @@ -25,13 +24,12 @@ import {t} from 'app/locale'; import space from 'app/styles/space'; import {Organization, Project} from 'app/types'; import {Series, SeriesDataUnit} from 'app/types/echarts'; -import {MINUTE} from 'app/utils/formatters'; import { getCrashFreeRateSeries, MINUTES_THRESHOLD_TO_DISPLAY_SECONDS, } from 'app/utils/sessions'; -import theme from 'app/utils/theme'; import withApi from 'app/utils/withApi'; +import {getComparisonMarkLines} from 'app/views/alerts/changeAlerts/comparisonMarklines'; import {COMPARISON_DELTA_OPTIONS} from 'app/views/alerts/incidentRules/constants'; import {isSessionAggregate, SESSION_AGGREGATE_TO_FIELD} from 'app/views/alerts/utils'; import {AlertWizardAlertNames} from 'app/views/alerts/wizard/options'; @@ -39,7 +37,6 @@ import {getAlertTypeFromAggregateDataset} from 'app/views/alerts/wizard/utils'; import { AlertRuleComparisonType, - AlertRuleThresholdType, Dataset, IncidentRule, SessionsAggregate, @@ -227,80 +224,6 @@ class TriggersChart extends React.PureComponent { ); } - getComparisonMarkLines( - timeseriesData: Series[] = [], - comparisonTimeseriesData: Series[] = [] - ): LineChartSeries[] { - const {timeWindow} = this.props; - const changeStatuses: {name: number | string; status: string}[] = []; - - if ( - timeseriesData?.[0]?.data !== undefined && - timeseriesData[0].data.length > 1 && - comparisonTimeseriesData?.[0]?.data !== undefined && - comparisonTimeseriesData[0].data.length > 1 - ) { - const changeData = comparisonTimeseriesData[0].data; - const baseData = timeseriesData[0].data; - - if ( - this.props.triggers.some(({alertThreshold}) => typeof alertThreshold === 'number') - ) { - const lastPointLimit = - (baseData[changeData.length - 1].name as number) - timeWindow * MINUTE; - changeData.forEach(({name, value: comparisonValue}, idx) => { - const baseValue = baseData[idx].value; - const comparisonPercentage = - comparisonValue === 0 - ? baseValue === 0 - ? 0 - : Infinity - : ((baseValue - comparisonValue) / comparisonValue) * 100; - const status = this.checkChangeStatus(comparisonPercentage); - if ( - idx === 0 || - idx === changeData.length - 1 || - status !== changeStatuses[changeStatuses.length - 1].status - ) { - changeStatuses.push({name, status}); - } - }); - - return changeStatuses.slice(0, -1).map(({name, status}, idx) => ({ - seriesName: t('status'), - type: 'line', - markLine: MarkLine({ - silent: true, - lineStyle: { - color: - status === 'critical' - ? theme.red300 - : status === 'warning' - ? theme.yellow300 - : theme.green300, - type: 'solid', - width: 4, - }, - data: [ - [ - {coord: [name, 0]}, - { - coord: [ - Math.min(changeStatuses[idx + 1].name as number, lastPointLimit), - 0, - ], - }, - ] as any, - ], - }), - data: [], - })); - } - } - - return []; - } - async fetchTotalCount() { const {api, organization, environment, projects, query} = this.props; const statsPeriod = this.getStatsPeriod(); @@ -318,55 +241,6 @@ class TriggersChart extends React.PureComponent { } } - checkChangeStatus(value: number): string { - const {thresholdType, triggers} = this.props; - const criticalTrigger = triggers?.find(trig => trig.label === 'critical'); - const warningTrigger = triggers?.find(trig => trig.label === 'warning'); - const criticalTriggerAlertThreshold = - typeof criticalTrigger?.alertThreshold === 'number' - ? criticalTrigger.alertThreshold - : undefined; - const warningTriggerAlertThreshold = - typeof warningTrigger?.alertThreshold === 'number' - ? warningTrigger.alertThreshold - : undefined; - - // Need to catch the critical threshold cases before warning threshold cases - if ( - thresholdType === AlertRuleThresholdType.ABOVE && - criticalTriggerAlertThreshold && - value >= criticalTriggerAlertThreshold - ) { - return 'critical'; - } - if ( - thresholdType === AlertRuleThresholdType.ABOVE && - warningTriggerAlertThreshold && - value >= warningTriggerAlertThreshold - ) { - return 'warning'; - } - // When threshold is below(lower than in comparison alerts) the % diff value is negative - // It crosses the threshold if its abs value is greater than threshold - // -80% change crosses below 60% threshold -1 * (-80) > 60 - if ( - thresholdType === AlertRuleThresholdType.BELOW && - criticalTriggerAlertThreshold && - -1 * value >= criticalTriggerAlertThreshold - ) { - return 'critical'; - } - if ( - thresholdType === AlertRuleThresholdType.BELOW && - warningTriggerAlertThreshold && - -1 * value >= warningTriggerAlertThreshold - ) { - return 'warning'; - } - - return ''; - } - renderChart( timeseriesData: Series[] = [], isLoading: boolean, @@ -448,11 +322,14 @@ class TriggersChart extends React.PureComponent { aggregate, environment, comparisonDelta, + triggers, + thresholdType, } = this.props; const period = this.getStatsPeriod(); - const renderComparisonStats = - organization.features.includes('change-alerts') && comparisonDelta; + const renderComparisonStats = Boolean( + organization.features.includes('change-alerts') && comparisonDelta + ); return isSessionAggregate(aggregate) ? ( { {({loading, reloading, timeseriesData, comparisonTimeseriesData}) => { let comparisonMarkLines: LineChartSeries[] = []; if (renderComparisonStats && comparisonTimeseriesData) { - comparisonMarkLines = this.getComparisonMarkLines( + comparisonMarkLines = getComparisonMarkLines( timeseriesData, - comparisonTimeseriesData + comparisonTimeseriesData, + timeWindow, + triggers, + thresholdType ); } diff --git a/static/app/views/alerts/issueRuleEditor/index.tsx b/static/app/views/alerts/issueRuleEditor/index.tsx index 2f408e3b904339..f6ce24d03a2157 100644 --- a/static/app/views/alerts/issueRuleEditor/index.tsx +++ b/static/app/views/alerts/issueRuleEditor/index.tsx @@ -44,7 +44,7 @@ import withTeams from 'app/utils/withTeams'; import { CHANGE_ALERT_CONDITION_IDS, CHANGE_ALERT_PLACEHOLDERS_LABELS, -} from 'app/views/alerts/issueRuleEditor/constants/changeAlerts'; +} from 'app/views/alerts/changeAlerts/constants'; import AsyncView from 'app/views/asyncView'; import Input from 'app/views/settings/components/forms/controls/input'; import Field from 'app/views/settings/components/forms/field'; diff --git a/static/app/views/alerts/issueRuleEditor/ruleNodeList.tsx b/static/app/views/alerts/issueRuleEditor/ruleNodeList.tsx index 8a9b6daa39c828..6f823559c32eaa 100644 --- a/static/app/views/alerts/issueRuleEditor/ruleNodeList.tsx +++ b/static/app/views/alerts/issueRuleEditor/ruleNodeList.tsx @@ -12,14 +12,14 @@ import { IssueAlertRuleCondition, IssueAlertRuleConditionTemplate, } from 'app/types/alerts'; -import {EVENT_FREQUENCY_PERCENT_CONDITION} from 'app/views/projectInstall/issueAlertOptions'; - import { CHANGE_ALERT_CONDITION_IDS, COMPARISON_INTERVAL_CHOICES, COMPARISON_TYPE_CHOICE_VALUES, COMPARISON_TYPE_CHOICES, -} from './constants/changeAlerts'; +} from 'app/views/alerts/changeAlerts/constants'; +import {EVENT_FREQUENCY_PERCENT_CONDITION} from 'app/views/projectInstall/issueAlertOptions'; + import RuleNode from './ruleNode'; type Props = { diff --git a/static/app/views/alerts/rules/details/metricChart.tsx b/static/app/views/alerts/rules/details/metricChart.tsx index c3e0ef1e0fed98..efd3e572e223bb 100644 --- a/static/app/views/alerts/rules/details/metricChart.tsx +++ b/static/app/views/alerts/rules/details/metricChart.tsx @@ -2,6 +2,7 @@ import * as React from 'react'; import {withRouter, WithRouterProps} from 'react-router'; import styled from '@emotion/styled'; import color from 'color'; +import capitalize from 'lodash/capitalize'; import moment from 'moment'; import momentTimezone from 'moment-timezone'; @@ -14,6 +15,7 @@ import MarkArea from 'app/components/charts/components/markArea'; import MarkLine from 'app/components/charts/components/markLine'; import EventsRequest from 'app/components/charts/eventsRequest'; import LineChart, {LineChartSeries} from 'app/components/charts/lineChart'; +import LineSeries from 'app/components/charts/series/lineSeries'; import SessionsRequest from 'app/components/charts/sessionsRequest'; import {SectionHeading} from 'app/components/charts/styles'; import { @@ -35,7 +37,9 @@ import { MINUTES_THRESHOLD_TO_DISPLAY_SECONDS, } from 'app/utils/sessions'; import theme from 'app/utils/theme'; +import {getComparisonMarkLines} from 'app/views/alerts/changeAlerts/comparisonMarklines'; import {alertDetailsLink} from 'app/views/alerts/details'; +import {COMPARISON_DELTA_OPTIONS} from 'app/views/alerts/incidentRules/constants'; import {makeDefaultCta} from 'app/views/alerts/incidentRules/incidentRulePresets'; import {Dataset, IncidentRule} from 'app/views/alerts/incidentRules/types'; import {AlertWizardAlertNames} from 'app/views/alerts/wizard/options'; @@ -326,7 +330,8 @@ class MetricChart extends React.PureComponent { renderChart( loading: boolean, timeseriesData?: Series[], - minutesThresholdToDisplaySeconds?: number + minutesThresholdToDisplaySeconds?: number, + comparisonTimeseriesData?: Series[] ) { const { router, @@ -346,6 +351,10 @@ class MetricChart extends React.PureComponent { return this.renderEmpty(); } + const renderComparisonStats = Boolean( + organization.features.includes('change-alerts') && rule.comparisonDelta + ); + const criticalTrigger = rule.triggers.find(({label}) => label === 'critical'); const warningTrigger = rule.triggers.find(({label}) => label === 'warning'); @@ -379,10 +388,13 @@ class MetricChart extends React.PureComponent { let criticalDuration = 0; let warningDuration = 0; - series.push( - createStatusAreaSeries(theme.green300, firstPoint, lastPoint, minChartValue) - ); - if (incidents) { + if (!renderComparisonStats) { + series.push( + createStatusAreaSeries(theme.green300, firstPoint, lastPoint, minChartValue) + ); + } + + if (incidents && !renderComparisonStats) { // select incidents that fall within the graph range const periodStart = moment.utc(firstPoint); @@ -520,6 +532,22 @@ class MetricChart extends React.PureComponent { maxThresholdValue = Math.max(maxThresholdValue, alertThreshold); } + let comparisonMarkLines: LineChartSeries[] = []; + if (renderComparisonStats && comparisonTimeseriesData) { + comparisonMarkLines = getComparisonMarkLines( + timeseriesData, + comparisonTimeseriesData, + 0, + rule.triggers, + rule.thresholdType + ); + } + + const comparisonSeriesName = capitalize( + COMPARISON_DELTA_OPTIONS.find(({value}) => value === rule.comparisonDelta)?.label || + '' + ); + return ( @@ -566,6 +594,28 @@ class MetricChart extends React.PureComponent { min: minChartValue || undefined, }} series={[...series, ...areaSeries]} + lineSeries={[ + ...(comparisonTimeseriesData || []).map( + ({data: _data, ...otherSeriesProps}) => + LineSeries({ + name: comparisonSeriesName, + data: _data.map(({name, value}) => [name, value]), + lineStyle: {color: theme.gray200, type: 'dashed', width: 1}, + animation: false, + animationThreshold: 1, + animationDuration: 0, + ...otherSeriesProps, + }) + ), + ...comparisonMarkLines.map( + ({seriesName, data: _data, ...seriesProps}) => + LineSeries({ + name: seriesName, + data: _data.map(({name, value}) => [name, value]), + ...seriesProps, + }) + ), + ]} graphic={Graphic({ elements: this.getRuleChangeThresholdElements(timeseriesData), })} @@ -582,6 +632,19 @@ class MetricChart extends React.PureComponent { seriesName ?? '', rule.aggregate ); + + const comparisonSeries = pointSeries.find( + ({seriesName: _seriesName}) => + _seriesName === comparisonSeriesName + ); + const comparisonPointY = comparisonSeries + ? alertTooltipValueFormatter( + comparisonSeries.data[1], + seriesName ?? '', + rule.aggregate + ) + : null; + const isModified = dateModified && pointX <= new Date(dateModified).getTime(); @@ -600,17 +663,26 @@ class MetricChart extends React.PureComponent { const title = isModified ? `${t('Alert Rule Modified')}` : `${marker} ${seriesName}`; + const value = isModified ? `${seriesName} ${pointYFormatted}` : pointYFormatted; + const comparisonTitle = isModified + ? `${comparisonSeriesName}` + : `${comparisonSeriesName}`; + return [ - `
`, - `${title}${value}`, - `
`, + `
`, + `
${title}${value}
`, + comparisonSeries && + `
${comparisonTitle}${comparisonPointY}
`, + `
`, `
${startTime} — ${endTime}
`, `
`, - ].join(''); + ] + .filter(e => e) + .join(''); }, }} onFinished={() => { @@ -703,6 +775,7 @@ class MetricChart extends React.PureComponent { .filter(p => p && p.slug) .map(project => Number(project.id))} interval={interval} + comparisonDelta={rule.comparisonDelta ? rule.comparisonDelta * 60 : undefined} start={viableStartDate} end={viableEndDate} yAxis={aggregate} @@ -711,7 +784,9 @@ class MetricChart extends React.PureComponent { partial={false} referrer="api.alerts.alert-rule-chart" > - {({loading, timeseriesData}) => this.renderChart(loading, timeseriesData)} + {({loading, timeseriesData, comparisonTimeseriesData}) => + this.renderChart(loading, timeseriesData, undefined, comparisonTimeseriesData) + } ); } From c02f5038a0a084ae6f9177db510d27a1ac39ac1e Mon Sep 17 00:00:00 2001 From: Taylan Gocmen Date: Wed, 27 Oct 2021 19:03:20 -0700 Subject: [PATCH 7/8] use additionalSeries for line chart too --- static/app/components/charts/lineChart.tsx | 28 ++++++++----------- .../alerts/rules/details/metricChart.tsx | 2 +- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/static/app/components/charts/lineChart.tsx b/static/app/components/charts/lineChart.tsx index 4ae3666e4b8c5b..a6f32a1f466aca 100644 --- a/static/app/components/charts/lineChart.tsx +++ b/static/app/components/charts/lineChart.tsx @@ -15,31 +15,27 @@ export type LineChartSeries = Series & type Props = Omit & { series: LineChartSeries[]; - lineSeries?: EChartOption.SeriesLine[]; seriesOptions?: EChartOption.SeriesLine; }; export default class LineChart extends React.Component { render() { - const {series, lineSeries, seriesOptions, ...props} = this.props; + const {series, seriesOptions, ...props} = this.props; return ( - LineSeries({ - ...seriesOptions, - ...options, - name: seriesName, - data: dataArray || data.map(({value, name}) => [name, value]), - animation: false, - animationThreshold: 1, - animationDuration: 0, - }) - ), - ...(lineSeries ?? []), - ]} + series={series.map(({seriesName, data, dataArray, ...options}) => + LineSeries({ + ...seriesOptions, + ...options, + name: seriesName, + data: dataArray || data.map(({value, name}) => [name, value]), + animation: false, + animationThreshold: 1, + animationDuration: 0, + }) + )} /> ); } diff --git a/static/app/views/alerts/rules/details/metricChart.tsx b/static/app/views/alerts/rules/details/metricChart.tsx index efd3e572e223bb..5eb95796a75ee0 100644 --- a/static/app/views/alerts/rules/details/metricChart.tsx +++ b/static/app/views/alerts/rules/details/metricChart.tsx @@ -594,7 +594,7 @@ class MetricChart extends React.PureComponent { min: minChartValue || undefined, }} series={[...series, ...areaSeries]} - lineSeries={[ + additionalSeries={[ ...(comparisonTimeseriesData || []).map( ({data: _data, ...otherSeriesProps}) => LineSeries({ From 69b424b2959a0798cd9b284ad0a8ef44ae925647 Mon Sep 17 00:00:00 2001 From: Taylan Gocmen Date: Thu, 28 Oct 2021 10:42:26 -0700 Subject: [PATCH 8/8] fix marklines to render --- .../alerts/rules/details/metricChart.tsx | 34 +++---------------- 1 file changed, 4 insertions(+), 30 deletions(-) diff --git a/static/app/views/alerts/rules/details/metricChart.tsx b/static/app/views/alerts/rules/details/metricChart.tsx index 5eb95796a75ee0..f9ec0fbe6025db 100644 --- a/static/app/views/alerts/rules/details/metricChart.tsx +++ b/static/app/views/alerts/rules/details/metricChart.tsx @@ -37,7 +37,6 @@ import { MINUTES_THRESHOLD_TO_DISPLAY_SECONDS, } from 'app/utils/sessions'; import theme from 'app/utils/theme'; -import {getComparisonMarkLines} from 'app/views/alerts/changeAlerts/comparisonMarklines'; import {alertDetailsLink} from 'app/views/alerts/details'; import {COMPARISON_DELTA_OPTIONS} from 'app/views/alerts/incidentRules/constants'; import {makeDefaultCta} from 'app/views/alerts/incidentRules/incidentRulePresets'; @@ -351,10 +350,6 @@ class MetricChart extends React.PureComponent { return this.renderEmpty(); } - const renderComparisonStats = Boolean( - organization.features.includes('change-alerts') && rule.comparisonDelta - ); - const criticalTrigger = rule.triggers.find(({label}) => label === 'critical'); const warningTrigger = rule.triggers.find(({label}) => label === 'warning'); @@ -388,13 +383,11 @@ class MetricChart extends React.PureComponent { let criticalDuration = 0; let warningDuration = 0; - if (!renderComparisonStats) { - series.push( - createStatusAreaSeries(theme.green300, firstPoint, lastPoint, minChartValue) - ); - } + series.push( + createStatusAreaSeries(theme.green300, firstPoint, lastPoint, minChartValue) + ); - if (incidents && !renderComparisonStats) { + if (incidents) { // select incidents that fall within the graph range const periodStart = moment.utc(firstPoint); @@ -532,17 +525,6 @@ class MetricChart extends React.PureComponent { maxThresholdValue = Math.max(maxThresholdValue, alertThreshold); } - let comparisonMarkLines: LineChartSeries[] = []; - if (renderComparisonStats && comparisonTimeseriesData) { - comparisonMarkLines = getComparisonMarkLines( - timeseriesData, - comparisonTimeseriesData, - 0, - rule.triggers, - rule.thresholdType - ); - } - const comparisonSeriesName = capitalize( COMPARISON_DELTA_OPTIONS.find(({value}) => value === rule.comparisonDelta)?.label || '' @@ -607,14 +589,6 @@ class MetricChart extends React.PureComponent { ...otherSeriesProps, }) ), - ...comparisonMarkLines.map( - ({seriesName, data: _data, ...seriesProps}) => - LineSeries({ - name: seriesName, - data: _data.map(({name, value}) => [name, value]), - ...seriesProps, - }) - ), ]} graphic={Graphic({ elements: this.getRuleChangeThresholdElements(timeseriesData),