feat: adds bifrost latency data to connectors - #5467
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (31)
🚧 Files skipped from review as they are similar to previous changes (28)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds per-request upstream latency accumulation, routes provider calls through instrumented helpers, exposes latency in traces, responses, and headers, computes Bifrost overhead, and records overhead in OTel and Prometheus telemetry. ChangesUpstream Latency and Overhead Instrumentation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
f9e0b06 to
f4f9848
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@core/schemas/upstreamlatency.go`:
- Around line 29-56: Update ResetUpstreamLatency so it never replaces the
accumulator on the shared fallback context used by nil callers; ensure each
request receives a distinct context/accumulator before reset, or otherwise keep
upstream latency state request-scoped. Preserve AddUpstreamLatency’s
context-based lookup and ensure response telemetry cannot combine or
misattribute concurrent nil-context requests.
In `@plugins/telemetry/main.go`:
- Around line 1039-1048: Update PostLLMHook so the cloned overheadLabels are
stored via ctx.SetValue before launching the asynchronous goroutine, only for
the final stream or non-stream request. Keep HTTPTransportPostHook able to read
those labels immediately, and retain direct SDK overhead recording inside the
goroutine.
In `@transports/bifrost-http/integrations/utils.go`:
- Around line 300-304: Update the upstream-latency header logic around
GetUpstreamLatency so the header is emitted only when an upstream duration was
actually measured, not merely when the accumulator exists. Treat zero or
otherwise unrecorded latency as absent while preserving the existing millisecond
formatting for valid measurements.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 00f65123-b5e3-4755-b599-27b1a3866aec
📒 Files selected for processing (28)
core/bifrost.gocore/mcp/toolmanager.gocore/providers/anthropic/anthropic.gocore/providers/azure/azure.gocore/providers/bedrock/bedrock.gocore/providers/cohere/cohere.gocore/providers/elevenlabs/elevenlabs.gocore/providers/gemini/gemini.gocore/providers/huggingface/huggingface.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/replicate/utils.gocore/providers/sarvam/sarvam.gocore/providers/utils/fetch.gocore/providers/utils/utils.gocore/providers/vertex/vertex.gocore/providers/vllm/vllm.gocore/schemas/bifrost.gocore/schemas/context.gocore/schemas/trace.gocore/schemas/upstreamlatency.goframework/tracing/tracer.goplugins/otel/converter.goplugins/otel/main.goplugins/otel/metrics.goplugins/telemetry/main.gotransports/bifrost-http/integrations/utils.go
f4f9848 to
f5ee311
Compare
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 `@core/providers/bedrock/bedrock.go`:
- Line 313: Update the Bedrock request flows using providerUtils.DoHTTPRequest
so upstream latency spans both HTTP execution and response-body consumption,
rather than stopping at response headers. Apply this consistently in
core/providers/bedrock/bedrock.go at lines 313, 410, 734, 815, 2483, 2612, 2822,
2903, 3108, 3233, 3351, 3413, and 3557, covering foundation/agent/Mantle calls,
S3 operations, and batch operations; preserve each method’s existing body and
error handling while recording the elapsed time only after the relevant body
read completes.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e26c08f2-bae8-4105-8505-be6c33318b7b
📒 Files selected for processing (28)
core/bifrost.gocore/mcp/toolmanager.gocore/providers/anthropic/anthropic.gocore/providers/azure/azure.gocore/providers/bedrock/bedrock.gocore/providers/cohere/cohere.gocore/providers/elevenlabs/elevenlabs.gocore/providers/gemini/gemini.gocore/providers/huggingface/huggingface.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/replicate/utils.gocore/providers/sarvam/sarvam.gocore/providers/utils/fetch.gocore/providers/utils/utils.gocore/providers/vertex/vertex.gocore/providers/vllm/vllm.gocore/schemas/bifrost.gocore/schemas/context.gocore/schemas/trace.gocore/schemas/upstreamlatency.goframework/tracing/tracer.goplugins/otel/converter.goplugins/otel/main.goplugins/otel/metrics.goplugins/telemetry/main.gotransports/bifrost-http/integrations/utils.go
🚧 Files skipped from review as they are similar to previous changes (26)
- core/providers/utils/fetch.go
- core/schemas/context.go
- core/providers/vllm/vllm.go
- core/providers/elevenlabs/elevenlabs.go
- core/providers/vertex/vertex.go
- core/providers/cohere/cohere.go
- core/providers/mistral/mistral.go
- core/providers/replicate/replicate.go
- core/providers/azure/azure.go
- core/providers/replicate/utils.go
- framework/tracing/tracer.go
- core/providers/sarvam/sarvam.go
- core/schemas/bifrost.go
- core/mcp/toolmanager.go
- core/providers/huggingface/huggingface.go
- core/schemas/trace.go
- core/providers/gemini/gemini.go
- core/providers/openai/openai.go
- transports/bifrost-http/integrations/utils.go
- core/schemas/upstreamlatency.go
- core/providers/anthropic/anthropic.go
- core/bifrost.go
- core/providers/utils/utils.go
- plugins/telemetry/main.go
- plugins/otel/main.go
- plugins/otel/converter.go
f5ee311 to
18c4935
Compare
Merge activity
|
## Summary Introduces a per-request upstream latency accumulator that tracks cumulative time Bifrost spends blocked on provider sockets across every attempt, retry, fallback, MCP tool call, and media fetch. Subtracting this from total wall time gives Bifrost's own processing overhead — a number that was previously impossible to derive accurately. ## Changes - **New `upstreamlatency.go` schema**: Installs an `*atomic.Int64` accumulator on the `BifrostContext` once per request. Uses an atomic pointer so streaming goroutines can keep writing after the request handler returns without touching the context's value map. - **`ResetUpstreamLatency` / `AddUpstreamLatency` / `GetUpstreamLatency`**: Core API for the accumulator. `Reset` is mandatory at request entry because Bifrost reuses a single process-global context for nil-ctx SDK callers; without it the counter would grow unboundedly. - **`DoStreamingRequest` / `DoHTTPRequest` helpers**: Thin wrappers around `fasthttp.Client.Do` and `net/http.Client.Do` that record the call duration as upstream latency. All provider call sites are migrated to these helpers. - **`idleTimeoutReader.Read` instrumentation**: Each blocking read in a streaming response is counted as upstream time, covering the token-generation window that `DoStreamingRequest` (which returns at first byte) cannot see. - **MCP tool call instrumentation**: `executeToolInternal` wraps `CallTool` with the same accumulator, since waiting on an MCP server is upstream time, not Bifrost overhead. - **`FetchAndEncodeURL` instrumentation**: Remote media fetches are counted as upstream, preventing multi-second fetches from appearing as Bifrost overhead. - **`StampUpstreamLatency` / `PopulateUpstreamLatency`**: Write the accumulated total onto the root trace span (`bifrost.upstream.duration_ms`) and onto `BifrostResponseExtraFields.UpstreamLatency` respectively. Both are called via a named-return `defer` in `handleRequest` so they fire even on error paths. - **`Trace.StampOverheadDuration`**: Computes `bifrost.overhead.duration_ms = root_span_duration - upstream_total` on the export snapshot, after the root span has ended. Clamped at zero to absorb clock skew. - **OTel plugin**: Reads `AttrBifrostOverheadDurationMs` from the root span and records it as a new `bifrost_overhead_latency_seconds` histogram with fine-grained sub-millisecond buckets appropriate for processing overhead rather than network latency. - **Prometheus plugin**: Records the same overhead histogram via the `HTTPTransportPreHook`/`HTTPTransportPostHook` window (widest available, matching the OTel root span). Falls back to the `PostLLMHook` window for SDK callers that bypass the transport layer. - **HTTP transport**: Emits `x-bifrost-upstream-latency-ms` response header so proxy callers can derive overhead from their own elapsed time without parsing the response body. - **New trace attributes**: `bifrost.upstream.duration_ms` and `bifrost.overhead.duration_ms` added to the attribute constant set. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./... ``` - Make a request through the HTTP transport and verify the `x-bifrost-upstream-latency-ms` response header is present and less than the total elapsed time. - Make a streaming request and confirm the header value grows to reflect the full generation window, not just time-to-first-byte. - Make a request that triggers a fallback and confirm the upstream latency reflects the sum of both attempts. - In OTel/Prometheus dashboards, verify `bifrost_overhead_latency_seconds` appears and that its values are in the sub-millisecond to low-tens-of-milliseconds range for healthy requests. - Confirm `bifrost.upstream.duration_ms` and `bifrost.overhead.duration_ms` appear on root spans in exported traces. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. The upstream latency value is derived from internal timing and contains no secrets or PII. The new response header exposes only a duration in milliseconds. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
Introduces a per-request upstream latency accumulator that tracks cumulative time Bifrost spends blocked on provider sockets across every attempt, retry, fallback, MCP tool call, and media fetch. Subtracting this from total wall time gives Bifrost's own processing overhead — a number that was previously impossible to derive accurately. - **New `upstreamlatency.go` schema**: Installs an `*atomic.Int64` accumulator on the `BifrostContext` once per request. Uses an atomic pointer so streaming goroutines can keep writing after the request handler returns without touching the context's value map. - **`ResetUpstreamLatency` / `AddUpstreamLatency` / `GetUpstreamLatency`**: Core API for the accumulator. `Reset` is mandatory at request entry because Bifrost reuses a single process-global context for nil-ctx SDK callers; without it the counter would grow unboundedly. - **`DoStreamingRequest` / `DoHTTPRequest` helpers**: Thin wrappers around `fasthttp.Client.Do` and `net/http.Client.Do` that record the call duration as upstream latency. All provider call sites are migrated to these helpers. - **`idleTimeoutReader.Read` instrumentation**: Each blocking read in a streaming response is counted as upstream time, covering the token-generation window that `DoStreamingRequest` (which returns at first byte) cannot see. - **MCP tool call instrumentation**: `executeToolInternal` wraps `CallTool` with the same accumulator, since waiting on an MCP server is upstream time, not Bifrost overhead. - **`FetchAndEncodeURL` instrumentation**: Remote media fetches are counted as upstream, preventing multi-second fetches from appearing as Bifrost overhead. - **`StampUpstreamLatency` / `PopulateUpstreamLatency`**: Write the accumulated total onto the root trace span (`bifrost.upstream.duration_ms`) and onto `BifrostResponseExtraFields.UpstreamLatency` respectively. Both are called via a named-return `defer` in `handleRequest` so they fire even on error paths. - **`Trace.StampOverheadDuration`**: Computes `bifrost.overhead.duration_ms = root_span_duration - upstream_total` on the export snapshot, after the root span has ended. Clamped at zero to absorb clock skew. - **OTel plugin**: Reads `AttrBifrostOverheadDurationMs` from the root span and records it as a new `bifrost_overhead_latency_seconds` histogram with fine-grained sub-millisecond buckets appropriate for processing overhead rather than network latency. - **Prometheus plugin**: Records the same overhead histogram via the `HTTPTransportPreHook`/`HTTPTransportPostHook` window (widest available, matching the OTel root span). Falls back to the `PostLLMHook` window for SDK callers that bypass the transport layer. - **HTTP transport**: Emits `x-bifrost-upstream-latency-ms` response header so proxy callers can derive overhead from their own elapsed time without parsing the response body. - **New trace attributes**: `bifrost.upstream.duration_ms` and `bifrost.overhead.duration_ms` added to the attribute constant set. - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ```sh go test ./... ``` - Make a request through the HTTP transport and verify the `x-bifrost-upstream-latency-ms` response header is present and less than the total elapsed time. - Make a streaming request and confirm the header value grows to reflect the full generation window, not just time-to-first-byte. - Make a request that triggers a fallback and confirm the upstream latency reflects the sum of both attempts. - In OTel/Prometheus dashboards, verify `bifrost_overhead_latency_seconds` appears and that its values are in the sub-millisecond to low-tens-of-milliseconds range for healthy requests. - Confirm `bifrost.upstream.duration_ms` and `bifrost.overhead.duration_ms` appear on root spans in exported traces. - [ ] Yes - [x] No None. The upstream latency value is derived from internal timing and contains no secrets or PII. The new response header exposes only a duration in milliseconds. - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Introduces a per-request upstream latency accumulator that tracks cumulative time Bifrost spends blocked on provider sockets across every attempt, retry, fallback, MCP tool call, and media fetch. Subtracting this from total wall time gives Bifrost's own processing overhead — a number that was previously impossible to derive accurately.
Changes
upstreamlatency.goschema: Installs an*atomic.Int64accumulator on theBifrostContextonce per request. Uses an atomic pointer so streaming goroutines can keep writing after the request handler returns without touching the context's value map.ResetUpstreamLatency/AddUpstreamLatency/GetUpstreamLatency: Core API for the accumulator.Resetis mandatory at request entry because Bifrost reuses a single process-global context for nil-ctx SDK callers; without it the counter would grow unboundedly.DoStreamingRequest/DoHTTPRequesthelpers: Thin wrappers aroundfasthttp.Client.Doandnet/http.Client.Dothat record the call duration as upstream latency. All provider call sites are migrated to these helpers.idleTimeoutReader.Readinstrumentation: Each blocking read in a streaming response is counted as upstream time, covering the token-generation window thatDoStreamingRequest(which returns at first byte) cannot see.executeToolInternalwrapsCallToolwith the same accumulator, since waiting on an MCP server is upstream time, not Bifrost overhead.FetchAndEncodeURLinstrumentation: Remote media fetches are counted as upstream, preventing multi-second fetches from appearing as Bifrost overhead.StampUpstreamLatency/PopulateUpstreamLatency: Write the accumulated total onto the root trace span (bifrost.upstream.duration_ms) and ontoBifrostResponseExtraFields.UpstreamLatencyrespectively. Both are called via a named-returndeferinhandleRequestso they fire even on error paths.Trace.StampOverheadDuration: Computesbifrost.overhead.duration_ms = root_span_duration - upstream_totalon the export snapshot, after the root span has ended. Clamped at zero to absorb clock skew.AttrBifrostOverheadDurationMsfrom the root span and records it as a newbifrost_overhead_latency_secondshistogram with fine-grained sub-millisecond buckets appropriate for processing overhead rather than network latency.HTTPTransportPreHook/HTTPTransportPostHookwindow (widest available, matching the OTel root span). Falls back to thePostLLMHookwindow for SDK callers that bypass the transport layer.x-bifrost-upstream-latency-msresponse header so proxy callers can derive overhead from their own elapsed time without parsing the response body.bifrost.upstream.duration_msandbifrost.overhead.duration_msadded to the attribute constant set.Type of change
Affected areas
How to test
go test ./...x-bifrost-upstream-latency-msresponse header is present and less than the total elapsed time.bifrost_overhead_latency_secondsappears and that its values are in the sub-millisecond to low-tens-of-milliseconds range for healthy requests.bifrost.upstream.duration_msandbifrost.overhead.duration_msappear on root spans in exported traces.Breaking changes
Related issues
Security considerations
None. The upstream latency value is derived from internal timing and contains no secrets or PII. The new response header exposes only a duration in milliseconds.
Checklist
docs/contributing/README.mdand followed the guidelines