Skip to content

Add request latency histogram metric - #872

Open
mikmatko wants to merge 1 commit into
emmett-framework:masterfrom
mikmatko:latency_metrics
Open

Add request latency histogram metric#872
mikmatko wants to merge 1 commit into
emmett-framework:masterfrom
mikmatko:latency_metrics

Conversation

@mikmatko

@mikmatko mikmatko commented Jul 2, 2026

Copy link
Copy Markdown

Solves #871

Note: AI implemented. I'm not familiar with Rust. I've reviewed the code. I apologize if this is seen as too much AI slop :)

Expose granian_request_duration_seconds, a per-worker Prometheus histogram of application request durations. Static-file requests are excluded.

Adds a LatencyHistogram to WorkerMetrics, a MetricValue::Hist variant for IPC transport, histogram exposition in the aggregator, and integration tests covering exposition, counts, static exclusion and the empty case.

Co-authored-by: GitHub Copilot [email protected]

@gi0baro gi0baro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few things:

  • the implementation is not measuring the actual e2e latency, as on responses wrapping an async stream the time to send the actual bytes is outside that future block
  • not sure why the naming for the is req/request instead of response
  • I generally don't allow AI generated code to be merged in Granian, especially if it's added as-is, without the proper refinements (eg: the giant-wall-of-comments is just useless and awful).

Expose granian_request_duration_seconds, a per-worker Prometheus histogram
of request durations, using OpenTelemetry-aligned buckets. Static-file
requests are excluded so the metric reflects application handling only.

Duration is measured end-to-end: a TimedBody response-body wrapper records
the elapsed time once the full response body has been sent (or on drop),
so streaming responses are measured to the last byte rather than to
time-to-first-byte.

Adds a DurationHistogram to WorkerMetrics, a MetricValue::Hist variant for
IPC transport, histogram exposition in the aggregator, and integration
tests covering exposition, counts, static exclusion, streaming end-to-end
timing and the empty case.

Co-authored-by: GitHub Copilot <[email protected]>
@mikmatko
mikmatko requested a review from gi0baro July 20, 2026 15:36
@mikmatko

mikmatko commented Jul 20, 2026

Copy link
Copy Markdown
Author

Thank you for the comments. I've pushed changes.

  • the implementation is not measuring the actual e2e latency, as on responses wrapping an async stream the time to send the actual bytes is outside that future block

I believe streaming responses are now correctly tracked.

  • not sure why the naming for the is req/request instead of response

OTEL convention, seemingly a "standard" thing for Prometheus metrics.

  • I generally don't allow AI generated code to be merged in Granian, especially if it's added as-is, without the proper refinements (eg: the giant-wall-of-comments is just useless and awful).

That's fair :) I've reduced the verbosity of the comments. Personally I think the overly verbose comments are a good thing - it helps a lot when jumping to an unknown codebase. A lot of people read and comprehend natural language faster than code. But yes, it's verbose, maybe even awful, but not useless :)

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