feat: add OpenTelemetry Prometheus metrics#34
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds OpenTelemetry-based telemetry and Prometheus metrics collection to Chronicle. It introduces dependencies, a metrics API endpoint, a telemetry initialization module, a Nitro plugin for request/response recording, SSR render timing, and configuration schema support. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Nitro Plugin
participant OTel SDK
participant Prometheus
Client->>Server: HTTP Request
Nitro Plugin->>Nitro Plugin: Record requestStart = now()
Server->>OTel SDK: recordRequest(method, path, status, duration)
OTel SDK->>OTel SDK: Update http_server_request_total counter
OTel SDK->>OTel SDK: Record http_server_request_duration_ms
Server->>Client: HTTP Response
Client->>Server: GET /api/metrics
Server->>Prometheus: getMetricsRequestHandler()
Prometheus-->>Server: Formatted metrics (text/plain)
Server->>Client: Prometheus metrics response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (3)
packages/chronicle/src/server/telemetry.ts (2)
13-16: Module-level metrics are uninitialized untilinitTelemetryis called.These variables are declared but not initialized. While
recordRequestandrecordSSRRenderuse optional chaining to handle this safely,getExporter()will returnundefinedbefore initialization. The metrics endpoint handler already checks for this, so this is acceptable.Consider adding explicit
| undefinedtypes for clarity:let exporter: PrometheusExporter | undefined let requestCounter: Counter | undefined let requestDuration: Histogram | undefined let ssrRenderDuration: Histogram | undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/server/telemetry.ts` around lines 13 - 16, The module-level metrics (exporter, requestCounter, requestDuration, ssrRenderDuration) are declared but not initialized which makes their type effectively possibly undefined; update the declarations for PrometheusExporter, Counter, and Histogram to explicitly include | undefined (i.e. exporter: PrometheusExporter | undefined, requestCounter: Counter | undefined, requestDuration: Histogram | undefined, ssrRenderDuration: Histogram | undefined) so callers like getExporter(), recordRequest(), and recordSSRRender() reflect the potential undefined state and keep type-checking clear.
18-36: Guard against multiple initializations.
initTelemetrycan be called multiple times, which would create duplicateMeterProviderinstances and potentially cause metric conflicts or memory leaks. Consider adding an initialization guard.♻️ Proposed fix with initialization guard
+let initialized = false + export function initTelemetry(config: ChronicleConfig) { + if (initialized) return + initialized = true + const resource = resourceFromAttributes({ [ATTR_SERVICE_NAME]: config.telemetry?.serviceName ?? 'chronicle', })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/server/telemetry.ts` around lines 18 - 36, initTelemetry currently recreates exporter, MeterProvider, and metrics on every call; add an initialization guard at the start of initTelemetry to make it idempotent (e.g., a module-level boolean like telemetryInitialized or check if exporter/provider are already set) and return early if already initialized; ensure you reference and set telemetryInitialized (or reuse existing exporter/provider) and avoid reassigning requestCounter, requestDuration, and ssrRenderDuration when the guard prevents reinitialization so duplicate MeterProvider instances are not created.packages/chronicle/src/server/entry-server.tsx (1)
61-101: Timing captures stream creation, not full render completion.The duration measured here is the time to create the
ReadableStream, not the time to fully render the HTML. The actual rendering continues as the stream is consumed by the client. This is a reasonable metric for initial response latency, but be aware it doesn't reflect total SSR work.If you intend to measure full render time, you'd need to consume the stream before measuring. The current approach is valid for measuring time-to-first-byte latency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chronicle/src/server/entry-server.tsx` around lines 61 - 101, The measured renderDuration uses performance.now() around renderToReadableStream (renderStart, renderDuration) which only captures stream creation (time-to-first-byte) not full SSR completion; if you need full render time, consume the ReadableStream produced by renderToReadableStream (e.g., read the stream to completion or create a Response and await .text()) before computing renderDuration and calling recordSSRRender(pathname, status, renderDuration); otherwise leave as-is for TTFB metrics and add a comment clarifying the current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/chronicle/src/server/api/metrics.ts`:
- Around line 11-18: The Promise that produces metricsString can hang because it
neither rejects on errors nor times out; update the Promise around
exporter.getMetricsRequestHandler to accept (resolve, reject), add a timeout
timer that rejects after a short period and is cleared on resolve/reject, wrap
the call to exporter.getMetricsRequestHandler in try/catch and call reject if it
throws synchronously, and update mockRes.end to resolve only when data is
received (and to reject if data is missing or invalid); ensure the timeout is
cleared in both the resolve and reject paths so metricsString never hangs
indefinitely.
---
Nitpick comments:
In `@packages/chronicle/src/server/entry-server.tsx`:
- Around line 61-101: The measured renderDuration uses performance.now() around
renderToReadableStream (renderStart, renderDuration) which only captures stream
creation (time-to-first-byte) not full SSR completion; if you need full render
time, consume the ReadableStream produced by renderToReadableStream (e.g., read
the stream to completion or create a Response and await .text()) before
computing renderDuration and calling recordSSRRender(pathname, status,
renderDuration); otherwise leave as-is for TTFB metrics and add a comment
clarifying the current behavior.
In `@packages/chronicle/src/server/telemetry.ts`:
- Around line 13-16: The module-level metrics (exporter, requestCounter,
requestDuration, ssrRenderDuration) are declared but not initialized which makes
their type effectively possibly undefined; update the declarations for
PrometheusExporter, Counter, and Histogram to explicitly include | undefined
(i.e. exporter: PrometheusExporter | undefined, requestCounter: Counter |
undefined, requestDuration: Histogram | undefined, ssrRenderDuration: Histogram
| undefined) so callers like getExporter(), recordRequest(), and
recordSSRRender() reflect the potential undefined state and keep type-checking
clear.
- Around line 18-36: initTelemetry currently recreates exporter, MeterProvider,
and metrics on every call; add an initialization guard at the start of
initTelemetry to make it idempotent (e.g., a module-level boolean like
telemetryInitialized or check if exporter/provider are already set) and return
early if already initialized; ensure you reference and set telemetryInitialized
(or reuse existing exporter/provider) and avoid reassigning requestCounter,
requestDuration, and ssrRenderDuration when the guard prevents reinitialization
so duplicate MeterProvider instances are not created.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f3c9910-8b36-4293-b529-5e266447e8e5
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
packages/chronicle/package.jsonpackages/chronicle/src/server/api/metrics.tspackages/chronicle/src/server/entry-server.tsxpackages/chronicle/src/server/plugins/telemetry.tspackages/chronicle/src/server/telemetry.tspackages/chronicle/src/types/config.ts
Comment on lines
+11
to
+18
| const metricsString = await new Promise<string>((resolve) => { | ||
| const mockRes = { | ||
| setHeader: () => mockRes, | ||
| end: (data: string) => resolve(data), | ||
| } as unknown as ServerResponse | ||
|
|
||
| exporter.getMetricsRequestHandler({} as unknown as IncomingMessage, mockRes) | ||
| }) |
There was a problem hiding this comment.
Promise may hang indefinitely if the handler fails.
The promise has no rejection path or timeout. If getMetricsRequestHandler throws an error or fails to call end(), this request will hang forever. Consider adding error handling and a timeout.
🛡️ Proposed fix with timeout and error handling
- const metricsString = await new Promise<string>((resolve) => {
+ const metricsString = await new Promise<string>((resolve, reject) => {
+ const timeout = setTimeout(() => reject(new Error('Metrics collection timed out')), 5000)
const mockRes = {
setHeader: () => mockRes,
- end: (data: string) => resolve(data),
+ end: (data: string) => {
+ clearTimeout(timeout)
+ resolve(data)
+ },
} as unknown as ServerResponse
- exporter.getMetricsRequestHandler({} as unknown as IncomingMessage, mockRes)
+ try {
+ exporter.getMetricsRequestHandler({} as unknown as IncomingMessage, mockRes)
+ } catch (error) {
+ clearTimeout(timeout)
+ reject(error)
+ }
- })
+ }).catch((error) => {
+ throw new Error(`Failed to collect metrics: ${error.message}`)
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const metricsString = await new Promise<string>((resolve) => { | |
| const mockRes = { | |
| setHeader: () => mockRes, | |
| end: (data: string) => resolve(data), | |
| } as unknown as ServerResponse | |
| exporter.getMetricsRequestHandler({} as unknown as IncomingMessage, mockRes) | |
| }) | |
| const metricsString = await new Promise<string>((resolve, reject) => { | |
| const timeout = setTimeout(() => reject(new Error('Metrics collection timed out')), 5000) | |
| const mockRes = { | |
| setHeader: () => mockRes, | |
| end: (data: string) => { | |
| clearTimeout(timeout) | |
| resolve(data) | |
| }, | |
| } as unknown as ServerResponse | |
| try { | |
| exporter.getMetricsRequestHandler({} as unknown as IncomingMessage, mockRes) | |
| } catch (error) { | |
| clearTimeout(timeout) | |
| reject(error) | |
| } | |
| }).catch((error) => { | |
| throw new Error(`Failed to collect metrics: ${error.message}`) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/chronicle/src/server/api/metrics.ts` around lines 11 - 18, The
Promise that produces metricsString can hang because it neither rejects on
errors nor times out; update the Promise around
exporter.getMetricsRequestHandler to accept (resolve, reject), add a timeout
timer that rejects after a short period and is cleared on resolve/reject, wrap
the call to exporter.getMetricsRequestHandler in try/catch and call reject if it
throws synchronously, and update mockRes.end to resolve only when data is
received (and to reject if data is missing or invalid); ensure the timeout is
cleared in both the resolve and reject paths so metricsString never hangs
indefinitely.
rohilsurana
approved these changes
Apr 14, 2026
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
telemetry.enabled,telemetry.serviceName) tochronicle.yaml/api/metricsendpoint/api/metricsfrom self-instrumentationTest plan
telemetry.enabled: trueinchronicle.yamlchronicle devand hit/api/metrics— verify Prometheus format outputhttp_server_request_totalandhttp_server_request_duration_msupdatehttp_server_ssr_render_duration_msrecords for SSR pages/api/metricsroute itself is not recorded in metricstelemetry.enabledis absent🤖 Generated with Claude Code