feat: address coderabbit issues on incidents

rioloc · rioloc · commit 4698f8b784dc · 2026-04-22T17:44:23.000+02:00
diff --git a/web/src/components/Incidents/processIncidents.spec.ts b/web/src/components/Incidents/processIncidents.spec.ts
@@ -1017,6 +1017,171 @@ describe('getIncidents', () => {
     });
   });
 
+  describe('severityMetrics per-segment correctness', () => {
+    it('should assign correct metric to each segment when severity repeats (W→C→W)', () => {
+      const t1 = 1000;
+      const t2 = 1300;
+      const t3 = 1600;
+      const t4 = 1900;
+      const t5 = 2200;
+      const t6 = 2500;
+
+      const data: PrometheusResult[] = [
+        {
+          metric: {
+            group_id: 'g1',
+            component: 'comp-warn-early',
+            src_alertname: 'AlertWarnEarly',
+            src_namespace: 'ns-early',
+            src_severity: 'warning',
+          },
+          values: [
+            [t1, '1'],
+            [t2, '1'],
+          ],
+        },
+        {
+          metric: {
+            group_id: 'g1',
+            component: 'comp-crit',
+            src_alertname: 'AlertCrit',
+            src_namespace: 'ns-crit',
+            src_severity: 'critical',
+          },
+          values: [
+            [t3, '2'],
+            [t4, '2'],
+          ],
+        },
+        {
+          metric: {
+            group_id: 'g1',
+            component: 'comp-warn-late',
+            src_alertname: 'AlertWarnLate',
+            src_namespace: 'ns-late',
+            src_severity: 'warning',
+          },
+          values: [
+            [t5, '1'],
+            [t6, '1'],
+          ],
+        },
+      ];
+
+      const result = getIncidents(data);
+      expect(result).toHaveLength(3);
+
+      const segments = [...result].sort((a, b) => a.values[0][0] - b.values[0][0]);
+
+      expect(segments[0].metric.src_alertname).toBe('AlertWarnEarly');
+      expect(segments[0].metric.src_namespace).toBe('ns-early');
+
+      expect(segments[1].metric.src_alertname).toBe('AlertCrit');
+
+      expect(segments[2].metric.src_alertname).toBe('AlertWarnLate');
+      expect(segments[2].metric.src_namespace).toBe('ns-late');
+    });
+
+    it('should not let later results overwrite earlier metric for same severity', () => {
+      const data: PrometheusResult[] = [
+        {
+          metric: {
+            group_id: 'g1',
+            component: 'first',
+            src_alertname: 'First',
+            src_namespace: 'ns-first',
+            src_severity: 'warning',
+          },
+          values: [[1000, '1']],
+        },
+        {
+          metric: {
+            group_id: 'g1',
+            component: 'middle',
+            src_alertname: 'Middle',
+            src_namespace: 'ns-middle',
+            src_severity: 'critical',
+          },
+          values: [[1300, '2']],
+        },
+        {
+          metric: {
+            group_id: 'g1',
+            component: 'second',
+            src_alertname: 'Second',
+            src_namespace: 'ns-second',
+            src_severity: 'warning',
+          },
+          values: [[1600, '1']],
+        },
+      ];
+
+      const result = getIncidents(data);
+      const segments = [...result].sort((a, b) => a.values[0][0] - b.values[0][0]);
+
+      const firstWarning = segments[0];
+      const secondWarning = segments[2];
+
+      expect(firstWarning.metric.src_alertname).not.toBe(secondWarning.metric.src_alertname);
+      expect(firstWarning.metric.src_alertname).toBe('First');
+      expect(secondWarning.metric.src_alertname).toBe('Second');
+    });
+
+    it('should handle four segments with two severity repetitions', () => {
+      const data: PrometheusResult[] = [
+        {
+          metric: {
+            group_id: 'g1',
+            component: 'c1',
+            src_alertname: 'A1',
+            src_namespace: 'ns1',
+            src_severity: 'critical',
+          },
+          values: [[1000, '2']],
+        },
+        {
+          metric: {
+            group_id: 'g1',
+            component: 'c2',
+            src_alertname: 'A2',
+            src_namespace: 'ns2',
+            src_severity: 'warning',
+          },
+          values: [[1300, '1']],
+        },
+        {
+          metric: {
+            group_id: 'g1',
+            component: 'c3',
+            src_alertname: 'A3',
+            src_namespace: 'ns3',
+            src_severity: 'critical',
+          },
+          values: [[1600, '2']],
+        },
+        {
+          metric: {
+            group_id: 'g1',
+            component: 'c4',
+            src_alertname: 'A4',
+            src_namespace: 'ns4',
+            src_severity: 'warning',
+          },
+          values: [[1900, '1']],
+        },
+      ];
+
+      const result = getIncidents(data);
+      const segments = [...result].sort((a, b) => a.values[0][0] - b.values[0][0]);
+
+      expect(segments).toHaveLength(4);
+      expect(segments[0].metric.src_alertname).toBe('A1');
+      expect(segments[1].metric.src_alertname).toBe('A2');
+      expect(segments[2].metric.src_alertname).toBe('A3');
+      expect(segments[3].metric.src_alertname).toBe('A4');
+    });
+  });
+
   describe('silenced status handling', () => {
     it('should use silenced value from most recent timestamp when merging', () => {
       // Use same severity to avoid severity splitting; test focuses on silenced status
diff --git a/web/src/components/Incidents/processIncidents.ts b/web/src/components/Incidents/processIncidents.ts
@@ -178,7 +178,7 @@ export function getIncidents(
 ): Array<PrometheusResult & { metric: Metric }> {
   const incidents = new Map<string, PrometheusResult & { metric: Metric }>();
   // Track which metric labels correspond to each severity value per group_id
-  const severityMetrics = new Map<string, Map<string, PrometheusLabels>>();
+  const severityMetrics = new Map<string, Map<string, PrometheusLabels[]>>();
 
   for (const result of prometheusResults) {
     const groupId = result.metric.group_id;
@@ -188,9 +188,13 @@ export function getIncidents(
       if (!severityMetrics.has(groupId)) {
         severityMetrics.set(groupId, new Map());
       }
+      const metricsForGroup = severityMetrics.get(groupId);
       const severityValues = new Set(result.values.map((v) => v[1]));
       for (const sv of severityValues) {
-        severityMetrics.get(groupId).set(sv, result.metric);
+        if (!metricsForGroup.has(sv)) {
+          metricsForGroup.set(sv, []);
+        }
+        metricsForGroup.get(sv).push(result.metric);
       }
     }
 
@@ -250,7 +254,8 @@ export function getIncidents(
       const segmentSeverity = segmentValues[0][1]; // uniform within a segment
       // Use the metric from the Prometheus result that contributed this severity,
       // preserving shared properties (componentList, silenced) from the merged incident
-      const severitySpecificMetric = groupSeverityMetrics?.get(segmentSeverity);
+      const metricsArray = groupSeverityMetrics?.get(segmentSeverity);
+      const severitySpecificMetric = metricsArray?.shift();
 
       result.push({
         metric: {
diff --git a/web/src/components/Incidents/utils.spec.ts b/web/src/components/Incidents/utils.spec.ts
@@ -1,11 +1,12 @@
 import {
+  createAlertsChartBars,
   getCurrentTime,
   insertPaddingPointsForChart,
   removeTrailingPaddingFromSeveritySegments,
   roundDateToInterval,
   roundTimestampToFiveMinutes,
 } from './utils';
-import { Incident } from './model';
+import { Alert, Incident } from './model';
 
 describe('getCurrentTime', () => {
   it('should return current time rounded down to 5-minute boundary', () => {
@@ -626,3 +627,146 @@ describe('removeTrailingPaddingFromSeveritySegments', () => {
     expect(original.values).toHaveLength(3);
   });
 });
+
+describe('createAlertsChartBars - padding boundary gap detection', () => {
+  const makeAlert = (
+    overrides: Partial<Alert> & { values: Array<[number, string]>; firstTimestamp: number },
+  ): Alert => ({
+    alertname: 'TestAlert',
+    alertsStartFiring: 0,
+    alertsEndFiring: 0,
+    alertstate: 'firing',
+    component: 'test-component',
+    layer: 'compute',
+    name: 'TestAlert',
+    namespace: 'test-ns',
+    resolved: false,
+    severity: 'critical',
+    silenced: false,
+    x: 1,
+    ...overrides,
+  });
+
+  it('should detect gap between alerts whose padded values would overlap', () => {
+    // Alert A: real data at [1000, 1300]. Padded: [700, 1000, 1300, 1600]
+    // Alert B: real data at [2200, 2500]. Padded: [1900, 2200, 2500, 2800]
+    // Real gap: 2200 - 1300 = 900s (15 min) — should be detected
+    // Without fix: padded timestamps 1600 and 1900 are only 300s (5 min) apart — no gap
+    const alertA = makeAlert({
+      values: [
+        [700, '2'],
+        [1000, '2'],
+        [1300, '2'],
+        [1600, '2'],
+      ],
+      firstTimestamp: 700,
+    });
+
+    const alertB = makeAlert({
+      values: [
+        [1900, '2'],
+        [2200, '2'],
+        [2500, '2'],
+        [2800, '2'],
+      ],
+      firstTimestamp: 1900,
+    });
+
+    const bars = createAlertsChartBars([alertA, alertB]);
+
+    const dataBars = bars.filter((b) => !b.nodata);
+    const nodataBars = bars.filter((b) => b.nodata);
+
+    expect(dataBars.length).toBe(2);
+    expect(nodataBars.length).toBe(1);
+  });
+
+  it('should merge alerts whose real data is within 5 minutes of each other', () => {
+    // Alert A: real data at [1000, 1300]. Padded: [700, 1000, 1300, 1600]
+    // Alert B: real data at [1500, 1800]. Padded: [1200, 1500, 1800, 2100]
+    // Real gap: 1500 - 1300 = 200s (3.3 min) — should merge
+    const alertA = makeAlert({
+      values: [
+        [700, '2'],
+        [1000, '2'],
+        [1300, '2'],
+        [1600, '2'],
+      ],
+      firstTimestamp: 700,
+    });
+
+    const alertB = makeAlert({
+      values: [
+        [1200, '2'],
+        [1500, '2'],
+        [1800, '2'],
+        [2100, '2'],
+      ],
+      firstTimestamp: 1200,
+    });
+
+    const bars = createAlertsChartBars([alertA, alertB]);
+
+    const dataBars = bars.filter((b) => !b.nodata);
+    expect(dataBars.length).toBe(1);
+  });
+
+  it('should handle alerts with only 2 values (single real point + padding)', () => {
+    // Alert with 2 values: [pad, real]. Only the last (real) value is used.
+    const alertA = makeAlert({
+      values: [
+        [700, '2'],
+        [1000, '2'],
+      ],
+      firstTimestamp: 700,
+    });
+
+    const alertB = makeAlert({
+      values: [
+        [1900, '2'],
+        [2200, '2'],
+      ],
+      firstTimestamp: 1900,
+    });
+
+    const bars = createAlertsChartBars([alertA, alertB]);
+
+    const dataBars = bars.filter((b) => !b.nodata);
+    const nodataBars = bars.filter((b) => b.nodata);
+
+    // Real gap: 2200 - 1000 = 1200s (20 min) > 5 min — should detect gap
+    expect(dataBars.length).toBe(2);
+    expect(nodataBars.length).toBe(1);
+  });
+
+  it('should preserve firstTimestamp per interval after gap detection', () => {
+    const alertA = makeAlert({
+      values: [
+        [700, '2'],
+        [1000, '2'],
+        [1300, '2'],
+        [1600, '2'],
+      ],
+      firstTimestamp: 500,
+    });
+
+    const alertB = makeAlert({
+      values: [
+        [1900, '2'],
+        [2200, '2'],
+        [2500, '2'],
+        [2800, '2'],
+      ],
+      firstTimestamp: 1700,
+    });
+
+    const bars = createAlertsChartBars([alertA, alertB]) as any[];
+
+    const dataBars = bars.filter((b) => !b.nodata);
+    expect(dataBars.length).toBe(2);
+
+    const startDates = dataBars.map((b) => b.startDate.getTime() / 1000);
+    expect(startDates).toContain(500);
+    expect(startDates).toContain(1700);
+  });
+});
diff --git a/web/src/components/Incidents/utils.ts b/web/src/components/Incidents/utils.ts
@@ -381,10 +381,14 @@ export const createIncidentsChartBars = (incidents: Incident[], dateArray: SpanD
 function consolidateAndMergeAlertIntervals(alerts: Array<Alert>): Array<AlertsIntervalsArray> {
   if (alerts.length === 0) return [];
 
-  // Tag each value with its source alert's firstTimestamp, then sort
-  const taggedValues = alerts.flatMap((alert) =>
-    alert.values.map((v) => ({ timestamp: v[0], firstTimestamp: alert.firstTimestamp })),
-  );
+  // Strip boundary padding (first/last values are ±300s padding from insertPaddingPointsForChart)
+  // so gap detection uses real data timestamps and doesn't merge across alert boundaries
+  const taggedValues = alerts.flatMap((alert) => {
+    const values = alert.values;
+    const coreValues =
+      values.length <= 2 ? values.slice(-1) : values.slice(1, -1);
+    return coreValues.map((v) => ({ timestamp: v[0], firstTimestamp: alert.firstTimestamp }));
+  });
 
   if (taggedValues.length === 0) return [];
 
