feat: add http.server.request.duration histogram metric#76
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughOpenTelemetry histogram Changes
Sequence DiagramsequenceDiagram
participant Client
participant Plugin as Elysia Plugin
participant Span as Root Span
participant Meter as Histogram Meter
participant Reader as MetricReader
Client->>Plugin: HTTP request
Plugin->>Plugin: capture startTime (performance.now())
Plugin->>Span: create root span
Span->>Span: handle request (route handler)
alt completes (success or error)
Span->>Plugin: invoke completion handlers (onAfterResponse / onError)
else aborted
Client-->>Plugin: abort signal
Plugin->>Plugin: trigger abort handler
end
Plugin->>Plugin: if not recorded -> compute duration (s) & set attrs (method, route, status, scheme, error.type?)
Plugin->>Meter: record histogram `http.server.request.duration`
Meter->>Reader: export datapoint
Reader->>Reader: collect/store datapoint
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
947-959:⚠️ Potential issue | 🟠 MajorRemove the abort event listener when the request completes~
That abort handler closes over
rootSpan,attributes,recordDuration, and the request context without ever removing itself. Even though the listener checks ifrootSpan.endedand bails early, you're still holding onto those closure references needlessly. If a signal gets reused or hangs around (especially in non-standard scenarios), you'll leak one stale listener per request, which is sloppy~♡Attach a corresponding
removeEventListenercall in yourevent.onStophandler or wherever the span cleanup happens (onAfterResponse,onError). Don't be lazy!🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 947 - 959, The abort listener added via context.request.signal.addEventListener currently closes over rootSpan, recordDuration and request context and is never removed; change the code to assign the listener to a const (e.g. const onAbort = () => { ... }) and register that instead, then call context.request.signal.removeEventListener('abort', onAbort) from your span cleanup path (event.onStop or onAfterResponse/onError where you already end rootSpan and call recordDuration) to release the closure and avoid leaking per-request listeners; ensure you still perform the same status/setDuration/end logic inside the onAbort handler before removing it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.ts`:
- Around line 497-515: The recordDuration function currently omits
server.address and server.port; modify the httpServerDuration.record call so it
conditionally includes 'server.address' and 'server.port' from the attributes
object only when they are already defined (e.g., attributes['server.address'] !=
null and attributes['server.port'] != null) to avoid recording undefined values
in early-abort cases; locate this change around the recordDuration function and
the httpServerDuration.record invocation where other attributes are spread and
add conditional inclusion for those two keys.
In `@test/metrics.test.ts`:
- Around line 38-46: Your test hooks only disable metrics but leave the global
tracer provider (started by opentelemetry() as a NodeSDK) around; update
beforeEach/afterEach to also reset/shutdown tracing: in beforeEach, ensure any
existing global tracer provider is cleared or replaced (match the pattern in
test/core.test.ts), and in afterEach await opentelemetry().shutdown() (or
shutdown the NodeSDK returned by opentelemetry()) in addition to awaiting
meterProvider.shutdown() and metrics.disable() so tracing state is cleaned
between tests.
---
Outside diff comments:
In `@src/index.ts`:
- Around line 947-959: The abort listener added via
context.request.signal.addEventListener currently closes over rootSpan,
recordDuration and request context and is never removed; change the code to
assign the listener to a const (e.g. const onAbort = () => { ... }) and register
that instead, then call context.request.signal.removeEventListener('abort',
onAbort) from your span cleanup path (event.onStop or onAfterResponse/onError
where you already end rootSpan and call recordDuration) to release the closure
and avoid leaking per-request listeners; ensure you still perform the same
status/setDuration/end logic inside the onAbort handler before removing it.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 461c9c6c-91dd-4a6b-bf70-7158fb44565c
📒 Files selected for processing (2)
src/index.tstest/metrics.test.ts
| const recordDuration = () => { | ||
| if (durationRecorded) return | ||
| durationRecorded = true | ||
|
|
||
| const durationS = | ||
| (performance.now() - requestStartTime) / 1000 | ||
| const statusCode = | ||
| attributes['http.response.status_code'] | ||
|
|
||
| httpServerDuration.record(durationS, { | ||
| 'http.request.method': attributes['http.request.method'] ?? method, | ||
| 'url.scheme': attributes['url.scheme'], | ||
| 'http.response.status_code': statusCode, | ||
| 'http.route': attributes['http.route'], | ||
| ...(typeof statusCode === 'number' && | ||
| statusCode >= 500 && { | ||
| 'error.type': String(statusCode) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the recordDuration() implementation around lines 497-515
sed -n '490,520p' src/index.tsRepository: elysiajs/opentelemetry
Length of output: 1035
🏁 Script executed:
# Now check where server.address and server.port are populated (around lines 647-650)
sed -n '640,660p' src/index.tsRepository: elysiajs/opentelemetry
Length of output: 602
🏁 Script executed:
# Get a broader view of the recordDuration function and when it's called
sed -n '480,550p' src/index.tsRepository: elysiajs/opentelemetry
Length of output: 1896
🏁 Script executed:
# Check the overall structure to understand the flow - when is recordDuration called vs when are attributes set
rg 'recordDuration|server\.address|server\.port' -B 2 -A 2 src/index.ts | head -100Repository: elysiajs/opentelemetry
Length of output: 701
🏁 Script executed:
# Get line numbers for context to see the execution flow
sed -n '470,660p' src/index.ts | cat -n | head -200Repository: elysiajs/opentelemetry
Length of output: 6159
🏁 Script executed:
# Find where the abort listener is installed and when recordDuration is called
rg -n 'addEventListener|recordDuration|server\.port|server\.address' src/index.ts | head -30Repository: elysiajs/opentelemetry
Length of output: 429
🏁 Script executed:
# Check the sequence: when are server attrs set vs when is abort listener attached
sed -n '490,700p' src/index.ts | cat -nRepository: elysiajs/opentelemetry
Length of output: 6899
🏁 Script executed:
# Let me verify the execution order of callbacks to confirm the timing issue
sed -n '947,960p' src/index.tsRepository: elysiajs/opentelemetry
Length of output: 422
🏁 Script executed:
# Check the semantic conventions for http.server.request.duration
web_searchRepository: elysiajs/opentelemetry
Length of output: 116
🏁 Script executed:
# Verify if server.address and server.port would be available by the time recordDuration is called
rg -n 'onTransform|onError|onAfterHandle|addEventListener' src/index.ts | head -20Repository: elysiajs/opentelemetry
Length of output: 336
🌐 Web query:
OpenTelemetry HTTP server request duration metric semantic conventions recommended attributes
💡 Result:
The OpenTelemetry HTTP server request duration metric is http.server.request.duration, a required Histogram instrument with unit s measuring the duration of HTTP server requests. It should use explicit bucket boundaries [0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10] seconds. When reported alongside an HTTP server span, the metric value should match the span duration. Recommended attributes (from official semantic conventions): Required: - http.request.method (string): HTTP request method (e.g., GET, POST, HEAD). Must be a known method or _OTHER. Conditionally Required: - url.scheme (string): URI scheme (e.g., http, https). - http.response.status_code (int): HTTP response status code, if received/sent. - error.type (string): Error class, if the request ended in error. Additional recommended attributes from conventions (often used with this metric): - server.address (string): Local server name/IP that received the request. - server.port (int): Local server port. - http.route (string): Matched route/path template (if supported by framework). - network.protocol.name (string) and network.protocol.version (string): e.g., http, 1.1. These are defined in the HTTP metrics semantic conventions (stable as of latest docs).
Citations:
- 1: https://opentelemetry.io/docs/specs/semconv/http/http-metrics/
- 2: https://hexdocs.pm/opentelemetry_semantic_conventions/http-metrics.html
🏁 Script executed:
# Let me understand the lifecycle better - when does onTransform execute relative to abort listener registration
sed -n '520,950p' src/index.ts | cat -n | grep -A 5 -B 5 'onTransform\|addEventListener'Repository: elysiajs/opentelemetry
Length of output: 868
🏁 Script executed:
# Check line 940 to see the other recordDuration call
sed -n '935,945p' src/index.tsRepository: elysiajs/opentelemetry
Length of output: 201
🏁 Script executed:
# Let me trace the actual execution flow more clearly - checking where each lifecycle hook/listener is registered
sed -n '520,950p' src/index.ts | cat -n | head -100Repository: elysiajs/opentelemetry
Length of output: 2926
🏁 Script executed:
# Verify the exact positions in the file
git ls-files src/index.ts
wc -l src/index.tsRepository: elysiajs/opentelemetry
Length of output: 95
🏁 Script executed:
# Let me trace the exact sequence to understand when server attributes would be available
# Check what's between the second onTransform registration and the abort listener registration
sed -n '607,947p' src/index.ts | tail -50Repository: elysiajs/opentelemetry
Length of output: 1247
🏁 Script executed:
# Also check if there's any early exit/completion before the second onTransform
# Look for the actual handler structure
sed -n '470,550p' src/index.tsRepository: elysiajs/opentelemetry
Length of output: 2195
🏁 Script executed:
# Let me verify the execution order more carefully by looking at the complete lifecycle
# Check if there are any conditions/early returns that would affect when server attributes are set
sed -n '607,700p' src/index.ts | cat -nRepository: elysiajs/opentelemetry
Length of output: 3335
Include server attrs when available, but handle the early-abort edge case~ ✧(๑•́ ᗜ •̀๑) ✧
The metric recording is skipping server.address and server.port even though these are in the attributes object and recommended by OpenTelemetry semantic conventions. However, be careful: if the request aborts very early (before the second onTransform executes), these attributes won't be populated yet. The patch shown adds them unconditionally, which could record undefined values in early abort scenarios.
Better approach: either (1) add them unconditionally with a fallback, or (2) conditionally include them only if already available. The simple addition shown is a good start though~ ♡
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 497 - 515, The recordDuration function currently
omits server.address and server.port; modify the httpServerDuration.record call
so it conditionally includes 'server.address' and 'server.port' from the
attributes object only when they are already defined (e.g.,
attributes['server.address'] != null and attributes['server.port'] != null) to
avoid recording undefined values in early-abort cases; locate this change around
the recordDuration function and the httpServerDuration.record invocation where
other attributes are spread and add conditional inclusion for those two keys.
There was a problem hiding this comment.
Intentionally omitted. The OTel spec marks server.address and server.port as Opt-In and explicitly warns they can be exploited to trigger cardinality limits since they're derived from headers. For a general-purpose plugin, the safer default is to exclude them. Users who need them can add them via a View on their MeterProvider.
There was a problem hiding this comment.
@turisanapo Ahh, that makes total sense~ ♡ You're right, I missed that distinction in the spec — marking them as Opt-In with an explicit cardinality warning is exactly the kind of thing that makes a default-safe general-purpose plugin! (๑•̀ᗜ•́)و ✧ Excluding them by default and letting users opt in via a View is the correct call here. I'll remember this for future reviews~
✏️ Learnings added
Learnt from: turisanapo
Repo: elysiajs/opentelemetry PR: 76
File: src/index.ts:0-0
Timestamp: 2026-04-14T10:47:55.631Z
Learning: In the `elysiajs/opentelemetry` plugin (`src/index.ts`), `server.address` and `server.port` are intentionally excluded from the `http.server.request.duration` histogram metric attributes. The OpenTelemetry HTTP metrics semantic conventions mark these as **Opt-In** and warn they can be exploited to trigger cardinality explosions since they are derived from request headers. The safe default for a general-purpose plugin is to omit them; users who need them can add them via a `View` on their `MeterProvider`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
turisanapo
force-pushed
the
feat/http-server-request-duration-metric
branch
4 times, most recently
from
0f9763f to
cdf5a2f
Compare
Record the standard OTel HTTP server duration metric alongside existing traces, per the HTTP metrics semantic conventions. The histogram is emitted at the same three points where the root span ends (onAfterResponse, onError, abort) with the required and conditionally required attributes: http.request.method, url.scheme, http.response.status_code, http.route, and error.type. No new runtime dependencies — uses the metrics API already included in @opentelemetry/api. When no MeterProvider is configured the meter is a no-op with zero overhead. Closes elysiajs#75 Made-with: Cursor
turisanapo
force-pushed
the
feat/http-server-request-duration-metric
branch
from
cdf5a2f to
3a0c958
Compare
2 participants
Summary
http.server.request.durationhistogram metric per the HTTP metrics semantic conventions@opentelemetry/api; when noMeterProvideris configured the meter is a no-op with zero overheadMetric attributes
http.request.methodrequest.methodurl.schemehttp.response.status_codehttp.routeerror.typeHistogram buckets
Uses the spec-recommended
ExplicitBucketBoundaries:[0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10]Closes #75
Test plan
error.typeattributecheckIfShouldTrace: () => falsesuppresses metricsSummary by CodeRabbit
New Features
Tests