Skip to content

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

Open
lcian wants to merge 5 commits into
mainfrom
fix/request-duration-end-to-end
Open

fix(server): Measure request duration end-to-end#555
lcian wants to merge 5 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.

lcian and others added 2 commits July 16, 2026 15:01
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
//! it when the inner body reaches end-of-stream (or is dropped on client
//! disconnect), at which point the guard emits `server.requests.duration`.
//!
//! When the body finishes, [`MetricsBody`] calls

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is kinda verbose and pretty detailed, but I think it's okay?

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