[SLO] Localize percent formatting; make SLO_PRECISION the single precision policy

[lezzago]

What & why

Localizes the % suffix and the — fallback and makes SLO_PRECISION the single source of precision. en-US output is unchanged for SLO value ranges (|value| < 10, i.e. percentages < 1000%, which covers all real SLO data); the change surfaces under non-en locales (localized grouping/decimal separators) and adds a grouping comma only for percentages ≥ 1000%.

Findings: CLAR10, CLAR3.

Before / after

Before / after (SLO listing; identical under en-US by design — see note)

Flow

Testing

format.test.ts incl. a de-locale assertion. Note: no visible change under en-US.

Review

Independent review agent: APPROVE — SLO_PRECISION values byte-identical to main; i18n id unique.

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

No dependencies — can merge independently. Consumers migrate to SLO_PRECISION in #2830 and #2833.

Draft — see the merge-order note above.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit ef0e16b)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

Persistent review updated to latest commit 72a23e7

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to ef0e16b
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Clamp decimals to avoid RangeError Intl.NumberFormat throws a RangeError when decimals is negative or above 20, or when it's non-integer/NaN. Since decimals comes from caller options and is later cached, a single bad call can also poison the cache with an entry that never gets used. Clamp/validate decimals to a safe integer range before constructing the formatter. common/slo/format.ts [37-50] const percentFormatterCache = new Map<string, Intl.NumberFormat>(); function getPercentFormatter(decimals: number): Intl.NumberFormat { + const safeDecimals = Math.min( + 20, + Math.max(0, Number.isFinite(decimals) ? Math.trunc(decimals) : 0) + ); const locale = i18n.getLocale(); - const key = `${locale}:${decimals}`; + const key = `${locale}:${safeDecimals}`; let formatter = percentFormatterCache.get(key); if (!formatter) { formatter = new Intl.NumberFormat(locale, { style: 'percent', - minimumFractionDigits: decimals, - maximumFractionDigits: decimals, + minimumFractionDigits: safeDecimals, + maximumFractionDigits: safeDecimals, }); percentFormatterCache.set(key, formatter); } return formatter; } Suggestion importance[1-10]: 4 __ Why: Valid defensive point: Intl.NumberFormat throws for out-of-range or non-integer decimals, and callers control this value. However, callers within this codebase pass known safe integer values (e.g., SLO_PRECISION keys), so impact is limited to hardening against misuse.	Low

---

Previous suggestions

Suggestions up to commit 72a23e7

Category	Suggestion	Impact
General	Resolve fallback translation at call time i18n.translate is evaluated at module load, so EMPTY_VALUE_FALLBACK captures the locale active at import time and will not update if the locale changes later (e.g., in tests that call i18n.setLocale). Convert it to a getter/function so translation resolves at call time, matching formatPct's runtime i18n.getLocale() behavior. common/slo/format.ts [21-23] -export const EMPTY_VALUE_FALLBACK = i18n.translate('observability.slo.format.emptyValue', { - defaultMessage: '—', -}); +export const getEmptyValueFallback = () => + i18n.translate('observability.slo.format.emptyValue', { + defaultMessage: '—', + }); +export const EMPTY_VALUE_FALLBACK = getEmptyValueFallback(); Suggestion importance[1-10]: 4 __ Why: The concern about module-load-time evaluation is valid in principle, but since the defaultMessage is an em dash likely unchanged across locales, and the suggested improved_code still evaluates EMPTY_VALUE_FALLBACK at module load (defeating its own point), the impact is marginal and the fix is incomplete.	Low

[github-actions]

Persistent review updated to latest commit 3a530ba

[TackAdam]

Worth addressing

1. The precision policy isn't actually consistent yet — budget-remaining still renders at two different precisions — slo_budget_remaining_chart.tsx:132,156 & slo_budget_sparkline.tsx:93

   These call formatPct(v) with no decimals, so they get the default 1, while slo_budget_panel.tsx:500 renders the same budget-remaining value at SLO_PRECISION.budget = 2.

   So 0.5 reads 50.0% in the chart tooltip/sparkline but 50.00% in the panel — the exact "four different precisions" inconsistency the new SLO_PRECISION doc says "every surface must read from this map" to eliminate.

   The PR body says consumers migrate in 2830/2833, but those cover the listing/overview — these budget-chart surfaces don't obviously fall under either. Worth confirming they're in a migration PR, or fixing here.

