fix(metrics): bucket streaming request counts - #1059
lloydmak99 wants to merge 1 commit into
Conversation
Streaming counters were emitted before provider usage and therefore carried only model and environment tags, which surfaced as N/A in Grafana. Interrupted streams could never be backfilled. Reuse the existing routing token estimate for admission metrics, then replace that estimate with provider-reported usage for completion-time metrics so every tag set contains exactly one input bucket. Co-Authored-By: OpenAI Codex <noreply@openai.com>
GitHub Actions
Failure
Review · Status🟩 CompletedIronLoop completed the review and posted it to GitHub. ResultRun detailsAutomatic trigger · attempt 1 of 3 · completed in 23m 50s |
|
Review — No prior human review threads on this PR (only the IronLoop status comment), so nothing to build on. Mechanically the change is sound: Three issues worth addressing before merge.
That was fine as a routing hint. As a metric label it is a structural undercount, not within-bucket noise:
This is not an edge case, because The repo already has the correct estimator — Suggested: use it for the metric bucket and leave // inference_provider_pool/mod.rs:25 — needs `pub(crate) mod context_routing;`
let est = context_routing::estimate_input(&chat_params);
let estimated_input_tokens = (est.countable_tokens + est.uncounted_tokens).min(u32::MAX as u64) as u32;If you would rather not widen that module visibility this round, at minimum add the
After this change It also breaks within-request consistency: Consider a distinguishing tag, e.g.
Related: this makes Also, the checklist claims "Inline metric semantics updated" but
Nit
Not an issue
I could not execute 🤖 Generated with Claude Code |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let prefix = format!("{TAG_INPUT_BUCKET}:"); | ||
| metric_tags.retain(|tag| !tag.starts_with(&prefix)); | ||
| metric_tags.push(format!("{prefix}{input_bucket}")); |
There was a problem hiding this comment.
This formats a fresh prefix String on every call, and the helper now runs twice per streaming request (admission via record_stream_admission_metrics, then again at usage time). The retain pass can match the tag prefix without allocating by using strip_prefix with a separator check; the only unavoidable allocation left is the final push. Alternatively, hoist the prefix into a const (e.g. TAG_INPUT_BUCKET_PREFIX in metrics/consts.rs) so it is built once.
Suggestion:
| let prefix = format!("{TAG_INPUT_BUCKET}:"); | |
| metric_tags.retain(|tag| !tag.starts_with(&prefix)); | |
| metric_tags.push(format!("{prefix}{input_bucket}")); | |
| metric_tags.retain(|tag| { | |
| !tag.strip_prefix(TAG_INPUT_BUCKET) | |
| .is_some_and(|rest| rest.starts_with(':')) | |
| }); | |
| metric_tags.push(format!("{TAG_INPUT_BUCKET}:{input_bucket}")); |
| model_name: &str, | ||
| estimated_input_tokens: u32, | ||
| ) -> Vec<String> { | ||
| let input_bucket = i32::try_from(estimated_input_tokens).map_or("128k+", get_input_bucket); |
There was a problem hiding this comment.
The fallback "128k+" duplicates the top-bucket label that already lives in get_input_bucket's catch-all arm. If the bucket boundaries/labels ever change (e.g. a "1m+" bucket is added), this hardcoded string silently drifts out of sync. Since any value that fails i32::try_from would map to the top bucket anyway, clamping and letting get_input_bucket produce the label keeps a single source of truth for bucket names.
Suggestion:
| let input_bucket = i32::try_from(estimated_input_tokens).map_or("128k+", get_input_bucket); | |
| let input_bucket = | |
| get_input_bucket(i32::try_from(estimated_input_tokens).unwrap_or(i32::MAX)); |
| metrics_service.record_count(METRIC_REQUEST_COUNT, 1, &tags); | ||
| metrics_service.record_latency(METRIC_LATENCY_QUEUE_TIME, queue_time, &tags); |
There was a problem hiding this comment.
With this change, METRIC_REQUEST_COUNT and METRIC_LATENCY_QUEUE_TIME now carry an input_bucket whose meaning differs by code path: streaming buckets by the request-estimated token count at admission (here), while the non-streaming path buckets by actual prompt_tokens at completion (see the tokio::spawn in create_chat_completion). Any dashboard grouping these metric names by input_bucket will silently mix the two populations. The codebase is careful about this distinction elsewhere (dedicated ..._BY_INPUT TTFT series plus an explanatory comment), so a brief note here — or a distinct tag key for the estimated series — would prevent future misreads of the telemetry.
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review · Summary
🟢 No actionable findings
No actionable issues found.
Validation
- ✅ Focused services metric tests — Admission-bucket and final-usage replacement tests passed.
- ✅ Formatting — Rust formatting check passed.
Review details
- Run:
a331434e-82cb-4b48-958b-47504a49023d - Attempts: 1
1 participant
Why
Grafana renders Cloud API request-count series without an
input_bucketlabel asN/A. Streaming requests emittedcloud_api.request.countat admission with only model and environment tags, before provider usage existed; interrupted streams could never be backfilled.What
Reuse the existing request-side routing estimate to attach a bounded context bucket to streaming admission metrics. When provider-reported usage becomes available, replace the estimate for completion-time metrics so each tag set contains exactly one
input_bucket. Non-streaming request counts continue to use actual provider prompt tokens.This does not change which requests the historical counter includes. Requests rejected before its existing writers remain outside the metric.
How to test
Expected: formatting, build, and Clippy exit 0; service tests report 660 passed and 1 ignored; focused bucket and stream-replacement tests pass.
Tier
Checklist
Risks / rollback
Streaming request-count buckets are request-side estimates because the counter must be recorded before stream completion; completed-stream metrics still use provider-reported prompt tokens. This may shift existing context-distribution dashboards and should be verified after rollout by checking that new
input_bucket=""samples stop across both production producers.Rollback by reverting commit
eecf15ab4b8eee5b34e9c7383ce55ae32f084832. Existing unlabeled historical series remain queryable until they age out of dashboard windows.