Skip to content

Commit 88165ab

Browse files
author
Max
committed
fix(db): call_logs provider stats read true on empty and legacy data
Fallback SUMs default to 0 instead of null on empty tables; latency averages type as null (not 0) when no durations exist, propagated through provider-metrics and search stats routes; failures logged before the error_type column existed group under pre_migration instead of unclassified; GROUP BY provider queries use two new composite indexes (provider,timestamp + request_type,provider). New failures surface as an additive pre_migration bucket in the error breakdown. search/stats may now return null for providers.avg_latency_ms (no dashboard reader today). idx_call_logs_request_type stays: dropping it is a separate change. Dashboard latency badges may render an empty value until follow-up.
1 parent 9d1a896 commit 88165ab

7 files changed

Lines changed: 150 additions & 21 deletions

File tree

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
- **fix(db):** provider stats stay truthful on empty and legacy databases: fallback counts default to `0` instead of `null`, latency averages read `null` (not `0`) when no durations were recorded, failures logged before the error-type column existed group under `pre_migration` instead of `unclassified`, and per-provider queries use two new composite indexes ([#12832](https://github.com/diegosouzapw/OmniRoute/pull/12832)) — thanks @maxmad64bis

src/app/api/provider-metrics/route.ts

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import pino from "pino";
44
import { buildErrorBody } from "@omniroute/open-sse/utils/error.ts";
55

66
import { getProviderMetrics } from "@/lib/db/callLogStats";
7-
import { toNumber } from "@/shared/utils/numeric";
7+
import { toNumber, toNumberOrNull } from "@/shared/utils/numeric";
88

99
const logger = pino({ name: "provider-metrics-api" });
1010

@@ -22,7 +22,7 @@ export async function GET() {
2222
totalRequests: number;
2323
totalSuccesses: number;
2424
successRate: number;
25-
avgLatencyMs: number;
25+
avgLatencyMs: number | null;
2626
lastRequestAt: string | null;
2727
lastErrorAt: string | null;
2828
lastStatus: number | null;
@@ -41,7 +41,7 @@ export async function GET() {
4141
: "unknown";
4242
const totalRequests = toNumber(row.totalRequests);
4343
const totalSuccesses = toNumber(row.totalSuccesses);
44-
const avgLatencyMs = toNumber(row.avgLatencyMs);
44+
const avgLatencyMs = toNumberOrNull(row.avgLatencyMs);
4545
const lastRequestAt = typeof row.lastRequestAt === "string" ? row.lastRequestAt : null;
4646
const lastErrorAt = typeof row.lastErrorAt === "string" ? row.lastErrorAt : null;
4747
const lastStatus = row.lastStatus == null ? null : toNumber(row.lastStatus);
@@ -66,8 +66,7 @@ export async function GET() {
6666
// Only flag as errorProvider if the provider's MOST RECENT request was itself
6767
// a failure. A provider with a historical lastErrorAt but a recent success
6868
// (lastStatus 2xx/3xx) must not be shown as currently errored (#3619).
69-
const isCurrentlyInError =
70-
lastStatus !== null && (lastStatus < 200 || lastStatus >= 400);
69+
const isCurrentlyInError = lastStatus !== null && (lastStatus < 200 || lastStatus >= 400);
7170
const errorTs = isCurrentlyInError && lastErrorAt ? Date.parse(lastErrorAt) : 0;
7271
if (Number.isFinite(errorTs) && errorTs > errorProviderTs) {
7372
errorProvider = provider;

src/app/api/search/stats/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ export async function GET(request: Request) {
1616

1717
const providers: Record<
1818
string,
19-
{ requests: number; avg_latency_ms: number; total_cost: number }
19+
{ requests: number; avg_latency_ms: number | null; total_cost: number }
2020
> = {};
2121
for (const row of providerStats) {
2222
const costPerQuery = SEARCH_PROVIDERS[row.provider]?.costPerQuery || 0;

src/lib/db/callLogStats.ts

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ export interface ProviderMetricRow {
1818
provider: string;
1919
totalRequests: number;
2020
totalSuccesses: number;
21-
avgLatencyMs: number;
21+
avgLatencyMs: number | null;
2222
lastRequestAt: string | null;
2323
lastErrorAt: string | null;
2424
lastStatus: number | null;
@@ -37,7 +37,7 @@ export interface ProviderUsageRow {
3737
export interface SearchProviderStatRow {
3838
provider: string;
3939
requests: number;
40-
avg_latency_ms: number;
40+
avg_latency_ms: number | null;
4141
}
4242

4343
export interface SearchRecentRow {
@@ -126,10 +126,9 @@ export function getProviderMetrics(): ProviderMetricRow[] {
126126
*
127127
* Deliberately NOT `getProviderMetrics()` with a `since` parameter: that query
128128
* carries two correlated subqueries (`lastStatus`, `lastErrorStatus`) which a
129-
* ranking never displays, and they dominate its cost — `call_logs` is indexed
130-
* on `timestamp` alone, so each correlated pass rescans the whole window per
131-
* provider. Here a single bounded `GROUP BY` uses `idx_cl_timestamp` and stops
132-
* there. The rules are shared with its neighbour, not the query: same success
129+
* ranking never displays, and they dominate its cost. Here a single bounded
130+
* `GROUP BY` leans on `idx_cl_timestamp` plus `idx_cl_provider_timestamp` /
131+
* `idx_cl_request_provider` (migration 174) and stops there. The rules are shared with its neighbour, not the query: same success
133132
* definition, same `#10714` guard against providers whose connections are gone.
134133
*/
135134
export function getProviderUsageSince(since: string): ProviderUsageRow[] {
@@ -259,25 +258,25 @@ export function getFallbackStats(
259258
.prepare(
260259
`
261260
SELECT
262-
SUM(CASE WHEN (combo_name IS NULL OR combo_name = '') THEN 1 ELSE 0 END) as total,
263-
SUM(CASE WHEN requested_model IS NOT NULL AND requested_model != '' AND (combo_name IS NULL OR combo_name = '') THEN 1 ELSE 0 END) as with_requested,
264-
SUM(CASE
261+
COALESCE(SUM(CASE WHEN (combo_name IS NULL OR combo_name = '') THEN 1 ELSE 0 END), 0) as total,
262+
COALESCE(SUM(CASE WHEN requested_model IS NOT NULL AND requested_model != '' AND (combo_name IS NULL OR combo_name = '') THEN 1 ELSE 0 END), 0) as with_requested,
263+
COALESCE(SUM(CASE
265264
WHEN (combo_name IS NULL OR combo_name = '')
266265
AND requested_model IS NOT NULL
267266
AND requested_model != ''
268267
AND model IS NOT NULL
269268
AND model != ''
270269
THEN 1 ELSE 0 END
271-
) as fallback_eligible,
272-
SUM(CASE
270+
), 0) as fallback_eligible,
271+
COALESCE(SUM(CASE
273272
WHEN (combo_name IS NULL OR combo_name = '')
274273
AND requested_model IS NOT NULL
275274
AND requested_model != ''
276275
AND model IS NOT NULL
277276
AND model != ''
278277
AND LOWER(CASE WHEN instr(requested_model, '/') > 0 THEN substr(requested_model, instr(requested_model, '/') + 1) ELSE requested_model END) != LOWER(model)
279278
THEN 1 ELSE 0 END
280-
) as fallbacks
279+
), 0) as fallbacks
281280
FROM call_logs
282281
${whereClause}
283282
`
@@ -289,8 +288,9 @@ export function getFallbackStats(
289288
/**
290289
* Failure-family breakdown over `call_logs` for the usage analytics endpoint.
291290
* Failures are rows with status >= 400 or a non-empty error summary; successes
292-
* are excluded in SQL. Pre-migration rows and failures the classifier does not
293-
* recognize (null family) land in the explicit `unclassified` bucket.
291+
* are excluded in SQL. Rows predating migration 158 (`error_type` NULL,
292+
* `timestamp` before 2026-08-20) land in `pre_migration`; other NULL families
293+
* land in `unclassified`.
294294
*
295295
* @param whereClause - SQL WHERE clause (may be empty string) using the same
296296
* named params as the usage_history queries.
@@ -305,7 +305,9 @@ export function getErrorTypeBreakdown(
305305
.prepare(
306306
`
307307
SELECT
308-
COALESCE(error_type, 'unclassified') AS errorType,
308+
-- '2026-08-20' = commit 4c15c05f9 that added error_type (migration 158).
309+
-- Lower bound, not exact: late upgraders have post-cutoff rows with NULL values.
310+
CASE WHEN error_type IS NULL AND timestamp < '2026-08-20' THEN 'pre_migration' WHEN error_type IS NULL THEN 'unclassified' ELSE error_type END AS errorType,
309311
COUNT(*) AS count
310312
FROM call_logs
311313
${whereClause} ${whereClause ? "AND" : "WHERE"} (status >= 400 OR error_summary IS NOT NULL)

src/lib/db/core.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -400,6 +400,8 @@ const SCHEMA_SQL = `
400400
);
401401
CREATE INDEX IF NOT EXISTS idx_cl_timestamp ON call_logs(timestamp);
402402
CREATE INDEX IF NOT EXISTS idx_cl_status ON call_logs(status);
403+
CREATE INDEX IF NOT EXISTS idx_cl_provider_timestamp ON call_logs(provider, timestamp);
404+
CREATE INDEX IF NOT EXISTS idx_cl_request_provider ON call_logs(request_type, provider);
403405
404406
CREATE TABLE IF NOT EXISTS proxy_logs (
405407
id TEXT PRIMARY KEY,
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
-- GROUP BY provider support. (provider,timestamp) backs the bare
2+
-- GROUP BY provider in getProviderMetrics; (request_type,provider)
3+
-- backs WHERE request_type='search' GROUP BY provider. Non-covering for the
4+
-- real queries (duration/status outside the index) by design — no third index.
5+
CREATE INDEX IF NOT EXISTS idx_cl_provider_timestamp ON call_logs(provider, timestamp);
6+
CREATE INDEX IF NOT EXISTS idx_cl_request_provider ON call_logs(request_type, provider);
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
import test from "node:test";
2+
import assert from "node:assert/strict";
3+
import fs from "node:fs";
4+
import os from "node:os";
5+
import path from "node:path";
6+
7+
const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-call-logs-stats-"));
8+
process.env.DATA_DIR = TEST_DATA_DIR;
9+
process.env.NODE_ENV = "test";
10+
process.env.DISABLE_SQLITE_AUTO_BACKUP = "true";
11+
12+
const core = await import("../../../src/lib/db/core.ts");
13+
const stats = await import("../../../src/lib/db/callLogStats.ts");
14+
15+
function resetDb() {
16+
core.resetDbInstance();
17+
fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 });
18+
fs.mkdirSync(TEST_DATA_DIR, { recursive: true });
19+
}
20+
21+
test.beforeEach(() => {
22+
resetDb();
23+
});
24+
25+
test.after(() => {
26+
core.resetDbInstance();
27+
fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 });
28+
});
29+
30+
test("getFallbackStats on empty DB returns zeros, not nulls", () => {
31+
core.getDbInstance(); // plays SCHEMA + runMigrations on the file DB
32+
const row = stats.getFallbackStats("", {});
33+
assert.deepEqual(row, {
34+
total: 0,
35+
with_requested: 0,
36+
fallback_eligible: 0,
37+
fallbacks: 0,
38+
});
39+
});
40+
41+
test("avgLatencyMs is null when all durations are NULL, and the route propagates null", async () => {
42+
const db = core.getDbInstance();
43+
const now = new Date().toISOString();
44+
db.prepare(
45+
`INSERT INTO provider_connections (id, provider, created_at, updated_at)
46+
VALUES ('conn-1', 'openai', ?, ?)`
47+
).run(now, now);
48+
db.prepare(
49+
`INSERT INTO call_logs (id, timestamp, provider, status, duration)
50+
VALUES ('log-1', ?, 'openai', 200, NULL)`
51+
).run(now);
52+
53+
const { toNumberOrNull } = await import("../../../src/shared/utils/numeric.ts");
54+
const rows = stats.getProviderMetrics();
55+
assert.equal(rows.length, 1);
56+
// Lib-level: passes before AND after (the driver already returns null — only
57+
// the TS type lied). Kept as documentation, not as red/green proof.
58+
assert.equal(toNumberOrNull(rows[0].avgLatencyMs), null);
59+
60+
const { GET } = await import("../../../src/app/api/provider-metrics/route.ts");
61+
const res = await GET();
62+
const body = (await res.json()) as {
63+
metrics: Record<string, { avgLatencyMs: number | null }>;
64+
};
65+
// Route-level: THIS is the red/green proof (toNumber(null) → 0 before fix).
66+
assert.equal(body.metrics["openai"].avgLatencyMs, null);
67+
});
68+
69+
test("error_type NULL splits into pre_migration vs unclassified by timestamp", () => {
70+
const db = core.getDbInstance();
71+
const now = new Date().toISOString();
72+
db.prepare(
73+
`INSERT INTO provider_connections (id, provider, created_at, updated_at)
74+
VALUES ('conn-1', 'openai', ?, ?)`
75+
).run(now, now);
76+
db.prepare(
77+
`INSERT INTO call_logs (id, timestamp, provider, status, error_type)
78+
VALUES ('old-1', '2026-08-01T00:00:00.000Z', 'openai', 500, NULL),
79+
('new-1', ?, 'openai', 500, NULL)`
80+
).run(now);
81+
82+
const breakdown = stats.getErrorTypeBreakdown("", {});
83+
const byType = new Map(breakdown.map((b) => [b.errorType, b.count]));
84+
assert.equal(byType.get("pre_migration"), 1);
85+
assert.equal(byType.get("unclassified"), 1);
86+
});
87+
88+
test("migration 174 creates provider GROUP BY indexes used by search stats", () => {
89+
const db = core.getDbInstance();
90+
91+
const names = (
92+
db.prepare("SELECT name FROM sqlite_master WHERE type = 'index'").all() as Array<{
93+
name: string;
94+
}>
95+
).map((r) => r.name);
96+
assert.ok(
97+
names.includes("idx_cl_provider_timestamp"),
98+
"idx_cl_provider_timestamp must exist after migrations"
99+
);
100+
assert.ok(
101+
names.includes("idx_cl_request_provider"),
102+
"idx_cl_request_provider must exist after migrations"
103+
);
104+
105+
const plan = (
106+
db
107+
.prepare(
108+
"EXPLAIN QUERY PLAN SELECT provider, COUNT(*), AVG(duration) FROM call_logs WHERE request_type = 'search' GROUP BY provider"
109+
)
110+
.all() as Array<{ detail: string }>
111+
)
112+
.map((r) => r.detail)
113+
.join(" | ");
114+
assert.ok(
115+
plan.includes("USING INDEX idx_cl_request_provider"),
116+
`planner must use idx_cl_request_provider, got: ${plan}`
117+
);
118+
assert.ok(!plan.includes("SCAN TABLE"), `must not table-scan, got: ${plan}`);
119+
});

0 commit comments

Comments
 (0)