Repository navigation
feat(observability): Prometheus alert rules + SLO definitions (PR-008) - #5018
KooshaPari wants to merge 50 commits into
Conversation
…C+D) (diegosouzapw#4622) C — scripts/quality/validate-release-green.mjs (npm run check:release-green): reproduces the release-equivalent validation (typecheck, eslint, db-rules, public-creds, full unit, vitest, ratchets, optional --with-build package-artifact) against the current working tree and classifies each red as HARD (real defect, exit 1) vs DRIFT (ratchet — reported, never affects exit / never blocks). Pure helpers exported + orchestration behind a direct-run guard; unit-tested. D — .github/workflows/nightly-release-green.yml: runs C on the active release branch nightly (and on workflow_dispatch) and opens/updates a single tracking issue on HARD failures. Never a required check, never touches a contributor PR. Closes the gap where the full gate (ci.yml) only ran on the release PR, so reds accrued silently on release/** and surfaced in 40-min layers at release time. Non-blocking by construction; drift is the maintainer's to rebaseline at release. Co-authored-by: Diego Rodrigues de Sa e Souza <diego.souza@cdwasolutions.com.br>
Integrated into release/v3.8.34
Integrated into release/v3.8.34
Integrated into release/v3.8.34
…#4599) Integrated into release/v3.8.34
…ative gate (diegosouzapw#4527) Integrated into release/v3.8.34 (supersedes diegosouzapw#4445)
Integrated into release/v3.8.34
…iegosouzapw#4564) Integrated into release/v3.8.34 (rebuilt onto tip)
…osouzapw#4561) Integrated into release/v3.8.34 (rebuilt onto tip)
…9router#951) (diegosouzapw#4566) Integrated into release/v3.8.34 (rebuilt onto tip)
…uzapw#1043) (diegosouzapw#4553) Integrated into release/v3.8.34 (rebuilt onto tip)
…ect fails (diegosouzapw#4562) Integrated into release/v3.8.34 (rebuilt onto tip)
…gosouzapw#4555) Integrated into release/v3.8.34 (rebuilt onto tip)
Integrated into release/v3.8.34 (rebuilt onto tip)
…w#4588) Integrated into release/v3.8.34 (rebuilt — bundled commits stripped)
Integrated into release/v3.8.34 (rebuilt — bundled commits stripped)
…pw#4590) Integrated into release/v3.8.34 (rebuilt — bundled commits stripped)
…s=0 (diegosouzapw#4615) Integrated into release/v3.8.34 (rebuilt onto tip)
…ouzapw#4525) Integrated into release/v3.8.34 (rebuilt — kept only the prefix rename, dropped stale-base reverts)
Integrated into release/v3.8.34 (rebuilt onto tip; merge before diegosouzapw#4612)
Integrated into release/v3.8.34 (rebuilt + reconciled with diegosouzapw#4593)
…e 3) (diegosouzapw#4645) * docs(compression): Phase 3 per-request header design spec Approved brainstorming output for the x-omniroute-compression header: header-first precedence, name-first combo matching (Decision A), explicit value bypasses auto-trigger (Decision B), DerivedPlan.source, and the X-OmniRoute-Compression response header. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(compression): Phase 3 per-request header implementation plan 4-task TDD plan (resolver header-first + source, parser, chatCore wiring + response header, docs/file-size) with full code and exact commands. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(compression): header-first resolver + plan source (Phase 3 core) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(compression): resolveCompressionHeader parser (Phase 3) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(compression): wire x-omniroute-compression header + response header (Phase 3) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(compression): extract plan-resolution leaf (planResolution.ts) under size cap (Phase 3) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(compression): document x-omniroute-compression header (Phase 3) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(compression): harden named-combo map + trim engine: header id (Phase 3 review) Addresses gemini-code-assist review on diegosouzapw#4645: - Extract buildNamedComboLookup (pure) so a blank/whitespace/null combo name contributes only its id key (no '' key, no throw that disables all combos). - Trim the engine:<id> header value so 'engine: rtk' resolves. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Diego Rodrigues de Sa e Souza <diego.souza@cdwasolutions.com.br> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Diego Rodrigues de Sa e Souza <souzamiriamrodrigues790@gmail.com>
Integrated into release/v3.8.34 (rebuilt + opt-in gate fix)
Integrated into release/v3.8.34 (rebuilt + test added)
Integrated into release/v3.8.34 (rebuilt + jsdom test)
…iegosouzapw#4606) Integrated into release/v3.8.34 (rebuilt + jsdom test)
…diegosouzapw#4563) Integrated into release/v3.8.34 (rebuilt onto tip; UI-cosmetic per owner)
…(extracted from diegosouzapw#4428) (diegosouzapw#4691) Integrated into release/v3.8.34 (cleanup core extracted from diegosouzapw#4428, credit @oyi77)
diegosouzapw#4674) (diegosouzapw#4677) Co-authored-by: Diego Rodrigues de Sa e Souza <souzamiriamrodrigues790@gmail.com>
Integrated into release/v3.8.34
…text retention (diegosouzapw#4644) Integrated into release/v3.8.34
…gosouzapw#4638) Integrated into release/v3.8.34 (authored by Rahul Sharma, AI co-author trailer stripped per project policy)
…cher (diegosouzapw#4580) (diegosouzapw#4684) Co-authored-by: Diego Rodrigues de Sa e Souza <souzamiriamrodrigues790@gmail.com>
…iegosouzapw#4692) Integrated into release/v3.8.34
…sh + best-free template (diegosouzapw#4621) Integrated into release/v3.8.34 (dead getFirstRegistryModelId dropped, rebuilt onto tip)
) (diegosouzapw#4618) Integrated into release/v3.8.34 (adapted onto diegosouzapw#4606's extracted topology section: default-hidden flip + enabled gate on useLiveDashboard)
…egosouzapw#4597) (diegosouzapw#4619) Integrated into release/v3.8.34 (data-dir.mjs refactor reconciled with diegosouzapw#4607; loadEnvFile aligned to getDefaultDataDir)
…eepseek-web.ts 1117->1125) (diegosouzapw#4695) file-size reconcile for diegosouzapw#4644
…#4642) Integrated into release/v3.8.34 (Ollama Cloud + OpenCode Go dashboard quota scraping; rebuilt onto tip, gates green: typecheck/public-creds/file-size/lint/docs-sync + 31 tests)
…iegosouzapw#4042) (diegosouzapw#4696) Land M365 pure modules ahead of draft diegosouzapw#4400
…diegosouzapw#4697) Incorporates Dependabot diegosouzapw#4667 + diegosouzapw#4668 + js-yaml v5 ESM migration into release/v3.8.34
…#4699) Integrated into release/v3.8.34 (noAuth in NOAUTH_PROVIDERS dynamic check + remove misrouted kimi web alias; 9 tests)
…files (diegosouzapw#4609) Integrated into release/v3.8.34 (extraction completed: added missing imports/exports per module, main imports handlers locally; 145 image-gen tests pass, typecheck/cycles/file-size green)
…elease-green reds - Finalize CHANGELOG [3.8.34] (43 bullets, full contributor attribution) + seed i18n mirrors - Rebaseline inherited cycle drift surfaced by release-green pre-flight: eslint warnings 3900->3907, cognitive-complexity 797->801 (release-finalize touches no prod code; all drift is from this cycle's contributor merges) - fix(providers): keep reka-flash-3 as the Reka provider default. diegosouzapw#4621 inserted reka-flash at the head of the model list, silently changing the default from reka-flash-3 (the free-tier model) to reka-flash; reorder so reka-flash-3 stays default, reka-flash retained. - test: align provider-models-config / provider-models-route / web-cookie-providers-new with diegosouzapw#4621 (reka-flash now in the Reka catalog) and diegosouzapw#4699 (the `kimi` API-key provider correctly falls through to DefaultExecutor instead of KimiWebExecutor) - chore(quality): allowlist the COMPRESSION_GUIDE doc name in check-fabricated-docs (false-positive env-var match; docs/compression/COMPRESSION_GUIDE.md exists)
Surfaced only on the release PR (these gates don't run on PR->release fast-gates): - fix(quota): complete HTML-comment sanitization in opencodeOllamaUsage SSR reset-time parsing — strip any <!--...--> generically instead of the two literal React hydration markers, so no partial "<!--" can survive (CodeQL js/incomplete-multi-character- sanitization, HIGH, introduced by diegosouzapw#4642). Regression test added. - test(codex): correct the Codex-fingerprint body key order assertion to match the canonical bodyFieldOrder (prompt_cache_key precedes include); diegosouzapw#4584 flipped the two and integration tests don't run on fast-gates so it never executed until the release PR. - chore(quality): rebaseline inherited cycle drift surfaced by full CI — zizmorFindings 152->155 (+3 unpinned-uses in nightly-release-green.yml from diegosouzapw#4622, same @vn convention as ci.yml) and openapiCoverage.pct 38.4->37.8 (-0.6, contributor routes added faster than openapi docs). Release-finalize touches no prod routes.
…ity drift - fix(quota): handle unterminated HTML comments in opencodeOllamaUsage SSR reset-time parsing — the `(?:-->|$)` arm consumes a trailing "<!--" with no closing "-->", so no partial "<!--" can survive (CodeQL js/incomplete-multi-character-sanitization persisted with the plain <!--...--> form because an unclosed comment could still leave "<!--"). - chore(quality): rebaseline cyclomatic complexity 1915->1916 (+1) — inherited v3.8.34 cycle drift (contributor feature branches); check:complexity does not run on PR->release fast-gates so it surfaced only on the release PR. Release-finalize adds 0 complexity (measured 1916 with/without the regex tweak). dead-code/cognitive/type-coverage/ compression-budget/codeql ratchets all pass.
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive, dependency-free observability stack for OmniRoute, featuring OpenTelemetry tracing, Prometheus metrics, structured logging, auto-instrumentation helpers, and Prometheus alert/recording rules with accompanying runbooks and validation scripts. The code review identified several critical issues that must be addressed: broken trace context propagation where child spans fail to inherit parent trace IDs; a missing end() method on the Span interface and stub spans; a bug in measureEventLoopLagMs that leaves the event loop lag metric static after startup; multiple TypeScript compilation errors from passing an unaccepted durationMs property to metrics functions; potential application crashes in the logger when serializing circular references or BigInt values; compilation failures in unit tests due to importing non-existent functions and using incorrect signatures; and logical flaws in Prometheus alert rules that could mask single-provider outages or trigger false positives during horizontal scaling.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| const traceId = generateId(16); | ||
| const spanId = generateId(8); | ||
| const parent = activeSpanStack[activeSpanStack.length - 1]; | ||
| const span: Span = { | ||
| name, | ||
| kind: opts?.kind ?? "INTERNAL", | ||
| traceId, | ||
| spanId, | ||
| parentSpanId: parent?.spanId, | ||
| startTimeUnixNano: process.hrtime.bigint(), | ||
| endTimeUnixNano: 0n, | ||
| attributes: { ...(opts?.attributes ?? {}) }, | ||
| status: { code: "UNSET" }, | ||
| resource: serviceResource(), |
There was a problem hiding this comment.
In OpenTelemetry, child spans must inherit the traceId of their parent span to maintain trace context propagation. Currently, a brand new traceId is generated for every span regardless of whether a parent exists, which completely breaks distributed tracing. Additionally, the Span interface is missing an end() method, which is required to properly pop spans from the active stack and avoid memory leaks.
| const traceId = generateId(16); | |
| const spanId = generateId(8); | |
| const parent = activeSpanStack[activeSpanStack.length - 1]; | |
| const span: Span = { | |
| name, | |
| kind: opts?.kind ?? "INTERNAL", | |
| traceId, | |
| spanId, | |
| parentSpanId: parent?.spanId, | |
| startTimeUnixNano: process.hrtime.bigint(), | |
| endTimeUnixNano: 0n, | |
| attributes: { ...(opts?.attributes ?? {}) }, | |
| status: { code: "UNSET" }, | |
| resource: serviceResource(), | |
| const parent = activeSpanStack[activeSpanStack.length - 1]; | |
| const traceId = parent ? parent.traceId : generateId(16); | |
| const spanId = generateId(8); | |
| const span: Span = { | |
| name, | |
| kind: opts?.kind ?? "INTERNAL", | |
| traceId, | |
| spanId, | |
| parentSpanId: parent?.spanId, | |
| startTimeUnixNano: process.hrtime.bigint(), | |
| endTimeUnixNano: 0n, | |
| attributes: { ...(opts?.attributes ?? {}) }, | |
| status: { code: "UNSET" }, | |
| resource: serviceResource(), | |
| end() { endSpan(this); } | |
| }; |
| function measureEventLoopLagMs(): number { | ||
| // Coarse but free: schedule an immediate and time how long it takes to | ||
| // fire. Averaged across the last 5 samples. | ||
| if (!lagSamples.length) { | ||
| for (let i = 0; i < 5; i++) { | ||
| const start = Date.now(); | ||
| setImmediate(() => lagSamples.push(Date.now() - start)); | ||
| } | ||
| return 0; | ||
| } | ||
| return lagSamples.reduce((a, b) => a + b, 0) / lagSamples.length; | ||
| } | ||
|
|
||
| const lagSamples: number[] = []; |
There was a problem hiding this comment.
The measureEventLoopLagMs function currently only schedules setImmediate calls when lagSamples is empty (at startup). Once lagSamples has 5 elements, it never schedules any new measurements, returning the exact same cached average forever. This makes the event loop lag metric completely static and useless after startup. Update it to schedule a new measurement on each call and maintain a rolling window of the last 5 samples.
| function measureEventLoopLagMs(): number { | |
| // Coarse but free: schedule an immediate and time how long it takes to | |
| // fire. Averaged across the last 5 samples. | |
| if (!lagSamples.length) { | |
| for (let i = 0; i < 5; i++) { | |
| const start = Date.now(); | |
| setImmediate(() => lagSamples.push(Date.now() - start)); | |
| } | |
| return 0; | |
| } | |
| return lagSamples.reduce((a, b) => a + b, 0) / lagSamples.length; | |
| } | |
| const lagSamples: number[] = []; | |
| function measureEventLoopLagMs(): number { | |
| const start = Date.now(); | |
| setImmediate(() => { | |
| lagSamples.push(Date.now() - start); | |
| if (lagSamples.length > 5) { | |
| lagSamples.shift(); | |
| } | |
| }); | |
| if (!lagSamples.length) return 0; | |
| return lagSamples.reduce((a, b) => a + b, 0) / lagSamples.length; | |
| } | |
| const lagSamples: number[] = []; |
| recordProviderAttempt({ | ||
| provider: safeProvider, | ||
| model: safeModel, | ||
| outcome: "success", | ||
| durationSeconds: durationMs / 1000, | ||
| durationMs, | ||
| }); |
There was a problem hiding this comment.
The recordProviderAttempt function does not accept durationMs in its signature. Passing it here will cause a TypeScript compilation error due to excess property checks on object literals.
recordProviderAttempt({
provider: safeProvider,
model: safeModel,
outcome: "success",
durationSeconds: durationMs / 1000,
});| recordProviderAttempt({ | ||
| provider: safeProvider, | ||
| model: safeModel, | ||
| outcome, | ||
| durationSeconds: durationMs / 1000, | ||
| durationMs, | ||
| }); |
There was a problem hiding this comment.
The recordProviderAttempt function does not accept durationMs in its signature. Passing it here will cause a TypeScript compilation error due to excess property checks on object literals.
recordProviderAttempt({
provider: safeProvider,
model: safeModel,
outcome,
durationSeconds: durationMs / 1000,
});| httpMetricsMiddleware({ | ||
| method, | ||
| route: path, | ||
| status, | ||
| durationSeconds: durationMs / 1000, | ||
| durationMs, | ||
| }); |
There was a problem hiding this comment.
| function stubSpan( | ||
| name: string, | ||
| opts?: { kind?: SpanKind; attributes?: Record<string, AttributeValue> } | ||
| ): Span { | ||
| return { | ||
| name, | ||
| kind: opts?.kind ?? "INTERNAL", | ||
| traceId: "00000000000000000000000000000000", | ||
| spanId: "0000000000000000", | ||
| startTimeUnixNano: 0n, | ||
| endTimeUnixNano: 0n, | ||
| attributes: { ...(opts?.attributes ?? {}) }, | ||
| status: { code: "UNSET" }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add the end() method to the stub span to satisfy the updated Span interface and ensure consistency when telemetry is disabled.
| function stubSpan( | |
| name: string, | |
| opts?: { kind?: SpanKind; attributes?: Record<string, AttributeValue> } | |
| ): Span { | |
| return { | |
| name, | |
| kind: opts?.kind ?? "INTERNAL", | |
| traceId: "00000000000000000000000000000000", | |
| spanId: "0000000000000000", | |
| startTimeUnixNano: 0n, | |
| endTimeUnixNano: 0n, | |
| attributes: { ...(opts?.attributes ?? {}) }, | |
| status: { code: "UNSET" }, | |
| }; | |
| } | |
| function stubSpan( | |
| name: string, | |
| opts?: { kind?: SpanKind; attributes?: Record<string, AttributeValue> } | |
| ): Span { | |
| return { | |
| name, | |
| kind: opts?.kind ?? "INTERNAL", | |
| traceId: "00000000000000000000000000000000", | |
| spanId: "0000000000000000", | |
| startTimeUnixNano: 0n, | |
| endTimeUnixNano: 0n, | |
| attributes: { ...(opts?.attributes ?? {}) }, | |
| status: { code: "UNSET" }, | |
| end() { endSpan(this); } | |
| }; | |
| } |
| import { | ||
| extractRoutePattern, | ||
| propagateTraceParent, | ||
| wrapProxy, | ||
| withProxySpan, | ||
| PROXY_SPAN_ROUTE_HEADER, | ||
| PROXY_SPAN_DURATION_HEADER, | ||
| } from "@/lib/observability/proxySpan"; | ||
| import { currentTraceId } from "@/lib/observability/otel"; |
There was a problem hiding this comment.
The test file imports and tests non-existent functions and constants (extractRoutePattern, withProxySpan, PROXY_SPAN_ROUTE_HEADER, and PROXY_SPAN_DURATION_HEADER) from @/lib/observability/proxySpan, which will cause compilation and test execution failures. Update the imports and refactor the test cases to only test the actual exported functions (wrapProxy and propagateTraceParent).
import {
propagateTraceParent,
wrapProxy,
} from "@/lib/observability/proxySpan";| import { | ||
| instrumentHttp, | ||
| instrumentFetch, | ||
| instrumentProvider, | ||
| instrumentCache, | ||
| instrumentDb, | ||
| sanitizeProvider, | ||
| sanitizeModel, | ||
| isAutoEnabled, | ||
| currentTraceId, | ||
| } from "@/lib/observability/auto"; |
There was a problem hiding this comment.
The test file imports non-existent functions instrumentHttp and instrumentProvider from @/lib/observability/auto, which will cause compilation and test execution failures. Additionally, the test cases call instrumentFetch and instrumentProvider with signatures that do not match the actual implementations in auto.ts (e.g., passing 3 arguments instead of the expected parameters). Update the imports and refactor the test cases to match the actual API signatures.
| import { | |
| instrumentHttp, | |
| instrumentFetch, | |
| instrumentProvider, | |
| instrumentCache, | |
| instrumentDb, | |
| sanitizeProvider, | |
| sanitizeModel, | |
| isAutoEnabled, | |
| currentTraceId, | |
| } from "@/lib/observability/auto"; | |
| import { | |
| instrumentFetch, | |
| instrumentProviderCall, | |
| instrumentCache, | |
| instrumentDb, | |
| sanitizeProvider, | |
| sanitizeModel, | |
| isAutoEnabled, | |
| currentTraceId, | |
| } from "@/lib/observability/auto"; | |
| import { wrapProxy } from "@/lib/observability/proxySpan"; |
| - alert: OmniRouteBifrostTimeoutRate | ||
| expr: | | ||
| sum(rate(omniroute_provider_upstream_attempts_total{outcome="timeout"}[5m])) | ||
| / | ||
| sum(rate(omniroute_provider_upstream_attempts_total[5m])) > 0.1 | ||
| for: 5m |
There was a problem hiding this comment.
The OmniRouteBifrostTimeoutRate alert uses a global sum(...) across all providers. If a low-traffic provider is completely broken (100% timeout rate), its timeouts will be masked by high-traffic healthy providers, and the alert will never fire. Group the expression by provider to ensure single-provider outages are caught.
- alert: OmniRouteBifrostTimeoutRate
expr: |
sum by (provider) (rate(omniroute_provider_upstream_attempts_total{outcome="timeout"}[5m]))
/
sum by (provider) (rate(omniroute_provider_upstream_attempts_total[5m])) > 0.1
for: 5m| - alert: OmniRouteHighConcurrency | ||
| expr: | | ||
| sum(rate(omniroute_http_requests_total[1m])) * 60 > 500 | ||
| for: 5m |
There was a problem hiding this comment.
The OmniRouteHighConcurrency alert is intended to detect single-node saturation (soft-concurrency ceiling for a single OmniRoute node). However, the PromQL expression uses a global sum(...) without grouping by instance. As the cluster scales horizontally, the aggregate request rate will naturally exceed 500 req/min, triggering false positive alerts even when individual nodes are completely healthy. Group the rate by instance.
- alert: OmniRouteHighConcurrency
expr: |
sum by (instance) (rate(omniroute_http_requests_total[1m])) * 60 > 500
for: 5m|
Diagnosis: CI is RED with Fast Quality Gates failing. |
|
Thanks for this work, @KooshaPari 🙏 — closing after a maintainer review of all 46 open refactor PRs in this batch (yours included). This is not a rejection of the effort. Why closed (applies to the whole batch): every branch here is 44–50 commits behind This PR specifically: Needs rework — well-calibrated alerts/SLOs, but they reference metrics that only exist in the orphaned observability stack. What happens to it: 📓 Captured as a documented lesson for our refactor roadmap — the approach is recorded (with your credit) and will guide the matching phase of our internal plan. The SLO thresholds + burn-rate definitions + the offline We ran a full case-by-case triage. The approaches worth keeping are being recorded internally with author credit, so when we pick them up the attribution travels with the code. Thank you for pushing on this. 🙌 |
Summary