2. EMPTY_VALUE_FALLBACK is introduced but almost nothing uses it (CLAR10 largely unrealized) — format.ts:21

   Dozens of hardcoded '—' literals remain across the SLO surfaces, and two callers (slo_detail_page.tsx:148, probe_sli_panel.tsx:327) pass a literal '—' as the null path rather than the new constant.

   A translator swapping observability.slo.format.emptyValue changes almost nothing today. Fine as a foundation if adoption lands in a follow-up — but flag it so the constant doesn't sit dead.

Nits

3. "en-US byte-for-byte unchanged" is slightly overstated — format.ts

   It's true for |value| < 10 (percentages < 1000%), which covers all real SLO data — but formatPct(12.3456) now gains a grouping comma (1,234.56%), and non-en locales get a comma decimal + NBSP-before-%.

   The author already pins this in the test (:42-44), so it's known; just tighten the PR wording to "unchanged for SLO value ranges."

4. EMPTY_VALUE_FALLBACK is frozen at module import — format.ts:21

   i18n.translate(...) runs once at import, so a runtime setLocale (or a translation registered after this module loads) won't be reflected.

   Same pattern as the theme-token-at-load note from 2826; low risk since the em-dash defaultMessage is locale-agnostic anyway.

5. Per-call Intl.NumberFormat allocation — format.ts:40

   A new formatter is constructed on every call, and formatPct is wired into ECharts axis/tooltip formatters (slo_budget_remaining_chart.tsx:156) that fire on every render/hover.

   Intl.NumberFormat construction is markedly heavier than the old toFixed. A small memo keyed on (locale, decimals) would erase the regression. Low priority.

6. de test assumes full ICU — format.test.ts:66

   Passes here and in CI (Node 22 ships full ICU), but under a small-ICU build de falls back to en and the assertions fail.

   Consider guarding or noting the ICU dependency.

[github-actions]

Persistent review updated to latest commit ef0e16b

[lezzago]

Thanks @TackAdam:

1. Precision not consistent (budget-remaining chart + sparkline) ✅ — those files are owned by [SLO] Charts: derive window from type, no false-green on missing burn data, dataviz polish #2832 (charts), so I fixed it there (commit 99d87f07): the budget-remaining tooltip/axis/threshold label and the budget sparkline now render via formatPct(v, { decimals: SLO_PRECISION.budget }), matching the budget panel. So 0.5 reads 50.00% everywhere. Kept out of this PR to preserve file-disjointness.
2. "en-US byte-for-byte unchanged" overstated ✅ — tightened the PR body to "unchanged for SLO value ranges (|value| < 10)" and noted the grouping comma only appears for percentages ≥ 1000%.
3. Per-call Intl.NumberFormat allocation ✅ — memoized the formatter per (locale, decimals) in format.ts (commit ef0e16b6); keying on locale keeps a runtime setLocale correct.
4. de test assumes full ICU ✅ — the test now detects real de support (comma separator) and skips the divergence assertions on a small-ICU build; CI's full-ICU Node 22 still exercises the real path.

2 & 4. EMPTY_VALUE_FALLBACK adoption / module-load freeze — acknowledged as foundation: broad migration of the remaining hardcoded '—' literals is a follow-up (touches many files across other PRs' surfaces), and the module-load freeze is low-risk since the em-dash defaultMessage is locale-agnostic. Flagged so the constant doesn't sit dead.

[ps48]

p2: the precision-policy doc tells callers to pass burnRate through formatPct, which misrenders

The added policy comment says to pass the relevant SLO_PRECISION key as formatPct's decimals, and lists burnRate (a 3.2x multiplier). But formatPct forces percent style and multiplies by 100, so formatPct(3.2, { decimals: SLO_PRECISION.burnRate }) renders 320.0% instead of 3.2x. Could we scope the 'pass to formatPct' instruction to the percent surfaces, and note that burnRate (and eventsRatio if ever shown as a raw ratio) is a precision-only key formatted elsewhere?

[ps48]

slo percent formatting review

Verified at head ef0e16b, scoped to common/slo/format.ts and its new test. formatPct moves to Intl.NumberFormat percent style with the raw ratio and pinned fraction digits (no double-multiply), EMPTY_VALUE_FALLBACK is an i18n-translatable glyph, and SLO_PRECISION is the single precision policy, all correctly kept in common/ where server and client share them. The per-locale:decimals formatter cache is a sensible bounded perf guard. Tests cover en defaults, fixed precision, the no-double-multiply invariant, grouping, non-finite fallback, and a real de-locale divergence. No new deps, routes, console, or dead code.

One small doc nit inline. No p0/p1.