Skip to content

fix(server): Measure request duration end-to-end - #555

Merged
lcian merged 7 commits into
mainfrom
fix/request-duration-end-to-end
Jul 27, 2026
Merged

fix(server): Measure request duration end-to-end#555
lcian merged 7 commits into
mainfrom
fix/request-duration-end-to-end

Conversation

@lcian

@lcian lcian commented Jul 14, 2026

Copy link
Copy Markdown
Member

server.requests.duration now measures the request through response-body completion rather than stopping when a handler produces headers. This includes time spent streaming GET payloads, batch responses, and multipart-complete responses.

The middleware moves its timing guard into MetricsBody, which emits the metric only after Hyper has completed or dropped the response body. The wrapper preserves response metadata and existing Content-Length behavior.

Status tagging reflects how streaming ended:

  • successful completion: the response status from the headers;
  • client disconnect before completion: 499;
  • server-side stream error: 500.

Completion handling is verified against Hyper 1.10.1 for empty/no-body responses, buffered bodies, ordinary streamed bodies, and terminal trailers. Regression tests cover each completion path, plus stream errors and interrupted streams.

Close FS-555

The server.requests.duration metric previously stopped as soon as the handler
produced the response headers, excluding the time spent streaming the response
body. This under-measured GET, batch, and multipart:complete responses, whose
bodies are streamed after the middleware returns.

Move the timing guard into a MetricsBody wrapper around the response body so it
is dropped only once the body has finished streaming. When the body does not
stream to completion (client disconnect or a server-side stream error, folded
together), the metric is tagged status:499 instead of the header status.
@codecov

This comment has been minimized.

Distinguish a server-side body stream error from a client disconnect: an error
frame while streaming the response body is now tagged status:500, while a client
disconnecting mid-stream remains status:499.
@jan-auer

Copy link
Copy Markdown
Member

The server.requests.duration metric now records the full request lifetime

Changing .duration to cover the full time span makes sense. It can be useful to retain the old TTFB metric in addition to the full duration. We indirectly have those measurements through the server calls, but having a metric that covers all requests is more convenient.

Comment thread objectstore-server/docs/architecture.md Outdated
Comment thread objectstore-server/src/web/middleware.rs Outdated
Comment thread objectstore-server/src/web/middleware.rs Outdated
@lcian
lcian marked this pull request as ready for review July 15, 2026 12:00
@lcian
lcian requested a review from a team as a code owner July 15, 2026 12:00
Comment thread objectstore-server/src/web/metrics_body.rs
@lcian
lcian marked this pull request as draft July 15, 2026 17:50
Comment thread objectstore-server/src/web/metrics_body.rs
Classify empty, buffered, streamed, and trailers-only responses as completed so request duration metrics retain the response status. Keep stream errors and interrupted bodies distinguishable, and simplify body wrapping with Response::map.

Co-Authored-By: Claude <noreply@anthropic.com>
@linear-code

This comment has been minimized.

@lcian

This comment has been minimized.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit c1d2a36. Configure here.

Keep response-body outcome coverage next to the MetricsBody state machine while retaining middleware timing coverage with its middleware tests.

Co-Authored-By: Claude <noreply@anthropic.com>
@lcian
lcian marked this pull request as ready for review July 16, 2026 13:35
@lcian
lcian requested a review from jan-auer July 16, 2026 13:37
Comment thread objectstore-server/src/web/metrics_body.rs Outdated
Comment thread objectstore-server/src/web/metrics_body.rs Outdated
Comment thread objectstore-server/src/web/metrics_body.rs Outdated
Comment thread objectstore-server/src/web/metrics_body.rs Outdated
Comment thread objectstore-server/src/web/metrics_body.rs Outdated
@lcian
lcian force-pushed the fix/request-duration-end-to-end branch from 7603c6f to 88bf634 Compare July 24, 2026 12:23
@lcian
lcian requested a review from jan-auer July 24, 2026 12:28
@lcian
lcian force-pushed the fix/request-duration-end-to-end branch from ccabebb to 88bf634 Compare July 27, 2026 09:24
@lcian
lcian enabled auto-merge (squash) July 27, 2026 09:24
@lcian
lcian merged commit 181df0a into main Jul 27, 2026
45 of 46 checks passed
@lcian
lcian deleted the fix/request-duration-end-to-end branch July 27, 2026 09:28
lcian pushed a commit that referenced this pull request Jul 28, 2026
Streaming responses that declare a `content-length` were recorded as
client disconnects (499) in `server.requests.duration`, a regression
from #555. Hyper stops polling a response body once the declared number
of bytes has been written, so the wrapper never saw an end-of-stream
poll and its guard fell back to 499 when dropped.

`MetricsBody` now counts the bytes it forwards and completes the request
once the declared length is reached, alongside the existing
end-of-stream and trailers cases. Responses whose body hyper never polls
at all are completed up front when they are wrapped.

The tests were reworked around a helper that serves a real axum handler
through the middleware and returns the tracked status and duration,
which made room for a paused-clock test asserting the duration spans
body streaming rather than ending at the response headers. Capturing
metrics across await points needs a new
`with_capturing_test_client_async` in `objectstore-metrics`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants