Skip to content

feat(aci): Use open period activites in chart #101130

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Oct 8, 2025
Merged

feat(aci): Use open period activites in chart #101130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6a2f6c3
feat(aci): Use open period activites in chart
malwilley Oct 7, 2025
619334f
Remove unused intervalMs prop
malwilley Oct 7, 2025
60a037c
Remove open-period-end type, instead determine if it's the last of it…
malwilley Oct 7, 2025
6e72e02
Fix tests and types
malwilley Oct 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions static/app/types/group.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,7 +943,15 @@ export interface BaseGroup {
substatus?: GroupSubstatus | null;
}

interface GroupOpenPeriodActivity {
dateCreated: string;
id: string;
type: 'opened' | 'status_change' | 'closed';
value: 'high' | 'medium' | null;
}

export interface GroupOpenPeriod {
activities: GroupOpenPeriodActivity[];
duration: string;
end: string;
id: string;
Expand Down
85 changes: 66 additions & 19 deletions static/app/views/detectors/components/details/metric/chart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {useMemo} from 'react';
import type {Theme} from '@emotion/react';
import {type Theme} from '@emotion/react';
import styled from '@emotion/styled';
import type {YAXisComponentOption} from 'echarts';

Expand All @@ -12,6 +12,7 @@ import Placeholder from 'sentry/components/placeholder';
import {IconWarning} from 'sentry/icons';
import {t} from 'sentry/locale';
import {space} from 'sentry/styles/space';
import type {GroupOpenPeriod} from 'sentry/types/group';
import type {MetricDetector, SnubaQuery} from 'sentry/types/workflowEngine/detectors';
import {useLocation} from 'sentry/utils/useLocation';
import {useNavigate} from 'sentry/utils/useNavigate';
Expand Down Expand Up @@ -40,26 +41,31 @@ function incidentSeriesTooltip(ctx: IncidentTooltipContext) {
const endTime = ctx.period.end
? defaultFormatAxisLabel(ctx.period.end, true, false, true, false)
: '-';
const priorityDot = `<span style="display:inline-block;width:10px;height:8px;border-radius:100%;background:${ctx.theme.red400};margin-right:6px;vertical-align:middle;"></span>`;
const color = ctx.period.priority === 'high' ? ctx.theme.red300 : ctx.theme.yellow300;
const priorityLabel = ctx.period.priority === 'high' ? t('Critical') : t('Warning');

const priorityDot = `<span style="display:inline-block;width:10px;height:8px;border-radius:100%;background:${color};margin-right:6px;vertical-align:middle;"></span>`;
return [
'<div class="tooltip-series">',
`<div><span class="tooltip-label"><strong>#${ctx.period.id}</strong></span></div>`,
`<div><span class="tooltip-label">${t('Started')}</span> ${startTime}</div>`,
`<div><span class="tooltip-label">${t('Ended')}</span> ${endTime}</div>`,
`<div><span class="tooltip-label">${t('Priority')}</span> ${priorityDot} ${t('Critical')}</div>`,
`<div><span class="tooltip-label">${t('Priority')}</span> ${priorityDot} ${priorityLabel}</div>`,
'</div>',
'<div class="tooltip-arrow arrow-top"></div>',
].join('');
}

function incidentMarklineTooltip(ctx: IncidentTooltipContext) {
const time = defaultFormatAxisLabel(ctx.period.start, true, false, true, false);
const priorityDot = `<span style="display:inline-block;width:10px;height:8px;border-radius:100%;background:${ctx.theme.red400};margin-right:6px;vertical-align:middle;"></span>`;
const color = ctx.period.priority === 'high' ? ctx.theme.red300 : ctx.theme.yellow300;
const priorityLabel = ctx.period.priority === 'high' ? t('Critical') : t('Warning');
const priorityDot = `<span style="display:inline-block;width:10px;height:8px;border-radius:100%;background:${color};margin-right:6px;vertical-align:middle;"></span>`;
return [
'<div class="tooltip-series">',
`<div><span class="tooltip-label"><strong>${t('#%s Triggered', ctx.period.id)}</strong></span></div>`,
`<div><span class="tooltip-label">${t('Started')}</span> ${time}</div>`,
`<div><span class="tooltip-label">${t('Priority')}</span> ${priorityDot} ${t('Critical')}</div>`,
`<div><span class="tooltip-label">${t('Priority')}</span> ${priorityDot} ${priorityLabel}</div>`,
'</div>',
'<div class="tooltip-arrow arrow-top"></div>',
].join('');
Expand All @@ -83,6 +89,53 @@ interface MetricDetectorChartProps {
statsPeriod?: string;
}

function createTriggerIntervalMarkerData({
period,
intervalMs,
}: {
intervalMs: number;
period: GroupOpenPeriod;
}): IncidentPeriod {
return {
id: period.id,
end: new Date(period.start).getTime(),
priority: period.activities[0]?.value ?? 'high',
start: new Date(period.start).getTime() - intervalMs,
type: 'trigger-interval',
};
}

function createOpenPeriodMarkerData({
period,
}: {
period: GroupOpenPeriod;
}): IncidentPeriod[] {
const endDate = period.end ? new Date(period.end).getTime() : Date.now();

const segments = period.activities
.filter(activity => activity.type !== 'closed')
.map((activity, i) => {
const activityEndTime = new Date(
period.activities[i + 1]?.dateCreated ?? period.end ?? endDate
).getTime();

return {
priority: activity.value,
end: activityEndTime,
start: new Date(activity.dateCreated).getTime(),
};
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Index Mismatch in Activity End Time Calculation

In createOpenPeriodMarkerData, the logic for calculating activityEndTime uses an index from a filtered array (segments) to access elements in the original period.activities array. This index mismatch can lead to incorrect end times for activity segments, particularly when 'closed' activities are present and filtered out.

Fix in Cursor Fix in Web

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Trigger Interval and Open Period Marker Data Errors

The createTriggerIntervalMarkerData function incorrectly assigns priority to the trigger interval by using the first activity of the open period, which may not reflect the state before the open period. This also results in a type mismatch if no activities are present. Additionally, createOpenPeriodMarkerData calculates incorrect open period segment end times due to an index mismatch when iterating over filtered activities but accessing the original activity array.

Fix in Cursor Fix in Web


return segments.map((segment, i) => ({
type: i === 0 ? 'open-period-start' : 'open-period-transition',
end: segment.end,
id: period.id,
name: t('Open Periods'),
priority: segment.priority ?? 'high',
start: segment.start,
}));
}

function MetricDetectorChart({
statsPeriod,
start,
Expand Down Expand Up @@ -127,28 +180,22 @@ function MetricDetectorChart({
});

const incidentPeriods = useMemo(() => {
const endDate = end ? new Date(end).getTime() : Date.now();

return openPeriods.map<IncidentPeriod>(period => ({
...period,
// TODO: When open periods return a priority, use that to determine the color
color: 'red',
name: t('Open Periods'),
type: 'openPeriod',
end: period.end ? new Date(period.end).getTime() : endDate,
start: new Date(period.start).getTime(),
}));
}, [openPeriods, end]);
return openPeriods.flatMap<IncidentPeriod>(period => [
createTriggerIntervalMarkerData({
period,
intervalMs: snubaQuery.timeWindow * 1000,
}),
...createOpenPeriodMarkerData({period}),
]);
}, [openPeriods, snubaQuery.timeWindow]);

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();
Expand Down
1 change: 0 additions & 1 deletion static/app/views/detectors/components/forms/metric/metricDetectorChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,6 @@ 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
Expand Down
71 changes: 0 additions & 71 deletions static/app/views/detectors/hooks/useIncidentMarkers.spec.tsx
View file
Open in desktop

This file was deleted.

Loading
Loading