[SLO] Charts: derive window from type, no false-green on missing burn data, dataviz polish

[lezzago]

What & why

Derives the window from window.type (kills a hard-coded '30d'), renders a null/empty window as no_data instead of false-green, dashes the reference line, adds non-hue status encoding, UTC + delta-vs-target tooltips, and smooth:false over step data.

Findings: M1, M2, CLAR5, CLAR6, m1, m2, m3, m4.

Before / after

Before / after

Testing

chart/panel tests (classifyTier null-window, etc.). Centrally green. Note: the local stack has no burn data, so these data-dependent deltas are near-identical on screen — correctness is covered by jest and flagged for managed-env re-verify.

Review

Independent review agent: PASS — verified all eight fixes; refuted two out-of-scope observations.

Regression check

The before/after above is a full-viewport capture, so adjacent components on the same surface are visible and unchanged — the diff is scoped to what's called out. Cross-component safety is also verified centrally: this change is file-disjoint from the other in-flight audit fixes (no file overlap, so it merges cleanly with them), and the affected plugin test suites pass with 0 new type errors vs main. Aside from the merge-order note below, it can be reviewed, merged, and reverted independently.

Dependency

None.

Draft — see the merge-order note above.

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 99d87f0

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Don't classify Infinity burn as no-data isMissingWindow returns true for Infinity, so a legitimately huge burn rate (or Number.MAX_VALUE) would be classified as no_data instead of firing. Consider treating only NaN/null/undefined as missing, while allowing Infinity (or clamping it) so extreme-but-real burn spikes still surface as firing rather than silently going grey. public/components/apm/pages/slos/slo_burn_rate_panel.tsx [98-105] export function classifyTier( short: number | null, long: number | null, threshold: number ): TierHealth { // A missing window can never be coerced to 0 → "ok" → green. If either - // window is absent we genuinely don't know the tier's health. - if (isMissingWindow(short) || isMissingWindow(long)) return 'no_data'; + // window is absent we genuinely don't know the tier's health. Note we + // still accept ±Infinity as a real (extreme) burn value. + const isMissing = (v: number | null | undefined) => + v === null || v === undefined || Number.isNaN(v); + if (isMissing(short) || isMissing(long)) return 'no_data'; Suggestion importance[1-10]: 5 __ Why: Valid point that Number.isFinite excludes Infinity, which could theoretically represent an extreme burn rate. However, in practice burn-rate values from PromQL are unlikely to be literal Infinity, so the impact is limited but the reasoning is sound.	Low
General	Normalize severity label in markLine The markLine label formatter interpolates warningThreshold.severity directly into a string that ECharts may render as rich text — and, more importantly, it bypasses the severityLabel mapping used elsewhere in the UI, so labels here read lowercase ("warning") while the burn panel reads title-case ("Warning"). Route the severity through severityLabel for consistency and escape it defensively. public/components/apm/pages/slos/slo_budget_remaining_chart.tsx [173-175] -formatter: `${warningThreshold.severity} @ ${formatPct(warningThreshold.threshold, { - decimals: SLO_PRECISION.budget, -})}`, +formatter: `${severityLabel(warningThreshold.severity)} @ ${formatPct( + warningThreshold.threshold, + { decimals: SLO_PRECISION.budget } +)}`, Suggestion importance[1-10]: 5 __ Why: Legitimate consistency concern: the burn panel uses severityLabel for title-case rendering while the markLine label uses the raw lowercase severity. However, severityLabel is defined in a different file and would need to be imported.	Low
	Guard against duplicate tier labels Keying thresholdByName on t.label (a translated/user-facing string) is fragile: two tiers sharing a label collide silently, and any label change breaks the tooltip's delta lookup. Prefer keying on a stable identifier (e.g., severity + multiplier) and match by that in the tooltip formatter, or pass the threshold through as a datum on the series. public/components/apm/pages/slos/slo_burn_rate_chart.tsx [112-116] const { tiers } = inputs; const tz = inputs.tz ?? resolveDisplayTz(); -// Series name → threshold multiplier, so the tooltip can show how far each -// tier's current burn sits above/below the threshold that would fire it. -const thresholdByName = new Map(tiers.map((t) => [t.label, t.multiplier])); +// Map series name → threshold. Duplicate labels would collide, so callers +// must ensure tier labels are unique per chart (enforced by tier config). +const thresholdByName = new Map<string, number>(); +tiers.forEach((t) => { + if (!thresholdByName.has(t.label)) thresholdByName.set(t.label, t.multiplier); +}); Suggestion importance[1-10]: 3 __ Why: Duplicate labels are unlikely in practice, and the suggested improved_code doesn't materially fix the underlying fragility (still keys on label). The suggestion is mostly a minor defensive tweak.	Low

