Repository navigation
feat(admin): routing election + exclusion telemetry - #3458
Merged
Merged
Conversation
Adds hourly aggregates for how routing picked a provider and which mappings were filtered out before scoring, so the admin dashboard can show what share of requests carry a service tier and what share of candidates never reached an election. The trigger: on gpt-5.6-sol, Azure scores best (-0.986) but serves 5% of traffic. Its mapping declares no serviceTiers, so every tiered request — including the implicit dev-plan flex default — drops it from the candidate set roughly a thousand lines before any score is computed, and nothing recorded that. - routing_election_hourly / routing_exclusion_hourly, keyed on closed enums so no code path can explode their cardinality - log.appliedServiceTier, since requestedServiceTier is snapshotted before the dev-plan default runs and recorded the implicit tier as NULL - service-tier counters on the mapping/model history tables - stable exclusion reason codes, with the persisted prose derived from the code so the log detail view is unchanged - record the six exclusion sites that previously dropped candidates with no reason at all Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
WalkthroughThe PR adds structured routing exclusion metadata, service-tier counters, hourly election and exclusion aggregation, expanded admin analytics responses, and dashboard visualizations for routing paths, exclusions, eligibility, and service-tier coverage. ChangesRouting telemetry
Sequence Diagram(s)sequenceDiagram
participant Gateway
participant RoutingLogs
participant calculateRoutingTelemetryForHour
participant AdminAnalytics
Gateway->>RoutingLogs: record routing metadata and exclusion codes
RoutingLogs->>calculateRoutingTelemetryForHour: aggregate the target UTC hour
calculateRoutingTelemetryForHour->>AdminAnalytics: provide hourly election and exclusion data
AdminAnalytics->>AdminAnalytics: compute eligibility and service-tier summaries
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
apps/gateway/src/chat/chat.ts (1)
577-577: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose the full
FilteredProvidercontract.
recordFilteredProviderwrites stablecodesintofilteredOut. Change this parameter toFilteredProvider[]so the signature preserves the telemetry contract.Proposed refactor
- filteredOut?: Array<{ providerId: string; reasons: string[] }>, + filteredOut?: FilteredProvider[],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/gateway/src/chat/chat.ts` at line 577, Update the filteredOut parameter in recordFilteredProvider to use FilteredProvider[] instead of the inline object type, preserving the full contract including stable codes written by the method.apps/worker/src/services/routing-telemetry-aggregator.ts (1)
291-294: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the sequential execution explicit.
const [a, b] = [await f(), await g()]evaluates the two calls in order, not concurrently. The array-destructuring shape reads likePromise.alland invites a later reader to assume concurrency. Use two statements, or usePromise.allif concurrency is wanted.♻️ Proposed clarification
- const [electionRows, exclusionRows] = [ - await aggregateElections(targetHour), - await aggregateExclusions(targetHour), - ]; + const electionRows = await aggregateElections(targetHour); + const exclusionRows = await aggregateExclusions(targetHour);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/worker/src/services/routing-telemetry-aggregator.ts` around lines 291 - 294, Update the aggregation flow around aggregateElections and aggregateExclusions to use separate statements, assigning electionRows before calling aggregateExclusions and then assigning exclusionRows. Preserve the current sequential execution order and downstream values.apps/worker/src/services/stats-calculator.ts (1)
406-427: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the service-tier classification SQL.
This block is repeated verbatim at lines 678-699 in this file. Two of the four expressions are repeated again in
apps/worker/src/services/routing-telemetry-aggregator.tsat lines 50-51. The three copies define whatexplicit,implicit,servedandunconfirmedmean. If one copy changes, the dashboard mixes two definitions of the same counter without any type error.Export the four SQL fragments from one module and import them at all three sites.
As per coding guidelines: "Apply DRY principles for reusable code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/worker/src/services/stats-calculator.ts` around lines 406 - 427, Extract the four service-tier classification SQL expressions from the stats calculator into one shared module, exporting symbols for explicit, implicit, served, and unconfirmed counts. Update both occurrences in stats-calculator and the matching expressions in routing-telemetry-aggregator to import and reuse those exports, preserving the existing aliases and counter semantics.Source: Coding guidelines
apps/api/src/routes/admin-routing-analytics.ts (1)
788-798: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType election kinds from the shared telemetry union.
routingSelectionKindreturnsRoutingSelectionKind, butelectionKindEntrySchemaaccepts any string and the"scored"lookup inscoredCountis hard-coded to"scored". If that kind is renamed or removed, this path can return0without a compiler error. Make theelectionKindEntrySchema.kindfield a typed reference to the shared kind union and derive thescoredvalue through that same type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/admin-routing-analytics.ts` around lines 788 - 798, Update electionKindEntrySchema.kind to use the shared RoutingSelectionKind union, then derive the scored-count lookup key from that typed kind rather than an untyped hard-coded string. Apply the change around the elections analytics construction and preserve the existing request-count aggregation and sorting behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/routes/admin-routing-analytics.ts`:
- Around line 623-631: Update the bucket aggregation around seenCandidateBuckets
to retain the maximum candidateCount for each (hourTimestamp, providerId) bucket
rather than the first encountered row. After all rows are processed, sum those
per-bucket maxima into candidateCountByProvider, preserving one deterministic
denominator per provider.
In `@apps/worker/src/services/routing-telemetry-aggregator.spec.ts`:
- Around line 84-92: Guard the destructured result from the user insert before
using testUser.email in the organization setup and subsequent references. Since
this test expects one inserted row, apply a non-null assertion to testUser or
add an explicit guard that fails when the returning result is empty.
In `@apps/worker/src/services/routing-telemetry-aggregator.ts`:
- Around line 94-96: Update aggregateElections to include
excludeRecoveredSameProviderRegionRetry() in the log query’s where conditions,
matching calculateHistoryForMinute and calculateModelHistoryForMinute. Keep the
existing time-range and grouping behavior unchanged so recovered
same-provider-region retries are excluded from election aggregations.
- Around line 188-221: Add a fourth UNION ALL branch to the candidates CTE,
alongside the existing availableProviders, filteredProviders, and
contentFilterExcludedProviders branches, that extracts providerId from each
metadata.providerScores entry. This ensures providers represented in
providerScores, including rate-limited providers, contribute to candidateCount.
In `@apps/worker/src/services/stats-calculator.ts`:
- Around line 1243-1251: Update backfillHourlyHistoryIfNeeded to include routing
telemetry presence when deciding whether an hourly bucket needs processing, so
existing mapping/model hours do not suppress routing backfills. Ensure
calculateHistoryForHour propagates or otherwise consistently handles
calculateRoutingTelemetryForHour failures in both rolling-up and backfilling
flows, matching the existing error behavior for the other history calculations.
In `@ee/admin/src/app/routing-analytics/client.tsx`:
- Around line 881-919: Replace the eligibility percentage calculation in the
table cell with an API-provided decision-level eligibility rate or a value
computed from the unique decision denominator and eligible count; do not use
eligibility.exclusionRate or 1 - eligibility.exclusionRate. Update the backing
API response/type to expose the required fields, then render that authoritative
value while preserving the existing topReason display and empty-state behavior.
---
Nitpick comments:
In `@apps/api/src/routes/admin-routing-analytics.ts`:
- Around line 788-798: Update electionKindEntrySchema.kind to use the shared
RoutingSelectionKind union, then derive the scored-count lookup key from that
typed kind rather than an untyped hard-coded string. Apply the change around the
elections analytics construction and preserve the existing request-count
aggregation and sorting behavior.
In `@apps/gateway/src/chat/chat.ts`:
- Line 577: Update the filteredOut parameter in recordFilteredProvider to use
FilteredProvider[] instead of the inline object type, preserving the full
contract including stable codes written by the method.
In `@apps/worker/src/services/routing-telemetry-aggregator.ts`:
- Around line 291-294: Update the aggregation flow around aggregateElections and
aggregateExclusions to use separate statements, assigning electionRows before
calling aggregateExclusions and then assigning exclusionRows. Preserve the
current sequential execution order and downstream values.
In `@apps/worker/src/services/stats-calculator.ts`:
- Around line 406-427: Extract the four service-tier classification SQL
expressions from the stats calculator into one shared module, exporting symbols
for explicit, implicit, served, and unconfirmed counts. Update both occurrences
in stats-calculator and the matching expressions in routing-telemetry-aggregator
to import and reuse those exports, preserving the existing aliases and counter
semantics.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 65bbbaf0-17b9-49f1-a61f-83a6951e627e
📒 Files selected for processing (17)
apps/api/src/routes/admin-routing-analytics.spec.tsapps/api/src/routes/admin-routing-analytics.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/provider-filter-reasons.spec.tsapps/gateway/src/chat/tools/provider-filter-reasons.tsapps/worker/src/services/routing-telemetry-aggregator.spec.tsapps/worker/src/services/routing-telemetry-aggregator.tsapps/worker/src/services/stats-calculator.tsee/admin/src/app/routing-analytics/client.tsxpackages/actions/src/get-cheapest-from-available-providers.tspackages/db/migrations/1786112381_yielding_hydra.sqlpackages/db/migrations/meta/1786112381_snapshot.jsonpackages/db/migrations/meta/_journal.jsonpackages/db/src/schema.tspackages/shared/package.jsonpackages/shared/src/index.tspackages/shared/src/routing-telemetry.ts
Comment on lines
+623
to
+631
| const bucketKey = `${row.hourTimestamp.getTime()}:${row.providerId}`; | ||
| if (!seenCandidateBuckets.has(bucketKey)) { | ||
| seenCandidateBuckets.add(bucketKey); | ||
| candidateCountByProvider.set( | ||
| row.providerId, | ||
| (candidateCountByProvider.get(row.providerId) ?? 0) + | ||
| row.candidateCount, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Take the maximum candidate count per bucket, not the first row.
seenCandidateBuckets keeps the candidateCount of whichever row for a (hour, provider) bucket arrives first. The query has no ORDER BY, so the row order is not defined. The schema comment for routingExclusionHourly.candidateCount states the value must be read with max() when grouping across reasons. If two reason rows for the same bucket ever disagree, for example after a partial rerun of the aggregator, the reported denominator becomes non-deterministic.
Track the maximum per bucket, then sum the buckets.
🐛 Proposed fix
const exclusionsByProvider = new Map<string, Map<string, number>>();
- const candidateCountByProvider = new Map<string, number>();
- const seenCandidateBuckets = new Set<string>();
+ const candidateCountByBucket = new Map<string, number>();
for (const row of exclusionRows) {
let reasonMap = exclusionsByProvider.get(row.providerId);
if (!reasonMap) {
reasonMap = new Map();
exclusionsByProvider.set(row.providerId, reasonMap);
}
reasonMap.set(
row.reason,
(reasonMap.get(row.reason) ?? 0) + row.excludedCount,
);
const bucketKey = `${row.hourTimestamp.getTime()}:${row.providerId}`;
- if (!seenCandidateBuckets.has(bucketKey)) {
- seenCandidateBuckets.add(bucketKey);
- candidateCountByProvider.set(
- row.providerId,
- (candidateCountByProvider.get(row.providerId) ?? 0) +
- row.candidateCount,
- );
- }
+ candidateCountByBucket.set(
+ bucketKey,
+ Math.max(candidateCountByBucket.get(bucketKey) ?? 0, row.candidateCount),
+ );
+ }
+
+ const candidateCountByProvider = new Map<string, number>();
+ for (const [bucketKey, candidateCount] of candidateCountByBucket) {
+ const providerId = bucketKey.slice(bucketKey.indexOf(":") + 1);
+ candidateCountByProvider.set(
+ providerId,
+ (candidateCountByProvider.get(providerId) ?? 0) + candidateCount,
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const bucketKey = `${row.hourTimestamp.getTime()}:${row.providerId}`; | |
| if (!seenCandidateBuckets.has(bucketKey)) { | |
| seenCandidateBuckets.add(bucketKey); | |
| candidateCountByProvider.set( | |
| row.providerId, | |
| (candidateCountByProvider.get(row.providerId) ?? 0) + | |
| row.candidateCount, | |
| ); | |
| } | |
| const exclusionsByProvider = new Map<string, Map<string, number>>(); | |
| const candidateCountByBucket = new Map<string, number>(); | |
| for (const row of exclusionRows) { | |
| let reasonMap = exclusionsByProvider.get(row.providerId); | |
| if (!reasonMap) { | |
| reasonMap = new Map(); | |
| exclusionsByProvider.set(row.providerId, reasonMap); | |
| } | |
| reasonMap.set( | |
| row.reason, | |
| (reasonMap.get(row.reason) ?? 0) + row.excludedCount, | |
| ); | |
| const bucketKey = `${row.hourTimestamp.getTime()}:${row.providerId}`; | |
| candidateCountByBucket.set( | |
| bucketKey, | |
| Math.max(candidateCountByBucket.get(bucketKey) ?? 0, row.candidateCount), | |
| ); | |
| } | |
| const candidateCountByProvider = new Map<string, number>(); | |
| for (const [bucketKey, candidateCount] of candidateCountByBucket) { | |
| const providerId = bucketKey.slice(bucketKey.indexOf(":") + 1); | |
| candidateCountByProvider.set( | |
| providerId, | |
| (candidateCountByProvider.get(providerId) ?? 0) + candidateCount, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/routes/admin-routing-analytics.ts` around lines 623 - 631,
Update the bucket aggregation around seenCandidateBuckets to retain the maximum
candidateCount for each (hourTimestamp, providerId) bucket rather than the first
encountered row. After all rows are processed, sum those per-bucket maxima into
candidateCountByProvider, preserving one deterministic denominator per provider.
Comment on lines
+84
to
+92
| const [testUser] = await db | ||
| .insert(user) | ||
| .values({ email: "routing-telemetry@example.com", name: "RT" }) | ||
| .returning(); | ||
| await db.insert(organization).values({ | ||
| id: "rt-org", | ||
| name: "RT Org", | ||
| billingEmail: testUser.email, | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether noUncheckedIndexedAccess is enabled for the worker package.
fd -t f 'tsconfig*.json' | xargs rg -nP 'noUncheckedIndexedAccess|"extends"'Repository: theopenco/llmgateway
Length of output: 1320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== root tsconfig =="
cat -n tsconfig.json | sed -n '1,140p'
echo
echo "== worker tsconfig =="
cat -n apps/worker/tsconfig.json | sed -n '1,120p'
echo
echo "== relevant spec snippet =="
cat -n apps/worker/src/services/routing-telemetry-aggregator.spec.ts | sed -n '70,110p'
echo
echo "== stats-calculator context =="
cat -n apps/worker/src/services/stats-calculator.ts | sed -n '1300,1325p'
echo
echo "== all noUncheckedIndexedAccess references =="
rg -n '"noUncheckedIndexedAccess"|noUncheckedIndexedAccess' -g 'tsconfig*.json' .Repository: theopenco/llmgateway
Length of output: 3807
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe the exact indexing result shape/type shape if possible by reading Drizzle insert return declarations/types
# only if they are available in the repository or installed type declarations. This is read-only and does not run
# repository code.
echo "== locate drizzle insert type declarations in node_modules =="
if [ -d node_modules/drizzle-orm ]; then
rg -n "type .*InsertResult|interface .*InsertResult|ReturningHelper|getInsertResult" node_modules/drizzle-orm -g '*.d.ts' | head -80
else
echo "node_modules/drizzle-orm not present"
fiRepository: theopenco/llmgateway
Length of output: 257
🌐 Web query:
Drizzle ORM insert returning() insert result returning rows type array optional
💡 Result:
In Drizzle ORM, the.returning method consistently returns an array of the inserted rows, regardless of whether a single object or an array of objects is passed to.values [1][2]. The type of the result is always an array (e.g., T[]), not an optional or single object type [3][4]. Key points regarding the return type: 1. Array Consistency: Even when inserting a single record, Drizzle returns an array containing that record [3][2]. To retrieve the object directly, developers commonly destructure the result, such as: const [user] = await db.insert(users).values(data).returning [2]. 2. Type Behavior: Because the return type is an array, accessing the first element (e.g., [user]) will result in a type of {... } | undefined [3]. This is because TypeScript correctly identifies that an array might be empty (though in the case of a successful insert, it will contain the record) [3]. 3. Alternative for MySQL: For databases that do not natively support RETURNING, such as MySQL, Drizzle provides a $returningId method, which similarly returns an array of objects containing the primary keys of the inserted rows (e.g., { id: number }[]) [5][6]. While there have been community requests to change this behavior to return tuples or single objects for single-row inserts [3], the current design maintains a uniform array return type across all operations [3][4].
Citations:
- 1: https://orm.drizzle.team/docs/insert
- 2: https://drizzle-team-drizzle-orm.mintlify.app/queries/insert
- 3: [FEATURE]: Insert should return tuple, not array, based on the number of items received drizzle-team/drizzle-orm#955
- 4: https://github.com/drizzle-team/drizzle-orm/blob/e8e6edfe/drizzle-orm/src/pg-core/query-builders/insert.ts
- 5: https://github.com/drizzle-team/drizzle-orm-docs/blob/926ce90e/src/content/docs/insert.mdx
- 6: https://orm.drizzle.team/docs/mysql/insert
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tsconfig strictNullChecks flag =="
python3 - <<'PY'
import json
from pathlib import Path
cfg = json.loads(Path("tsconfig.json").read_text())
print("compilerOptions.strictNullChecks:", cfg.get("compilerOptions", {}).get("strictNullChecks"))
worker = json.loads(Path("apps/worker/tsconfig.json").read_text())
print("apps/worker extends:", worker.get("extends"))
PY
echo
echo "== insert statements in worker tests/services =="
rg -n '\.insert\([^)]*\)\.values\(.*\)\.returning\(\)' apps/worker/src -SRepository: theopenco/llmgateway
Length of output: 325
Guard the destructured insert result.
strictNullChecks is active for apps/worker. .returning() returns an array whose first element is undefined when the insert returns no rows, so testUser can be undefined before lines 91 and 101. Guard testUser, or use testUser! if the insert is guaranteed to return one row.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/worker/src/services/routing-telemetry-aggregator.spec.ts` around lines
84 - 92, Guard the destructured result from the user insert before using
testUser.email in the organization setup and subsequent references. Since this
test expects one inserted row, apply a non-null assertion to testUser or add an
explicit guard that fails when the returning result is empty.
Comment on lines
+94
to
+96
| .from(log) | ||
| .where(and(gte(log.createdAt, start), lt(log.createdAt, end))) | ||
| .groupBy(usedBaseModelSql, log.usedProvider, selectionReasonSql); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the retry-exclusion helper and every read of `log` in the worker.
rg -nP -C 10 'function excludeRecoveredSameProviderRegionRetry' apps/worker packages
rg -nP -C 3 'excludeRecoveredSameProviderRegionRetry\s*\(' appsRepository: theopenco/llmgateway
Length of output: 3645
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'routing-telemetry-aggregator\.ts|stats-calculator\.ts' . | sed 's#^\./##'
echo
echo "== routing-telemetry-aggregator outline =="
ast-grep outline apps/worker/src/services/routing-telemetry-aggregator.ts --view expanded || true
echo
echo "== relevant routing-telemetry-aggregator lines =="
sed -n '1,180p' apps/worker/src/services/routing-telemetry-aggregator.ts
echo
echo "== stats-calculator relevant helper and minute aggregations =="
sed -n '40,70p' apps/worker/src/services/stats-calculator.ts
sed -n '400,450p' apps/worker/src/services/stats-calculator.ts
sed -n '685,720p' apps/worker/src/services/stats-calculator.tsRepository: theopenco/llmgateway
Length of output: 12339
Exclude recovered same-provider-region retries from election aggregations.
calculateHistoryForMinute and calculateModelHistoryForMinute already filter log with excludeRecoveredSameProviderRegionRetry(). aggregateElections does not, so retried requests recovered on the same provider/region can contribute multiple log rows to routingTelemetryElectionHistory while mapping/model history counts only the final successful row. Add the same predicate here, or document that elections intentionally count every log row.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/worker/src/services/routing-telemetry-aggregator.ts` around lines 94 -
96, Update aggregateElections to include
excludeRecoveredSameProviderRegionRetry() in the log query’s where conditions,
matching calculateHistoryForMinute and calculateModelHistoryForMinute. Keep the
existing time-range and grouping behavior unchanged so recovered
same-provider-region retries are excluded from election aggregations.
Comment on lines
+188
to
+221
| union all | ||
| -- rate-limited mappings, annotated on the score entries | ||
| select | ||
| hour_logs.model_id, | ||
| score ->> 'providerId' as provider_id, | ||
| 'rate_limited' as reason | ||
| from hour_logs | ||
| cross join lateral jsonb_array_elements( | ||
| coalesce(hour_logs.metadata -> 'providerScores', '[]'::jsonb) | ||
| ) as score | ||
| where (score ->> 'rate_limited')::boolean is true | ||
| ), | ||
| -- Every provider this model saw in a routing decision, whether it was | ||
| -- kept or dropped. This is the denominator: without it an exclusion count | ||
| -- cannot be turned into a rate. | ||
| candidates as ( | ||
| select hour_logs.model_id, provider_id | ||
| from hour_logs | ||
| cross join lateral jsonb_array_elements_text( | ||
| coalesce(hour_logs.metadata -> 'availableProviders', '[]'::jsonb) | ||
| ) as provider_id | ||
| union all | ||
| select hour_logs.model_id, entry ->> 'providerId' as provider_id | ||
| from hour_logs | ||
| cross join lateral jsonb_array_elements( | ||
| coalesce(hour_logs.metadata -> 'filteredProviders', '[]'::jsonb) | ||
| ) as entry | ||
| union all | ||
| select hour_logs.model_id, provider_id | ||
| from hour_logs | ||
| cross join lateral jsonb_array_elements_text( | ||
| coalesce(hour_logs.metadata -> 'contentFilterExcludedProviders', '[]'::jsonb) | ||
| ) as provider_id | ||
| ), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how availableProviders, providerScores and filteredProviders are populated in routing metadata.
rg -nP -C 6 "availableProviders|providerScores|rate_limited" apps/gateway/src packages/actions/src packages/shared/srcRepository: theopenco/llmgateway
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefaile
echo "== target file excerpt =="
nl -ba apps/worker/src/services/routing-telemetry-aggregator.ts | sed -n '1,280p'
echo
echo "== precise metadata construction sites =="
rg -n -C 4 "availableProviders\s*:\s*|providerScores\s*:\s*\[|filteredProviders\s*:\s*|contentFilterExcludedProviders\s*:\s*|rate_limited" apps/gateway/src packages/shared/src packages/actions/src | head -n 300Repository: theopenco/llmgateway
Length of output: 208
🏁 Script executed:
#!/bin/bash
set -eu -o pipefail
echo "== target file excerpt =="
nl -ba apps/worker/src/services/routing-telemetry-aggregator.ts | sed -n '1,280p'
echo
echo "== precise metadata construction sites =="
rg -n -C 4 "availableProviders\s*:\s*|providerScores\s*:\s*\[|filteredProviders\s*:\s*|contentFilterExcludedProviders\s*:\s*|rate_limited" apps/gateway/src packages/shared/src packages/actions/src | head -n 300Repository: theopenco/llmgateway
Length of output: 224
🏁 Script executed:
#!/bin/bash
set -eu -o pipefail
echo "== target file excerpt =="
awk '{printf "%6d\t%s\n", NR, $0}' apps/worker/src/services/routing-telemetry-aggregator.ts | sed -n '1,280p'
echo
echo "== route selection helpers excerpt =="
rg -n -C 5 "function selectNextProvider|const selectNextProvider|providerScores|filteredProviders|contentFilterExcludedProviders|availability|availableProvider|rate_limited" apps/gateway/src packages/shared/src packages/actions/src | sed -n '1,380p'Repository: theopenco/llmgateway
Length of output: 39922
🏁 Script executed:
#!/bin/bash
set -eu -o pipefail
echo "== selectNextProvider definition =="
rg -n -C 20 "selectNextProvider" apps/gateway/src/lib apps/gateway/src | sed -n '1,220p'
echo
echo "== providerScore/rateLimited construction =="
rg -n -C 5 "ProviderScoreForHysteresis|rate_limited|rateLimited|is_rate_limited|RPM|rpm|429|unavailable" apps/gateway/src packages/shared/src packages/actions/src | sed -n '1,400p'Repository: theopenco/llmgateway
Length of output: 41281
🏁 Script executed:
#!/bin/bash
set -eu -o pipefail
echo "== focused rate_limited construction =="
rg -n -C 8 "rate_limited|rateLimited|is_rate_limited|selectProvider|ProviderScoreForHysteresis|getCheapestFromAvailableProviders" apps/gateway/src packages/shared/src packages/actions/src | sed -n '1,520p'
echo
echo "== selectNextProvider file locations =="
rg -n "export .*selectNextProvider|function selectNextProvider|const selectNextProvider" apps/gateway/src packages/shared/src packages/actions/srcRepository: theopenco/llmgateway
Length of output: 34961
🏁 Script executed:
#!/bin/bash
set -eu -o pipefail
echo "== retry-with-fallback excerpts around selectNextProvider and score construction =="
awk '{printf "%6d\t%s\n", NR, $0}' apps/gateway/src/chat/tools/retry-with-fallback.ts | sed -n '1,210p'
awk '{printf "%6d\t%s\n", NR, $0}' apps/gateway/src/chat/tools/retry-with-fallback.ts | sed -n '210,380p'
echo
echo "== get-cheapest-from-available-providers excerpts around metadata/selected provider =="
rg -n -C 12 "providerScores|selectedProvider|routingMetadata|score|rate_limited|returned" packages/actions/src/get-cheapest-from-available-providers.ts packages/actions/src/models.ts packages/actions/src/models.spec.ts | sed -n '1,420p'Repository: theopenco/llmgateway
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -eu -o pipefail
echo "== retry loop rate_limited mutation excerpts =="
rg -n -C 10 "rate_limited|availableProviders|providerScores|rateLimit|shouldRateLimit|checkRateLimit|rate limit" packages/actions/src packages/shared/src apps/gateway/src | sed -n '1,600p'
echo
echo "== provider score conversion tests =="
rg -n -C 6 "rate_limited|isRateLimi|rateLimited|RPM|429" packages/actions/src packages/shared/src apps/gateway/src | sed -n '1,400p'Repository: theopenco/llmgateway
Length of output: 50377
Add scored providers from providerScores to the exclusion candidate denominator.
rate_limited comes from providerScores, but candidates only collects availableProviders, filteredProviders, and contentFilterExcludedProviders. Add a fourth branch for providerScores.map(...) so a rate-limited provider has a candidateCount; otherwise candidateCount falls back to the exclusion count and the aggregator stores 100% as the exclusion rate.
♻️ Proposed additional union branch
cross join lateral jsonb_array_elements_text(
coalesce(hour_logs.metadata -> 'contentFilterExcludedProviders', '[]'::jsonb)
) as provider_id
+ union all
+ select hour_logs.model_id, score ->> 'providerId' as provider_id
+ from hour_logs
+ cross join lateral jsonb_array_elements(
+ coalesce(hour_logs.metadata -> 'providerScores', '[]'::jsonb)
+ ) as score
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| union all | |
| -- rate-limited mappings, annotated on the score entries | |
| select | |
| hour_logs.model_id, | |
| score ->> 'providerId' as provider_id, | |
| 'rate_limited' as reason | |
| from hour_logs | |
| cross join lateral jsonb_array_elements( | |
| coalesce(hour_logs.metadata -> 'providerScores', '[]'::jsonb) | |
| ) as score | |
| where (score ->> 'rate_limited')::boolean is true | |
| ), | |
| -- Every provider this model saw in a routing decision, whether it was | |
| -- kept or dropped. This is the denominator: without it an exclusion count | |
| -- cannot be turned into a rate. | |
| candidates as ( | |
| select hour_logs.model_id, provider_id | |
| from hour_logs | |
| cross join lateral jsonb_array_elements_text( | |
| coalesce(hour_logs.metadata -> 'availableProviders', '[]'::jsonb) | |
| ) as provider_id | |
| union all | |
| select hour_logs.model_id, entry ->> 'providerId' as provider_id | |
| from hour_logs | |
| cross join lateral jsonb_array_elements( | |
| coalesce(hour_logs.metadata -> 'filteredProviders', '[]'::jsonb) | |
| ) as entry | |
| union all | |
| select hour_logs.model_id, provider_id | |
| from hour_logs | |
| cross join lateral jsonb_array_elements_text( | |
| coalesce(hour_logs.metadata -> 'contentFilterExcludedProviders', '[]'::jsonb) | |
| ) as provider_id | |
| ), | |
| union all | |
| -- rate-limited mappings, annotated on the score entries | |
| select | |
| hour_logs.model_id, | |
| score ->> 'providerId' as provider_id, | |
| 'rate_limited' as reason | |
| from hour_logs | |
| cross join lateral jsonb_array_elements( | |
| coalesce(hour_logs.metadata -> 'providerScores', '[]'::jsonb) | |
| ) as score | |
| where (score ->> 'rate_limited')::boolean is true | |
| ), | |
| -- Every provider this model saw in a routing decision, whether it was | |
| -- kept or dropped. This is the denominator: without it an exclusion count | |
| -- cannot be turned into a rate. | |
| candidates as ( | |
| select hour_logs.model_id, provider_id | |
| from hour_logs | |
| cross join lateral jsonb_array_elements_text( | |
| coalesce(hour_logs.metadata -> 'availableProviders', '[]'::jsonb) | |
| ) as provider_id | |
| union all | |
| select hour_logs.model_id, entry ->> 'providerId' as provider_id | |
| from hour_logs | |
| cross join lateral jsonb_array_elements( | |
| coalesce(hour_logs.metadata -> 'filteredProviders', '[]'::jsonb) | |
| ) as entry | |
| union all | |
| select hour_logs.model_id, provider_id | |
| from hour_logs | |
| cross join lateral jsonb_array_elements_text( | |
| coalesce(hour_logs.metadata -> 'contentFilterExcludedProviders', '[]'::jsonb) | |
| ) as provider_id | |
| union all | |
| select hour_logs.model_id, score ->> 'providerId' as provider_id | |
| from hour_logs | |
| cross join lateral jsonb_array_elements( | |
| coalesce(hour_logs.metadata -> 'providerScores', '[]'::jsonb) | |
| ) as score | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/worker/src/services/routing-telemetry-aggregator.ts` around lines 188 -
221, Add a fourth UNION ALL branch to the candidates CTE, alongside the existing
availableProviders, filteredProviders, and contentFilterExcludedProviders
branches, that extracts providerId from each metadata.providerScores entry. This
ensures providers represented in providerScores, including rate-limited
providers, contribute to candidateCount.
Comment on lines
+1243
to
+1251
| * Roll up a single hour of minute history into the hourly summary tables, plus | ||
| * the routing telemetry for that hour. Routing telemetry rides along here rather | ||
| * than on its own schedule so it is covered by the same backfill pass. | ||
| */ | ||
| async function calculateHistoryForHour(targetHour: Date) { | ||
| const mappingResult = await calculateMappingHistoryForHour(targetHour); | ||
| const modelResult = await calculateModelHistoryForHour(targetHour); | ||
| return { mappingResult, modelResult }; | ||
| const routingResult = await calculateRoutingTelemetryForHour(targetHour); | ||
| return { mappingResult, modelResult, routingResult }; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the backfill gate and any other caller of the hourly rollup.
rg -nP -C 6 'mappingHourSet|modelHourSet|calculateHistoryForHour' apps/worker/src
rg -nP -C 4 'calculateHourlyHistory|backfillHourlyHistoryIfNeeded' apps/worker/srcRepository: theopenco/llmgateway
Length of output: 19864
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Relevant function sections"
sed -n '1259,1390p' apps/worker/src/services/stats-calculator.ts
sed -n '1310,1350p' apps/worker/src/services/stats-calculator.ts
echo
echo "## Routing telemetry calculation and callers"
rg -n -C 8 'calculateRoutingTelemetryForHour|routing_election_hourly|routing_exclusion_hourly|election' apps/worker/src/services/stats-calculator.ts
echo
echo "## Tests around backfill/routing"
rg -n -C 8 'routing|election|exclusion|backfillHourlyHistoryIfNeeded' apps/worker/src/services/stats-calculator.spec.ts
echo
echo "## Read-only behavioral/invariant probe"
python3 - <<'PY'
from pathlib import Path
import re
p = Path("apps/worker/src/services/stats-calculator.ts")
s = p.read_text()
# Extract backfill gate condition.
m = re.search(r'if\s*\((.*?)\)\s*\{\s*const result = await calculateHistoryForHour\(hour\)', s, re.S)
if not m:
print("missing gate")
else:
print("gate_condition:")
print(" ".join(m.group(1).split()))
# Extract calculateRoutingTelemetryForHour implementation.
start = s.index("async function calculateRoutingTelemetryForHour")
end = s.index("function", start + 1)
body = s[start:end]
print("calculateRoutingTelemetryForHour_calls:")
for name in ["db.select", "routing_election_hourly", "routing_exclusion_hourly"]:
print(f" {name}: {body.count(name)}")
# Check whether calculateHistoryForHour awaits routing result.
calc = re.search(r'async function calculateHistoryForHour[\s\S]*?\n\}', s)
print("history_aids_routing_result:", "await calculateRoutingTelemetryForHour(targetHour)" in calc.group(0))
# Check whether calculateHistoryForHour returns routing result.
print("history_returns_routing_result:", "routingResult" in calc.group(0))
PYRepository: theopenco/llmgateway
Length of output: 50376
Include routing telemetry in the hourly backfill guard and handle its failure
backfillHourlyHistoryIfNeeded checks only mappingHourSet and modelHourSet; after prior rollups, those sets cover existing hours, so calculateHistoryForHour skips historical buckets and never backfills routing_election_hourly or routing_exclusion_hourly. Add a routing-telemetry presence check to the guard, and decide whether telemetry query failures should fail the rolling-up or backfilling stats flow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/worker/src/services/stats-calculator.ts` around lines 1243 - 1251,
Update backfillHourlyHistoryIfNeeded to include routing telemetry presence when
deciding whether an hourly bucket needs processing, so existing mapping/model
hours do not suppress routing backfills. Ensure calculateHistoryForHour
propagates or otherwise consistently handles calculateRoutingTelemetryForHour
failures in both rolling-up and backfilling flows, matching the existing error
behavior for the other history calculations.
Comment on lines
+881
to
+919
| {/* Eligibility and its dominant cause share one column: as two | ||
| separate columns they pushed Score past the visible width. */} | ||
| <TableCell className="text-right font-mono text-xs"> | ||
| {eligibility?.exclusionRate !== null && | ||
| eligibility?.exclusionRate !== undefined ? ( | ||
| <> | ||
| <span | ||
| className={cn( | ||
| eligibility.exclusionRate > 0.5 && | ||
| "font-semibold text-amber-600", | ||
| )} | ||
| > | ||
| {( | ||
| (1 - eligibility.exclusionRate) * | ||
| 100 | ||
| ).toFixed(0)} | ||
| % | ||
| </span> | ||
| {eligibility.topReason ? ( | ||
| <div | ||
| className="font-sans text-[11px] text-muted-foreground" | ||
| title={eligibility.exclusions | ||
| .map( | ||
| (entry) => | ||
| `${exclusionReasonLabel(entry.reason)}: ${entry.excludedCount.toLocaleString()}`, | ||
| ) | ||
| .join("\n")} | ||
| > | ||
| {exclusionReasonLabel(eligibility.topReason)} | ||
| {eligibility.exclusions.length > 1 | ||
| ? ` +${eligibility.exclusions.length - 1}` | ||
| : ""} | ||
| </div> | ||
| ) : null} | ||
| </> | ||
| ) : ( | ||
| "—" | ||
| )} | ||
| </TableCell> |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not derive eligibility from exclusionRate.
The API computes exclusionRate from exclusion events divided by candidateCount. One routing decision can emit several exclusion events. Therefore, 1 - exclusionRate is not the share of decisions where this mapping was eligible.
Expose a unique decision denominator and eligible count or an API-computed eligibility rate. Render that value here. Otherwise, the dashboard can show 0% eligible for a mapping that was eligible in some decisions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ee/admin/src/app/routing-analytics/client.tsx` around lines 881 - 919,
Replace the eligibility percentage calculation in the table cell with an
API-provided decision-level eligibility rate or a value computed from the unique
decision denominator and eligible count; do not use eligibility.exclusionRate or
1 - eligibility.exclusionRate. Update the backing API response/type to expose
the required fields, then render that authoritative value while preserving the
existing topReason display and empty-state behavior.
Regenerated after the merge per the migrations workflow, so the journal and snapshot never carry a hand-resolved conflict. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reconciles this branch with #3459, which landed the gateway half of this work independently: - Drops log.appliedServiceTier. #3459 made requestedServiceTier hold the tier the gateway actually asks for and put the origin in routingMetadata.serviceTierSource, so a second column was redundant. The service-tier counters now read both, which also leaves the `log` table untouched by this migration. - Drops preRoutingExclusions and mergeFilteredProviders in favour of #3459's preRoutingFilteredProviders and filteredProvidersMetadata, which cover more exclusion sites and are wired into every routing branch. - Keeps the stable exclusion codes the hourly rollup needs, but reshapes them: recordFilteredProvider now takes {code, message} pairs so #3459's request-specific prose (which tier, whether it was a plan default) is preserved verbatim while aggregation keys off the code. - Adds codes for #3459's new drops (coding_plan_cache, service_tier_key) and its new selection reason (single-candidate-after-filtering, mapped to a "narrowed" kind — the shape a filtered-out mapping produces). - Rebuilds the admin page additions on top of #3454's restyle instead of merging against it: stat cards for score-decided share, average candidates, tiered requests and exclusions, then the election/tier/ exclusion cards in the new card idiom. Migrations were reset to origin/main before merging and regenerated after, so no snapshot or journal conflict was hand-resolved. They are also split in two so the index creation can be decoupled from the deploy: - 1786133228_lowly_rage.sql creates the two tables (with the unique constraints the upserts need) and adds the history-table columns. - 1786133241_tiny_bishop.sql creates the four secondary indexes, adapted to CREATE INDEX IF NOT EXISTS. - migrations/manual/routing-telemetry-indexes.sql holds the same four as CREATE INDEX CONCURRENTLY, for running out-of-band. It cannot be a migration: drizzle runs every pending migration inside one transaction, and CONCURRENTLY is rejected there. Both were produced by pnpm migrations (by staging the index definitions in schema.ts), not hand-authored; only the IF NOT EXISTS is a .sql-level adaptation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/migrations/meta/_journal.json`:
- Around line 1560-1570: Reindent the new journal entries with tabs instead of
spaces, including the entries identified by idx 222 and idx 223 in the migration
journal. Preserve all values and structure while applying tab indentation
consistently.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b595ee2c-6a18-4ad3-9502-e114046dbac6
📒 Files selected for processing (16)
apps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/provider-filter-reasons.spec.tsapps/gateway/src/chat/tools/provider-filter-reasons.tsapps/worker/src/services/routing-telemetry-aggregator.spec.tsapps/worker/src/services/routing-telemetry-aggregator.tsapps/worker/src/services/stats-calculator.tsee/admin/src/app/routing-analytics/client.tsxpackages/actions/src/get-cheapest-from-available-providers.tspackages/db/migrations/1786133228_lowly_rage.sqlpackages/db/migrations/1786133241_tiny_bishop.sqlpackages/db/migrations/manual/routing-telemetry-indexes.sqlpackages/db/migrations/meta/1786133228_snapshot.jsonpackages/db/migrations/meta/1786133241_snapshot.jsonpackages/db/migrations/meta/_journal.jsonpackages/db/src/schema.tspackages/shared/src/routing-telemetry.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/actions/src/get-cheapest-from-available-providers.ts
- apps/worker/src/services/routing-telemetry-aggregator.ts
- apps/worker/src/services/routing-telemetry-aggregator.spec.ts
- apps/worker/src/services/stats-calculator.ts
- packages/db/src/schema.ts
- ee/admin/src/app/routing-analytics/client.tsx
- packages/shared/src/routing-telemetry.ts
- apps/gateway/src/chat/chat.ts
Second sync with main invalidated the previous pair, so both were regenerated on the new base by staging the index definitions in schema.ts and running pnpm migrations twice: - 1786134049_familiar_darkstar.sql — the two tables plus the history-table columns. Safe to auto-apply: on PG 17 an `ADD COLUMN <int> DEFAULT 0 NOT NULL` is metadata-only. - 1786134059_previous_tombstone.sql — the four secondary indexes, adapted to CREATE INDEX IF NOT EXISTS so it is a no-op wherever they were already built out-of-band. - migrations/manual/routing-telemetry-indexes.sql — the same four as CREATE INDEX CONCURRENTLY, for running async against production. Verified: the full chain applies from scratch, and the production sequence (migration 1 → concurrent script → migration 2) leaves exactly four valid secondary indexes with migration 2 as a no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/migrations/1786134059_previous_tombstone.sql`:
- Around line 1-9: Reset the migration state and regenerate the migration and
journal entries using the repository’s prescribed migration procedure, rather
than retaining the manually edited SQL in the migration file. Ensure the
regenerated artifacts in the migration set contain only tool-generated output
and preserve the intended schema changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d33d49a-f83f-4081-bcb4-b1bc3113f78a
📒 Files selected for processing (8)
apps/gateway/src/chat/chat.tspackages/db/migrations/1786134049_familiar_darkstar.sqlpackages/db/migrations/1786134059_previous_tombstone.sqlpackages/db/migrations/manual/routing-telemetry-indexes.sqlpackages/db/migrations/meta/1786134049_snapshot.jsonpackages/db/migrations/meta/1786134059_snapshot.jsonpackages/db/migrations/meta/_journal.jsonpackages/shared/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shared/src/index.ts
- packages/db/migrations/manual/routing-telemetry-indexes.sql
- apps/gateway/src/chat/chat.ts
Drops the two-migration split and the manual CREATE INDEX CONCURRENTLY script. The four indexes are on tables this migration creates empty, so there is nothing to build concurrently and nothing to lock. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pull Bot
pushed a commit
to soitun/llmgateway
that referenced
this pull request
Aug 8, 2026
Follow-up to the review feedback left on theopenco#3458, which merged before it was addressed. Six findings; five were still valid against current `main` and are fixed here, and the sixth needed a different fix than the one suggested. Three further findings from the review of this PR are fixed in the last commit. Every one of them is a counting bug in the same place: the denominators the new routing telemetry divides by. ## Eligibility was computed per reason instead of per decision `exclusionRate` summed the per-reason exclusion counts and divided by the candidate count. One routing decision can drop a mapping for several reasons at once, so the numerator counts constraints while the denominator counts requests. A mapping that was a candidate in 10 decisions and dropped from 5 of them, twice over each time, reported 10/10 — **0% eligible for a mapping that served half its requests.** The existing `Math.min(…, 1)` cap did not fix this; it is what made the bug look like a rounding artifact instead of a wrong answer. The aggregator now records `excludedDecisionCount` alongside the per-reason counts: each request counted once, whatever it tripped. Eligibility derives from that. The per-reason breakdown is unchanged and still sums past it — that is the point of keeping the two separate, and both column comments say so. This is the one finding where the suggested fix (recompute eligibility in the dashboard) would not have worked: the decision count does not exist in the API response, so it had to come from the rollup. ## Rate-limited mappings had no denominator at all The exclusion candidate set unioned `availableProviders`, `filteredProviders` and `contentFilterExcludedProviders`, but `rate_limited` is read from `providerScores` — and a rate-limited mapping is often only ever recorded there (`chat.ts:4496-4517` pushes a fresh score entry when the provider is not already scored). With no candidate row, `candidate_count` fell back to the exclusion count and every such mapping reported a flat 100% exclusion rate. `providerScores` is now a fourth branch. It overlaps the others — a rate-limited mapping can appear in `availableProviders` too — so the candidate set is deduplicated per decision (`select distinct log_id, …`) rather than unioned blind, which would have halved the rate instead of fixing it. ## Retried requests were counted twice `calculateHistoryForMinute` and `calculateModelHistoryForMinute` both filter `log` with `excludeRecoveredSameProviderRegionRetry()`; the election rollup did not. A request that failed and recovered on the same provider and region contributed two `log` rows to the election counts and one to mapping history, so the two tables disagreed about how many requests the same mapping served. The predicate now applies to the election and exclusion rollups too, and moves to `apps/worker/src/services/log-filters.ts` — importing it from `stats-calculator.ts` would have been circular, since that module imports the aggregator. ## Non-deterministic candidate denominator in the analytics API `candidateCount` is repeated on every reason row of a mapping-hour, and the API kept whichever row arrived first. The query has no `ORDER BY`, and the schema comment already says to read the column with `max()`. If a partially rerun aggregation leaves two rows of one bucket disagreeing, the reported denominator depends on row order. Now reduced with `max()` per `(hour, provider)` before summing across hours, for both repeated columns. ## Routing telemetry would never have backfilled `backfillHourlyHistoryIfNeeded` decides whether to recompute an hour by checking `modelProviderMappingHistoryHourly` and `modelHistoryHourly`. Routing telemetry shipped after both, so on the first pass after deploy every historical hour is present in those two and absent from routing — the guard skips the hour and the routing tables stay empty. Riding along inside `calculateHistoryForHour` is what made this invisible: the call was there, it just never ran for a historical hour. The guard now checks `routing_election_hourly` as well, bounded to hours at or after the oldest surviving log row. Hours whose logs retention has already pruned can never produce routing rows, and requiring them would recompute the same empty hours on every worker start. While in there: `calculateRoutingTelemetryForHour` failures no longer propagate. Routing telemetry is diagnostic and reads `log`, while the two rollups above read minute history — a failure in it should not cost the hour its usage and cost stats. It is logged and skipped, which leaves the hour absent from `routing_election_hourly`, which is exactly what makes the next backfill retry it. ## The rollup is now atomic Making the election table the completion marker, while letting the two aggregations fail independently, opened a hole: elections are persisted before exclusions, so an exclusion failure left the hour looking complete with its exclusion rows permanently missing. Both aggregations now share one transaction, so the hour lands all-or-nothing and a failure leaves it absent and therefore retryable. One known, bounded cost remains: an hour that has no logs at all, but sits inside the retained log range, produces no election row and is rescanned on every backfill pass. Both queries are indexed range scans returning nothing, and the range is capped by log retention, so this is a handful of empty queries at worker start rather than something worth a completion table. ## Legacy rows Existing `routing_exclusion_hourly` rows would sit at the column default of 0 and read as *never excluded* — 100% eligible, the exact inverse of the bug this fixes — and the backfill above does not reach them, because those hours are already present in `routing_election_hourly`. The migration therefore seeds each row with its own reason count. The API reduces the column with `max()` across a mapping-hour's reasons, so the seeded value becomes the largest single reason's count: a lower bound on decisions excluded, exact whenever one reason dominates the hour. The minutely rollup rewrites the current and previous hour unconditionally, so recent hours get real counts within minutes and older ones age out of the window. ## Also - Guarded the destructured `.returning()` result in the aggregator spec, per the review note. - Corrected the `exclusionRate` nullability doc: candidate totals come only from `routing_exclusion_hourly`, which has no row for a mapping that was never excluded, so `null` means "no exclusion telemetry", not "no routing decisions". ## Schema One additive column plus its data backfill: ```sql ALTER TABLE "routing_exclusion_hourly" ADD COLUMN "excluded_decision_count" integer DEFAULT 0 NOT NULL; UPDATE "routing_exclusion_hourly" SET "excluded_decision_count" = "excluded_count"; ``` Generated with `pnpm migrations`; only the data backfill was added to the generated `.sql`. `main` landed a migration of its own partway through, so this was regenerated via the repo's reset-and-regenerate procedure rather than by resolving the journal by hand. ## Testing `pnpm format` and `pnpm build` pass. - `apps/worker/src/services/routing-telemetry-aggregator.spec.ts` — 13/13 (4 added). New: a decision counted once when several reasons fire; a rate-limited mapping getting a real denominator from its score entries, including the dedup when it appears in both branches; a same-provider retry counting as one election; and no election rows left behind when the exclusion pass fails. - `apps/api/src/routes/admin-routing-analytics.spec.ts` — 7/7. The old "caps the rate at 100%" test is replaced by one asserting the 5-of-10-decisions case reports 50% rather than the capped 100%, plus a test that a disagreeing bucket resolves to the larger denominator. - Full `apps/worker` suite: 117/117. Full `apps/api` suite: 838 passed, 1 skipped. No dashboard change: `ee/admin` renders `1 - exclusionRate`, which is correct once the rate is decision-level. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds routing telemetry so the admin dashboard can answer two questions it previously could not: what share of requests carry a service tier, and what share of routing candidates get filtered out, and why.
The trigger: on
gpt-5.6-sol, Azure scores-0.986(best of three mappings) yet serves 5% of traffic while OpenAI scores+0.453and serves 88%. The score table looked wrong. It wasn't — Azure's mapping declares noserviceTiers, so every request carrying a tier (including the coding-planflexdefault) drops Azure from the candidate set roughly a thousand lines before any score is computed.#3459 has since made the gateway log that per request. This PR makes it queryable in aggregate, so the question can be answered for a model over a window instead of by opening one log at a time.
Everything here is derived from data routing already produces. No new gateway hot-path work, no per-request writes.
Schema
Two aggregate tables, both keyed on closed enums so a future code path cannot explode their cardinality:
routing_election_hourly—(hour, model, provider, selectionReason)→requestCount,candidateCount, service-tier split.1 - scored/totalis the "score was never consulted" share;candidateCount / requestCountis the average candidate-set size.routing_exclusion_hourly—(hour, model, provider, reason)→excludedCount,candidateCount. The ratio is "for this mapping, what share of the decisions it could have served was it not eligible for, and which constraint was responsible."Row volume is models × providers × reasons × 24 — low thousands per day, against millions of log rows.
Plus four counters —
serviceTierExplicitCount/ImplicitCount/ServedCount/UnconfirmedCount— onmodel_provider_mapping_history{,_hourly}andmodel_history{,_hourly}.No new
logcolumn. An earlier revision addedlog.appliedServiceTier, becauserequestedServiceTierwas snapshotted before the coding-plan default ran and recorded the implicitflexasNULL. #3459 fixed that at the source:requestedServiceTiernow holds the tier the gateway actually asks for, androutingMetadata.serviceTierSourcesays whether it came from the request or the plan default. The counters read both, so the largest table in the schema is untouched by this migration.unconfirmedis deliberately not called a downgrade: a nullusedServiceTiermeans either Google downgraded to standard or the provider reports no tier at all (Fireworks). Both are billed at the standard rate, which is what makes the count worth watching either way.Exclusion reason codes
filteredProviders[].reasonsis free-form prose, and several call sites interpolate request detail into it (which tier, whether it came from a plan default) — #3459's specs assert that exact wording, and the log detail view shows it. Prose is therefore unusable as an aggregation key, but rewriting it would be a regression.So
recordFilteredProvidernow takes{code, message}pairs: the message is stored exactly as before, and a parallelcodesarray carries a closed-enum key for the rollup.exclusionReason(code)defaults the message to the code's canonical wording for the sites that have nothing specific to add.Codes are recorded at every exclusion site, including #3459's (
service_tier,service_tier_key,coding_plan_cache,compliance) and four the routing-time filter had left silent (no_provider_key,locked_region,deprecated,context_size).rate_limitedandcontent_filterneeded no gateway change — routing already records them inproviderScores[].rate_limitedandcontentFilterExcludedProviders, and the rollup maps those onto the same codes so every exclusion lands in one place.One case is deliberately not recorded: the non-reasoning-variant preference. The provider still serves the request via its other variant, so recording it would make a working mapping look ineligible.
Aggregation
calculateRoutingTelemetryForHourrides along in the existing hourly rollup, so it is covered by the same backfill pass. It readslog, which is safe here in a way it would not be for a dashboard query:routingMetadataand the tier columns survive retention stripping (log-retention.tsonly nulls payload fields), it runs once per hour over one hour's rows, and the dashboard only ever reads the hourly output. Backfilling an hour whose logs are already pruned produces no rows and leaves existing ones untouched.One trap worth flagging for future raw-SQL work: a JS
Datebound into a rawsqltemplate becomes an ISO string with a trailingZand silently matches zero rows against atimestamp without time zonecolumn. The drizzle query builder is unaffected.formatUTCTimestamp+::timestampis the existing convention and is now reused.Dashboard
Rebuilt on top of #3454's restyle rather than merged against it, so the new cards use the current card/stat-card idiom.
/routing-analyticsgains:Scorepast the visible width.Testing
New/updated specs, all passing:
apps/worker/src/services/routing-telemetry-aggregator.spec.ts— 9 specs against Postgres: reason/kind splits, candidate denominators,unknown/otherfolding for unrecognized values, pre-codes rows contributing a candidate but no reason, idempotency, hour-boundary filtering.apps/api/src/routes/admin-routing-analytics.spec.ts— 2 added (6/6 pass): the derived election/eligibility/service-tier payload, and that a request firing several reasons on one mapping caps the exclusion rate at 100% instead of reporting 200%.apps/gateway/src/chat/tools/provider-filter-reasons.spec.ts— updated for codes, plus a check that every code maps to a message.pnpm formatandpnpm buildpass. The page was driven end-to-end in a browser against a local API with syntheticgpt-5.6-soltraffic (that is the screenshot below); the exclusion/election SQL was verified against real Postgres, including idempotency of a re-run hour.On the full
pnpm test:unitrun: it reported 3 failures, infallback.spec.tsandvideos.spec.ts. These do not reproduce and are not from this change:videos.spec.tspasses 48/48 when run alone on a clean database.fallback.spec.tspasses 54/54 when re-run alone. An earlier isolated run of the same commit failed a different pair of tests (429 … classified as upstream_errorexpecting 500 but getting 403;retries after random explorationseeing 1 attempt instead of ≥2) — three runs of identical code, three different outcomes. A regression would fail the same test every time.Worth a maintainer's eye anyway if these are not already known flakes.
Screenshot
🤖 Generated with Claude Code