feat(server): export HTTP request duration metrics via OTLP - #1565
feat(server): export HTTP request duration metrics via OTLP#1565gegemeimingzi wants to merge 3 commits into
Conversation
Add a server.http.request.duration histogram recorded by a thin ASGI middleware, so direct REST clients and requests that fail before a lifecycle operation produce Server-side request telemetry. The histogram uses the matched route template (not the raw URL path) for low cardinality, and covers successful responses, auth failures, validation errors, unmatched routes, and unhandled exceptions. Closes opensandbox-group#1560 Co-Authored-By: Claude <noreply@anthropic.com>
github-actions
Bot
added
component/server
size/L
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08016ceaa7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # even when set_meter_provider() cannot override a preexisting global provider. | ||
| _meter_provider = provider | ||
| _create_duration_histogram = _histogram_from_provider(provider) | ||
| _http_request_duration_histogram = _http_request_histogram_from_provider(provider) |
There was a problem hiding this comment.
Because _http_request_duration_histogram is omitted from the global declaration in setup_otel_metrics(), this assignment creates a local variable. The module-level instrument read by record_http_request_duration() therefore remains None, so every HTTP sample is discarded even when OTEL export is enabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed. _http_request_duration_histogram is now declared global in setup_otel_metrics so the module-scope instrument is actually bound; the previous assignment created a local and discarded every HTTP sample. Added test_setup_binds_http_request_histogram, which fails without this fix.
| response = await call_next(request) | ||
| return response | ||
| finally: | ||
| duration_ms = (time.perf_counter() - start) * 1000.0 |
There was a problem hiding this comment.
When an endpoint returns a streaming response—most notably the lifecycle proxy's _ProxyStreamingResponse—BaseHTTPMiddleware.call_next() returns after the response starts, and this finally block records before the body is consumed. Long downloads and streams are consequently reported as only backend time-to-headers rather than request duration; record after the downstream ASGI application completes, for example by wrapping send, so the histogram includes the full response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Rewrote HttpRequestMetricsMiddleware as a pure ASGI middleware that awaits the downstream app to completion, so streaming responses (e.g. the lifecycle proxy) are measured for their full duration instead of time-to-headers. Added test_records_full_streaming_duration, which asserts a 200ms streamed body is included in the recorded duration.
| 60000.0, | ||
| ) | ||
|
|
||
| _HTTP_REQUEST_DURATION_HISTOGRAM_NAME = "server.http.request.duration" |
There was a problem hiding this comment.
This adds operations-visible telemetry, but the [otel] section of server/configuration.md still describes the integration as exporting only SDK-reported sandbox-creation latency and does not document this metric, its attributes, or units. Update the operations documentation alongside the behavior so operators can discover and consume the newly exported series.
AGENTS.md reference: AGENTS.md:L79-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Documented both exported metrics (opensandbox.sandbox.create.duration and server.http.request.duration) with their types, units, and attributes in the [otel] section of server/configuration.md.
…ent metric - Declare _http_request_duration_histogram global in setup_otel_metrics so the module-scope instrument is actually bound; previously the assignment created a local and every HTTP sample was discarded. - Switch HttpRequestMetricsMiddleware to a pure ASGI middleware that awaits the downstream app to completion, so streaming responses (e.g. the proxy) are measured for their full duration rather than time-to-headers. - Add a setup regression test (histogram binding) and a streaming-duration test; document both exported metrics in configuration.md [otel]. Co-Authored-By: Claude <noreply@anthropic.com>
setup_otel_metrics only unbound _create_duration_histogram in the disabled branch; _http_request_duration_histogram could remain bound to a stale provider after an enabled setup. Unbind both and cover it with a regression test. Co-Authored-By: Claude <noreply@anthropic.com>
2 participants
Closes #1560
What
Adds a
server.http.request.durationhistogram recorded by a thin ASGI middleware, reusing the existing Server OTLP pipeline. Direct REST clients and requests that fail before a lifecycle operation now produce Server-side request telemetry (request rate/QPS, status-code error rate, route latency).server.http.request.durationmshttp_method,http_route,http_status_codeBehavior
scope["route"].path), never the raw URL path, falling back tounknownwhen no route matched (e.g. auth short-circuits before routing, or a 404).record_sandbox_create_duration).Implementation
integrations/otel/metrics.py: second histogram instrument (server.http.request.duration, ms, explicit buckets) wired into the existing provider/views, plusrecord_http_request_duration().middleware/http_request_metrics.py:HttpRequestMetricsMiddleware(BaseHTTPMiddleware) that times the request and records in afinallyblock so exceptions still produce a sample.main.py: middleware added outermost in the user stack so auth failures and unmatched routes are also covered.Tests
/things/{thing_id}, not/things/sbx-12345).unknownroute, 404.Verified: full
server/tests/suite passes (1402 passed; the single failure is a Windows-only symlink-privilege test unrelated to this change),ruff checkclean.