---

Previous suggestions

Suggestions up to commit 652f47c

Category	Suggestion	Impact
General	Avoid masking infinite burn as no-data isMissingWindow currently accepts Infinity values as missing because Number.isFinite(Infinity) is false, but the test also asserts classifyTier(Infinity, 0.02, threshold) returns 'no_data'. This behavior is correct, but note that a legitimate very large burn rate could theoretically be reported as Infinity by PromQL (division by zero). Consider treating +Infinity explicitly as a firing signal rather than lumping it with no_data, so a division-by-zero burn does not silently mask an active incident. public/components/apm/pages/slos/slo_burn_rate_panel.tsx [98-111] export function classifyTier( short: number | null, long: number | null, threshold: number ): TierHealth { - // A missing window can never be coerced to 0 → "ok" → green. If either - // window is absent we genuinely don't know the tier's health. - if (isMissingWindow(short) || isMissingWindow(long)) return 'no_data'; - const s = short as number; - const l = long as number; + // Treat +Infinity (e.g. divide-by-zero in PromQL) as an extreme burn, not + // as missing data — otherwise a real incident could be masked as no_data. + const normalize = (v: number | null | undefined): number | null => { + if (v === null || v === undefined || Number.isNaN(v)) return null; + if (v === Infinity) return Number.MAX_VALUE; + if (!Number.isFinite(v)) return null; + return v; + }; + const s = normalize(short); + const l = normalize(long); + if (s === null || l === null) return 'no_data'; if (s > threshold && l > threshold) return 'firing'; if (s > threshold || l > threshold) return 'at_risk'; return 'ok'; } Suggestion importance[1-10]: 5 __ Why: Reasonable concern that Infinity (from PromQL divide-by-zero) shouldn't be treated as no_data, but the PR's tests explicitly assert Infinity maps to no_data, so applying this change would break tests. The suggestion has some merit for correctness but contradicts intended behavior.	Low
Possible issue	Guard against unknown calendar periods deriveWindowDuration will return undefined (typed as string) if a future CalendarWindow['period'] value is added that's not in the map, which would silently produce an invalid PromQL range like rate(...[undefined]). Add a defensive fallback and/or exhaustiveness check so an unknown period fails loudly or safely defaults, rather than emitting a malformed query. public/components/apm/pages/slos/slo_budget_remaining_chart.tsx [61-70] const CALENDAR_PERIOD_DURATIONS: Record<CalendarWindow['period'], string> = { week: '7d', month: '30d', quarter: '90d', }; /** Resolve the PromQL range duration the chart should query for this window. */ export function deriveWindowDuration(window: Window): string { - return window.type === 'rolling' ? window.duration : CALENDAR_PERIOD_DURATIONS[window.period]; + if (window.type === 'rolling') return window.duration; + const mapped = CALENDAR_PERIOD_DURATIONS[window.period]; + if (!mapped) { + // Unknown calendar period — fall back to a safe rolling range rather + // than emit `rate(...[undefined])`. + return '30d'; + } + return mapped; } Suggestion importance[1-10]: 3 __ Why: Minor defensive improvement; since CalendarWindow['period'] is typed, TypeScript should enforce exhaustiveness, and the risk of an undefined value is low. The fallback adds marginal safety.	Low

Suggestions up to commit c7decbe

Category	Suggestion	Impact
Security	Escape all values interpolated into tooltip HTML The delta span interpolates formatMultiplier(...) directly into HTML without escaping. While formatMultiplier currently returns numeric strings, the rest of this tooltip carefully escapes all interpolated values via escapeHtml for defense-in-depth. Apply the same treatment here to keep the invariant consistent and prevent future regressions. public/components/apm/pages/slos/slo_burn_rate_chart.tsx [175-183] deltaStr = delta >= 0 - ? ` <span style="color:${euiThemeVars.euiColorDanger};">(${formatMultiplier( - delta - )} over)</span>` - : ` <span style="color:${euiThemeVars.euiColorDarkShade};">(${formatMultiplier( - Math.abs(delta) - )} under)</span>`; + ? ` <span style="color:${escapeHtml( + euiThemeVars.euiColorDanger + )};">(${escapeHtml(formatMultiplier(delta))} over)</span>` + : ` <span style="color:${escapeHtml( + euiThemeVars.euiColorDarkShade + )};">(${escapeHtml(formatMultiplier(Math.abs(delta)))} under)</span>`; Suggestion importance[1-10]: 4 __ Why: Valid defense-in-depth suggestion for consistency with the rest of the tooltip's escaping pattern, though formatMultiplier only produces numeric strings so the actual security risk is minimal.	Low
General	Avoid classifying Infinity as missing data isMissingWindow uses !Number.isFinite(value) which also returns true for Infinity, but the test classifyTier(Infinity, 0.02, threshold) expects 'no_data' — this works, but any legitimate very-large burn should not be reclassified as missing. Consider treating only NaN/null/undefined as missing while allowing Infinity through (or explicitly documenting Infinity as unreliable), so a genuinely huge burn isn't silently hidden as "no data". public/components/apm/pages/slos/slo_burn_rate_panel.tsx [94-96] -export function classifyTier( - short: number | null, - long: number | null, - threshold: number -): TierHealth { - // A missing window can never be coerced to 0 → "ok" → green. If either - // window is absent we genuinely don't know the tier's health. - if (isMissingWindow(short) || isMissingWindow(long)) return 'no_data'; - const s = short as number; - const l = long as number; - if (s > threshold && l > threshold) return 'firing'; - if (s > threshold || l > threshold) return 'at_risk'; - return 'ok'; +function isMissingWindow(value: number | null | undefined): boolean { + return value === null || value === undefined || Number.isNaN(value as number); } Suggestion importance[1-10]: 3 __ Why: The suggestion contradicts the tests in the PR which explicitly expect Infinity to be classified as no_data. Changing the behavior would break the tests, though the observation about hiding a legitimately huge burn has some merit as a design consideration.	Low

Suggestions up to commit 7dbee1a

Category	Suggestion	Impact
General	Widen parameter types to match contract The test expect(classifyTier(Infinity, 0.02, threshold)).toBe('no_data') will fail because Number.isFinite(Infinity) is false — wait, that returns false, so isMissingWindow(Infinity) returns true and this works. However, the type signature declares short: number | null but the test passes undefined and NaN; Number.isFinite handles both correctly, so behavior is fine. Consider widening the parameter type to number | null | undefined to match the documented contract and tested inputs. public/components/apm/pages/slos/slo_burn_rate_panel.tsx [98-111] export function classifyTier( - short: number | null, - long: number | null, + short: number | null | undefined, + long: number | null | undefined, threshold: number ): TierHealth { - // A missing window can never be coerced to 0 → "ok" → green. If either - // window is absent we genuinely don't know the tier's health. if (isMissingWindow(short) || isMissingWindow(long)) return 'no_data'; const s = short as number; const l = long as number; if (s > threshold && l > threshold) return 'firing'; if (s > threshold || l > threshold) return 'at_risk'; return 'ok'; } Suggestion importance[1-10]: 4 __ Why: Widening the parameter types to number | null | undefined aligns the signature with the tested inputs and documented contract, a minor type-safety improvement.	Low
	Document calendar-window approximation drift Approximating a calendar month as a fixed 30d and a calendar quarter as 90d will drift from the actual calendar boundaries (months are 28–31 days, quarters ~90–92 days). The description copy claims this is "approximated over a rolling {window} range," which is accurate, but the value shown may misalign with the user's actual calendar window by several days. Consider documenting this drift in the constant's JSDoc, or computing the range from the current calendar period's start to now for tighter accuracy. public/components/apm/pages/slos/slo_budget_remaining_chart.tsx [49-53] +/** + * Approximate range for each calendar period, used only until calendar-window + * recording rules land. Note: month/quarter are nominal — actual calendar + * months are 28–31d and quarters ~90–92d, so the rolling range can drift by + * a few days from the true calendar boundary. + */ const CALENDAR_PERIOD_DURATIONS: Record<CalendarWindow['period'], string> = { week: '7d', month: '30d', quarter: '90d', }; Suggestion importance[1-10]: 3 __ Why: Documentation-only suggestion noting the drift in the approximation; the code already handles this with the surrounding context, so impact is low.	Low

Suggestions up to commit 6957798

Category	Suggestion	Impact
General	Don't treat Infinity as missing data isMissingWindow treats Infinity/-Infinity as missing via !Number.isFinite, but the test expect(classifyTier(Infinity, 0.02, threshold)).toBe('no_data') will pass while conceptually an infinite burn rate should be treated as firing rather than no_data. Consider distinguishing "not a real number" (NaN/null/undefined → no_data) from "±Infinity" (treat as an extreme value → firing) to avoid silently hiding a genuinely exploding burn rate as a grey "no data" pill. public/components/apm/pages/slos/slo_burn_rate_panel.tsx [98-111] export function classifyTier( short: number | null, long: number | null, threshold: number ): TierHealth { - // A missing window can never be coerced to 0 → "ok" → green. If either - // window is absent we genuinely don't know the tier's health. - if (isMissingWindow(short) || isMissingWindow(long)) return 'no_data'; + const isUnknown = (v: number | null | undefined) => + v === null || v === undefined || Number.isNaN(v as number); + if (isUnknown(short) || isUnknown(long)) return 'no_data'; const s = short as number; const l = long as number; if (s > threshold && l > threshold) return 'firing'; if (s > threshold || l > threshold) return 'at_risk'; return 'ok'; } Suggestion importance[1-10]: 4 __ Why: Reasonable semantic point that Infinity might represent an extreme burn rather than missing data, but this contradicts explicit test expectations in the PR (expect(classifyTier(Infinity, 0.02, threshold)).toBe('no_data')), so applying it would break tests. The impact is minor since Infinity is unlikely in real PromQL results.	Low
	Guard against unknown calendar periods deriveWindowDuration assumes window.period is always one of the three known keys, but Window is a discriminated union — if a future calendar period (e.g. year) is added to the type, this returns undefined and the PromQL query silently becomes invalid. Add a defensive fallback so an unknown period doesn't produce a broken PromQL range like rate(...[undefined]). public/components/apm/pages/slos/slo_budget_remaining_chart.tsx [49-58] -const CALENDAR_PERIOD_DURATIONS: Record<CalendarWindow['period'], string> = { - week: '7d', - month: '30d', - quarter: '90d', -}; - -/** Resolve the PromQL range duration the chart should query for this window. */ export function deriveWindowDuration(window: Window): string { - return window.type === 'rolling' ? window.duration : CALENDAR_PERIOD_DURATIONS[window.period]; + if (window.type === 'rolling') return window.duration; + return CALENDAR_PERIOD_DURATIONS[window.period] ?? '30d'; } Suggestion importance[1-10]: 4 __ Why: A minor defensive improvement for future-proofing against additions to the CalendarWindow['period'] union. Currently TypeScript's exhaustive typing prevents this, so the practical impact is low.	Low

Suggestions up to commit 4314a73

Category	Suggestion	Impact
General	Fallback for unknown calendar periods If window.type is calendar but window.period is an unknown/new value, the lookup returns undefined and gets fed to PromQL as a range duration, producing an invalid query. Add a defensive fallback so unexpected periods degrade gracefully instead of breaking the chart. public/components/apm/pages/slos/slo_budget_remaining_chart.tsx [49-60] const CALENDAR_PERIOD_DURATIONS: Record<CalendarWindow['period'], string> = { week: '7d', month: '30d', quarter: '90d', }; /** Resolve the PromQL range duration the chart should query for this window. */ export function deriveWindowDuration(window: Window): string { - return window.type === 'rolling' - ? window.duration - : CALENDAR_PERIOD_DURATIONS[window.period]; + if (window.type === 'rolling') return window.duration; + return CALENDAR_PERIOD_DURATIONS[window.period] ?? '30d'; } Suggestion importance[1-10]: 5 __ Why: Since CalendarWindow['period'] is a typed union, unknown periods shouldn't occur at compile time, but a runtime fallback adds resilience against untyped/malformed input. Modest defensive improvement.	Low
	Guard tooltip lookup against duplicate labels Building the threshold lookup by t.label will collide silently if two tiers share the same label (e.g. duplicated severities), and any tier whose ECharts series name differs from label will not show a delta. Consider keying on a stable identifier or ensuring labels are guaranteed unique upstream. public/components/apm/pages/slos/slo_burn_rate_chart.tsx [104] -const thresholdByName = new Map(tiers.map((t) => [t.label, t.multiplier])); +const thresholdByName = new Map<string, number>(); +tiers.forEach((t) => { + if (!thresholdByName.has(t.label)) thresholdByName.set(t.label, t.multiplier); +}); Suggestion importance[1-10]: 3 __ Why: Duplicate labels in tiers are unlikely in practice since each tier has a distinct severity/multiplier, and the suggested fix doesn't meaningfully change behavior for the common case. Minor defensive improvement.	Low

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 99d87f0)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Sparkline polish (smooth:false, budget precision) Relevant files: public/components/apm/pages/services_home/slo_budget_sparkline.tsx public/components/apm/pages/services_home/tests/slo_budget_sparkline.test.tsx Sub-PR theme: Burn-rate panel — no-data classification and severity labels Relevant files: public/components/apm/pages/slos/slo_burn_rate_panel.tsx public/components/apm/pages/slos/tests/slo_burn_rate_panel.test.tsx Sub-PR theme: Budget/burn-rate chart dataviz (window derivation, tooltips, dashed refs) Relevant files: public/components/apm/pages/slos/slo_budget_remaining_chart.tsx public/components/apm/pages/slos/slo_burn_rate_chart.tsx public/components/apm/pages/slos/tests/slo_budget_remaining_chart.test.tsx public/components/apm/pages/slos/tests/slo_burn_rate_chart.test.tsx

⚡ Recommended focus areas for review Variable shadowing The useMemo result is assigned to a local named window, which shadows the global window object within the component scope. While it appears to work in the current code, any later addition that references browser window (e.g., window.location, window.matchMedia) inside this component will silently resolve to the string duration instead. Rename to windowDuration or similar to prevent latent bugs. const window = useMemo(() => deriveWindowDuration(slo.spec.window), [slo.spec.window]); const query = useMemo( () => buildBudgetRemainingExpr(slo, objective, window), [slo, objective, window] ); Possible Issue thresholdByName is keyed by tier.label, but tooltip lookup uses p.seriesName. If the series is registered in the ECharts spec with a name that differs from tier.label (e.g., includes the multiplier suffix or severity prefix), thresholdByName.get(p.seriesName) will return undefined and the "X over/under" delta will silently disappear. Verify the series name used when constructing the chart series matches tier.label exactly. const thresholdByName = new Map(tiers.map((t) => [t.label, t.multiplier]));

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 7dbee1a.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

Path	Line	Severity	Description
public/components/apm/pages/slos/slo_burn_rate_chart.tsx	167	medium	ECharts tooltip formatter directly interpolates `p.color` into an inline CSS style attribute and `p.seriesName` into HTML without sanitization. If these values are ever derived from user-controlled or externally-sourced data (e.g. SLO names, label values from Prometheus), this creates a stored XSS vector in the tooltip rendering path.
public/components/apm/pages/slos/slo_budget_remaining_chart.tsx	177	medium	ECharts tooltip formatter constructs raw HTML by interpolating `ts` (timezone-formatted timestamp) and i18n-translated strings with embedded `formatPct` values directly into the returned HTML string. The `deltaLine` block also injects formatted values without escaping. If any upstream value contains angle brackets or script content, the tooltip renderer will execute it.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 2 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

Persistent review updated to latest commit 7dbee1a

[github-actions]

Persistent review updated to latest commit c7decbe

[github-actions]

Persistent review updated to latest commit 652f47c

[lezzago]

Cross-referencing @TackAdam's precision finding on #2836 (item 1): the budget-remaining chart tooltip/axis/threshold label and the budget sparkline live in this PR's files, so the fix landed here in 99d87f07 — they now render via formatPct(v, { decimals: SLO_PRECISION.budget }) (2 decimals), matching slo_budget_panel.tsx. Tests updated for the 2-decimal output.

[github-actions]

Persistent review updated to latest commit 99d87f0