diff --git a/static/app/views/detectors/components/details/metric/chart.tsx b/static/app/views/detectors/components/details/metric/chart.tsx index 3fa158fe3af793..7c29b314e8aec2 100644 --- a/static/app/views/detectors/components/details/metric/chart.tsx +++ b/static/app/views/detectors/components/details/metric/chart.tsx @@ -143,11 +143,13 @@ function MetricDetectorChart({ const openPeriodMarkerResult = useIncidentMarkers({ incidents: incidentPeriods, + includePreviousIntervalMarker: true, seriesName: t('Open Periods'), seriesId: '__incident_marker__', yAxisIndex: 1, // Use index 1 to avoid conflict with main chart axis seriesTooltip: incidentSeriesTooltip, markLineTooltip: incidentMarklineTooltip, + intervalMs: snubaQuery.timeWindow * 1000, onClick: context => { const startMs = context.period.start; const endMs = context.period.end ?? Date.now(); diff --git a/static/app/views/detectors/components/forms/metric/metricDetectorChart.tsx b/static/app/views/detectors/components/forms/metric/metricDetectorChart.tsx index 7d676acd1fc747..b142f3582b8913 100644 --- a/static/app/views/detectors/components/forms/metric/metricDetectorChart.tsx +++ b/static/app/views/detectors/components/forms/metric/metricDetectorChart.tsx @@ -210,6 +210,7 @@ export function MetricDetectorChart({ yAxisIndex: 1, // Use index 1 to avoid conflict with main chart axis seriesTooltip: anomalySeriesTooltip, markLineTooltip: anomalyMarklineTooltip, + intervalMs: interval * 1000, }); // Calculate y-axis bounds to ensure all thresholds are visible diff --git a/static/app/views/detectors/hooks/useIncidentMarkers.spec.tsx b/static/app/views/detectors/hooks/useIncidentMarkers.spec.tsx new file mode 100644 index 00000000000000..4668c9485f28f1 --- /dev/null +++ b/static/app/views/detectors/hooks/useIncidentMarkers.spec.tsx @@ -0,0 +1,71 @@ +import {renderHookWithProviders} from 'sentry-test/reactTestingLibrary'; + +import { + useIncidentMarkers, + type IncidentPeriod, +} from 'sentry/views/detectors/hooks/useIncidentMarkers'; + +describe('useIncidentMarkers', () => { + const incident: IncidentPeriod = { + id: 'incident-1', + name: 'Test Incident', + color: '#ff0000', + type: 'test', + start: 10_000, + end: 10_000 + 60_000, // ms + }; + const intervalMs = 60_000; // 1 minute + + describe('includePreviousIntervalMarker', () => { + it('omits trigger-interval marker when includePreviousIntervalMarker is false', () => { + const {result} = renderHookWithProviders(() => + useIncidentMarkers({ + incidents: [incident], + seriesName: 'Incidents', + intervalMs, + includePreviousIntervalMarker: false, + }) + ); + + // Just a single open period marker + const series = result.current.incidentMarkerSeries; + expect(series).not.toBeNull(); + expect(series!.data).toHaveLength(1); + }); + + it('adds trigger-interval marker when includePreviousIntervalMarker is true', () => { + const {result} = renderHookWithProviders(() => + useIncidentMarkers({ + incidents: [incident], + seriesName: 'Incidents', + intervalMs, + includePreviousIntervalMarker: true, + }) + ); + + const series = result.current.incidentMarkerSeries; + const data = series!.data as IncidentPeriod[]; + expect(data).toHaveLength(2); + + const triggerInterval = data[0]; + const openPeriod = data[1]; + + expect(triggerInterval).toEqual( + expect.objectContaining({ + color: incident.color, + // Should start 1 interval before the incident and end at the start of the incident + start: incident.start - intervalMs, + end: incident.start, + }) + ); + + expect(openPeriod).toEqual( + expect.objectContaining({ + color: incident.color, + start: incident.start, + end: incident.end, + }) + ); + }); + }); +}); diff --git a/static/app/views/detectors/hooks/useIncidentMarkers.tsx b/static/app/views/detectors/hooks/useIncidentMarkers.tsx index 13ad3ef2ecca10..5bd08826eea433 100644 --- a/static/app/views/detectors/hooks/useIncidentMarkers.tsx +++ b/static/app/views/detectors/hooks/useIncidentMarkers.tsx @@ -66,14 +66,72 @@ export interface IncidentPeriod { interface IncidentMarkerSeriesProps { incidentPeriods: IncidentPeriod[]; + intervalMs: number; markLineTooltip: UseIncidentMarkersProps['markLineTooltip']; seriesId: string; seriesName: string; seriesTooltip: UseIncidentMarkersProps['seriesTooltip']; theme: Theme; yAxisIndex: number; + includePreviousIntervalMarker?: boolean; } +interface IncidentMarkerPeriod extends IncidentPeriod { + type: 'trigger-interval' | 'open-period'; +} + +function createTriggerPeriodMarkerData({ + period, + intervalMs, +}: { + intervalMs: number; + period: IncidentPeriod; +}): IncidentMarkerPeriod { + return { + ...period, + start: period.start - intervalMs, + end: period.start, + type: 'trigger-interval', + }; +} + +function createOpenPeriodMarkerData({ + period, +}: { + period: IncidentPeriod; +}): IncidentMarkerPeriod { + return { + ...period, + type: 'open-period', + }; +} + +const makeStripeBackgroundSvgNode = (color: string) => { + return { + key: 'stripe-background', + tag: 'svg', + attrs: { + width: '2', + height: `${INCIDENT_MARKER_HEIGHT}`, + viewBox: `0 0 2 ${INCIDENT_MARKER_HEIGHT}`, + shapeRendering: 'crispEdges', + }, + children: [ + { + key: 'stripe-background-line', + tag: 'rect', + attrs: { + x: '0', + y: '0', + width: '1', + height: `${INCIDENT_MARKER_HEIGHT}`, + fill: color, + }, + }, + ], + }; +}; + /** * Creates a custom series that renders incident highlights underneath the main chart */ @@ -85,11 +143,23 @@ function IncidentMarkerSeries({ yAxisIndex, seriesTooltip, markLineTooltip, + intervalMs, + includePreviousIntervalMarker, }: IncidentMarkerSeriesProps): CustomSeriesOption | null { if (!incidentPeriods.length) { return null; } + // TODO: Handle case where trigger period may overlap previous open period + const markerData = incidentPeriods.flatMap(period => { + return includePreviousIntervalMarker + ? [ + createTriggerPeriodMarkerData({period, intervalMs}), + createOpenPeriodMarkerData({period}), + ] + : [createOpenPeriodMarkerData({period})]; + }); + /** * Renders incident highlight rectangles underneath the main chart */ @@ -97,7 +167,7 @@ function IncidentMarkerSeries({ params: CustomSeriesRenderItemParams, api: CustomSeriesRenderItemAPI ): CustomSeriesRenderItemReturn => { - const dataItem = incidentPeriods[params.dataIndex]; + const dataItem = markerData[params.dataIndex]; if (!dataItem) { return {type: 'group', children: []}; @@ -133,7 +203,7 @@ function IncidentMarkerSeries({ y: incidentStartY + renderMarkerPadding - 1, width: width - renderMarkerPadding, height: INCIDENT_MARKER_HEIGHT, - r: 2, + r: [0, 2, 2, 0], }; return { @@ -141,45 +211,54 @@ function IncidentMarkerSeries({ transition: ['shape'], shape, style: { - fill: dataItem.color, - opacity: 0.9, + fill: + dataItem.type === 'trigger-interval' + ? { + svgElement: makeStripeBackgroundSvgNode(dataItem.color), + svgWidth: 2, + svgHeight: INCIDENT_MARKER_HEIGHT, + } + : dataItem.color, + opacity: 1, }, }; }; // Create mark lines for the start of each incident period - const markLineData: MarkLineComponentOption['data'] = incidentPeriods.map(period => { - const lineStyle: MarkLineComponentOption['lineStyle'] = { - color: period.color ?? theme.gray400, - type: 'solid', - width: 1, - opacity: 0.8, - }; + const markLineData: MarkLineComponentOption['data'] = markerData + .filter(period => period.type === 'open-period') + .map(period => { + const lineStyle: MarkLineComponentOption['lineStyle'] = { + color: period.color ?? theme.gray400, + type: 'solid', + width: 1, + opacity: 0.8, + }; - return { - xAxis: period.start, - lineStyle, - emphasis: { - lineStyle: { - ...lineStyle, - width: 2, - opacity: 1, + return { + xAxis: period.start, + lineStyle, + emphasis: { + lineStyle: { + ...lineStyle, + width: 2, + opacity: 1, + }, }, - }, - label: { - show: false, - }, - tooltip: { - trigger: 'item', - position: 'bottom', - formatter: markLineTooltip - ? () => { - return markLineTooltip({theme, period}); - } - : undefined, - }, - }; - }); + label: { + show: false, + }, + tooltip: { + trigger: 'item', + position: 'bottom', + formatter: markLineTooltip + ? () => { + return markLineTooltip({theme, period}); + } + : undefined, + }, + }; + }); return { id: seriesId ?? INCIDENT_MARKER_SERIES_ID, @@ -187,7 +266,7 @@ function IncidentMarkerSeries({ type: 'custom', yAxisIndex, renderItem: renderIncidentHighlight, - data: incidentPeriods, + data: markerData, color: theme.red300, animation: false, markLine: MarkLine({ @@ -212,7 +291,19 @@ function IncidentMarkerSeries({ interface UseIncidentMarkersProps { incidents: IncidentPeriod[]; + intervalMs: number; seriesName: string; + /** + * If true, adds a marker for the duration of the interval before the beginning + * of an open period. This is used to communicate to the user that the change was + * detected in the preceding interval, which can be a source of confusion in the + * case of large intervals (like 1 day). + * + * If we stored a historical list of evaluated values from the detector, we could + * plot that as a line series instead of using this marker, but that is not something + * that is supported at present. + */ + includePreviousIntervalMarker?: boolean; /** * Provide a custom tooltip for the mark line items */ @@ -248,6 +339,8 @@ export function useIncidentMarkers({ markLineTooltip, yAxisIndex = 0, onClick, + intervalMs, + includePreviousIntervalMarker, }: UseIncidentMarkersProps): UseIncidentMarkersResult { const theme = useTheme(); const chartRef = useRef(null); @@ -329,7 +422,7 @@ export function useIncidentMarkers({ renderItem: () => null, markArea: { itemStyle: { - color: data.hoverColor, + color: data.hoverColor ?? data.color, opacity: 0.2, }, data: [ @@ -427,6 +520,8 @@ export function useIncidentMarkers({ seriesId, seriesTooltip, markLineTooltip, + intervalMs, + includePreviousIntervalMarker, }); }, [ incidentPeriods, @@ -436,6 +531,8 @@ export function useIncidentMarkers({ seriesId, seriesTooltip, markLineTooltip, + intervalMs, + includePreviousIntervalMarker, ]); return {