feat(metrics): add OTLP metrics support via getMeter - #13
Conversation
Adds a metrics signal to `setupOtel()` using the OpenTelemetry SDK metrics package and OTLP/HTTP exporter. The returned `OtelHandle` now exposes `getMeter(name, version?)` and `meterProvider` so callers can create instruments without touching the global API. Key changes: - New `src/otlp-config.ts` centralises endpoint, header, and periodic reader timing resolution for all three signals, replacing ad-hoc logic scattered across setup code. - `setupOtel()` registers a `M
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Disabled knowledge base sources:
📝 WalkthroughWalkthrough
ChangesMetrics support
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/otlp-config.ts (1)
32-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd JSDoc for the exported configuration helpers.
As per coding guidelines, “Include JSDoc comments for exported functions and classes.”
Also applies to: 38-51, 53-63, 76-91
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/otlp-config.ts` around lines 32 - 36, Add JSDoc comments to each exported configuration helper in this module, including parseEnvHeaders and the helpers at the referenced ranges. Document each function’s purpose, parameters, and return value accurately without changing its implementation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/index.mdx`:
- Line 46: Update the setupOtel() documentation to state that it installs global
OpenTelemetry providers by default, and explicitly mention that register: false
keeps the tracer, logger, and meter providers scoped. Preserve the existing list
of runtime components.
In `@src/otlp-config.ts`:
- Around line 47-50: Update the generic endpoint selection in the OTLP
configuration flow to use a falsy fallback so an empty
OTEL_EXPORTER_OTLP_ENDPOINT uses base; add a regression test covering the
empty-string environment value and expected endpoint behavior.
---
Nitpick comments:
In `@src/otlp-config.ts`:
- Around line 32-36: Add JSDoc comments to each exported configuration helper in
this module, including parseEnvHeaders and the helpers at the referenced ranges.
Document each function’s purpose, parameters, and return value accurately
without changing its implementation.
🪄 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: b29534c0-2ccf-4c7b-866d-deeac32157d2
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/workflows/integration.ymlREADME.mddocs/concepts/architecture.mdxdocs/configuration.mdxdocs/guides/metrics.mdxdocs/guides/setup.mdxdocs/index.mdxdocs/quickstart.mdxdocs/reference/api.mdxdocs/reference/environment.mdxpackage.jsonsrc/otlp-config.tssrc/setup.tstests/integration/README.mdtests/integration/collector-config.yamltests/integration/otel-collector.test.tstests/otlp-config.test.tstests/setup.scoped.test.tstests/setup.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Cursor Bugbot
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Prefer
interfacefor defining object shapes in TypeScript rather thantypealiases
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Useas constconst assertions for immutable values and literal types
Leverage TypeScript type narrowing instead of type assertions
Use meaningful variable names instead of magic numbers; extract descriptive constants
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, and never usevar
Alwaysawaitpromises in async functions and use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code withtry-catchblocks
Don't use async functions as Promise executors
Files:
tests/setup.scoped.test.tstests/otlp-config.test.tssrc/otlp-config.tstests/setup.test.tstests/integration/otel-collector.test.tssrc/setup.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
**/*.{js,jsx,ts,tsx}: Use camelCase for variable and function names in JavaScript/TypeScript
Use PascalCase for class and component names in JavaScript/TypeScript
Always use async/await for promise handling instead of .then() chains
Include JSDoc comments for exported functions and classes
Use meaningful variable names that clearly describe their purpose
Avoid deeply nested conditionals; use early returns or guard clauses instead
Use const by default, let when reassignment is needed, avoid var
**/*.{js,jsx,ts,tsx}: Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully; don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Keep functions focused and under reasonable cognitive complexity limits
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Group related code together and separate concerns
Addrel="noopener"when usingtarget="_blank"on links
AvoiddangerouslySetInnerHTMLunless absolutely necessary
Don't useeval()or assign directly todocument.cookie
Validate and sanitize user input
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Avoid barrel files (indexfiles that re-export everything)
Use proper image components (for example, Next.js<Image>) over<img>tags
Usenext/heador the App Router metadata API for head elements
Use Server Components for async data fetching instead of async Client Components
Files:
tests/setup.scoped.test.tstests/otlp-config.test.tssrc/otlp-config.tstests/setup.test.tstests/integration/otel-collector.test.tssrc/setup.ts
**/*.test.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Write unit tests for all public functions and components
Files:
tests/setup.scoped.test.tstests/otlp-config.test.tstests/setup.test.tstests/integration/otel-collector.test.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,jsx,ts,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests; use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat; avoid excessivedescribenesting
Files:
tests/setup.scoped.test.tstests/otlp-config.test.tstests/setup.test.tstests/integration/otel-collector.test.ts
🪛 ast-grep (0.44.1)
tests/integration/otel-collector.test.ts
[warning] 38-38: Do not use Math.random() to generate security-sensitive values such as tokens, secrets, passwords, API keys, salts, nonces, OTPs, or session IDs. Math.random() is not cryptographically secure and is predictable. Use crypto.randomBytes()/crypto.randomUUID() (Node) or crypto.getRandomValues() (Web Crypto) instead.
Context: Math.random()
Note: [CWE-330] Use of Insufficiently Random Values.
(insecure-random-security-token-typescript)
🪛 LanguageTool
docs/index.mdx
[style] ~99-~99: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...s) when wrapping business operations. - Use Metrics when creatin...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/guides/setup.mdx
[style] ~47-~47: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...dProcessor` with an OTLP log exporter. When a metrics endpoint is resolved, setup c...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~182-~182: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: .... 3. Shuts down the logger provider. 4. Shuts down the meter provider, performing a f...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
README.md
[grammar] ~273-~273: Use a hyphen to join words.
Context: ...pans nest across the boundary). - Auto fetch instrumentation defaults off (wr...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (13)
tests/integration/collector-config.yaml (1)
23-24: LGTM!Also applies to: 38-40
tests/integration/otel-collector.test.ts (1)
35-44: LGTM!Also applies to: 95-113, 133-139, 243-273, 304-304, 320-325, 383-385, 440-450
tests/integration/README.md (1)
5-8: LGTM!Also applies to: 31-31, 47-51
.github/workflows/integration.yml (1)
76-77: LGTM!README.md (1)
7-8: LGTM!Also applies to: 29-96, 156-163, 250-250, 261-278
docs/concepts/architecture.mdx (1)
8-14: LGTM!Also applies to: 27-27, 41-58, 76-76, 91-91
docs/index.mdx (1)
10-11: LGTM!Also applies to: 23-23, 36-40, 42-45, 47-50, 99-99
docs/quickstart.mdx (1)
3-6: LGTM!Also applies to: 31-31, 43-43, 53-78, 100-100, 121-121, 136-188
docs/guides/metrics.mdx (1)
1-203: LGTM!docs/guides/setup.mdx (1)
3-3: LGTM!Also applies to: 19-57, 78-102, 177-184
docs/reference/api.mdx (1)
27-27: LGTM!Also applies to: 44-44, 53-53, 62-87
docs/configuration.mdx (1)
69-83: LGTM!Also applies to: 106-115, 240-250
docs/reference/environment.mdx (1)
17-21: LGTM!Also applies to: 37-37, 65-74, 88-108, 179-184
| 2. `withSpan()` uses the global tracer provider to create application spans. | ||
| 3. `createLogger()` uses the global logger provider and active context so logs inside spans carry trace correlation. | ||
| 4. Fetch auto-instrumentation (native undici on Node when its optional packages are installed, otherwise the `globalThis.fetch` wrap that Bun always uses) makes outbound `fetch` carry W3C trace context to downstream services. | ||
| 1. `setupOtel()` installs the global OpenTelemetry runtime pieces: context manager, propagator, tracer provider, logger provider, and meter provider. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Qualify global provider registration.
Line 46 implies setupOtel() always installs global providers, but register: false deliberately keeps tracer/logger/meter providers scoped. Say “by default” and mention scoped mode.
Proposed fix
-1. `setupOtel()` installs the global OpenTelemetry runtime pieces: context manager, propagator, tracer provider, logger provider, and meter provider.
+1. By default, `setupOtel()` installs the global OpenTelemetry runtime pieces: context manager, propagator, tracer provider, logger provider, and meter provider. With `register: false`, the signal providers remain scoped to the returned handle.📝 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.
| 1. `setupOtel()` installs the global OpenTelemetry runtime pieces: context manager, propagator, tracer provider, logger provider, and meter provider. | |
| 1. By default, `setupOtel()` installs the global OpenTelemetry runtime pieces: context manager, propagator, tracer provider, logger provider, and meter provider. With `register: false`, the signal providers remain scoped to the returned handle. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/index.mdx` at line 46, Update the setupOtel() documentation to state
that it installs global OpenTelemetry providers by default, and explicitly
mention that register: false keeps the tracer, logger, and meter providers
scoped. Preserve the existing list of runtime components.
`??` treats an empty string as a valid value, causing an empty endpoint to override the code-supplied base URL. Switch to `||` so an empty `OTEL_EXPORTER_OTLP_ENDPOINT` correctly falls back.
Summary
setupOtel()builds aMeterProvideralongside the tracer and logger providers, wired to aPeriodicExportingMetricReader+OTLPMetricExporterwhen a metrics endpoint resolves. Metrics share the same resource (service.name/service.version/deployment.environment/ customresourceAttributes), the same no-endpoint local-dev behavior, and the same idempotency and shutdown guarantees as traces and logs.otel.getMeter(name, version?, options?)on the handle. The recommended way to create instruments: it always resolves through this setup's meter provider, so the same application code works in default global mode and in scoped (register: false) mode. There is intentionally no top-levelgetMeter()export — requiring the handle keeps initialization order explicit and avoids binding instruments to OpenTelemetry's no-op provider during module init.OtelHandlenow exposes the providers.getMeter(),meterProvider,tracerProvider, andloggerProviderall live on the handle;shutdown()now also drains the meter provider (final collection + export) through the existingPromise.allSettledpath.src/otlp-config.tsthat handles all three signals uniformly. AddsOTEL_EXPORTER_OTLP_METRICS_ENDPOINTand per-signalOTEL_EXPORTER_OTLP_{TRACES,LOGS,METRICS}_HEADERS(signal headers override generic env headers, which override code headers). Header parsing now uses OTel'sparseKeyPairsIntoRecord, which percent-decodes keys/values.OTEL_METRIC_EXPORT_INTERVAL/OTEL_METRIC_EXPORT_TIMEOUT(milliseconds; defaults60000/30000). Invalid or non-positive values fall back to the defaults, and the timeout is capped at the interval.docs/guides/metrics.mdxdocumenting the setup-handle path, reusable-moduleMeterinjection, shared-publisher usage, attribute/cardinality guidance, and shutdown.Usage
Reusable modules should accept a
Meterrather than reach for the global provider — the application entry point owns setup and injects the meter:Changes
src/otlp-config.tsresolveOtlpEndpoint/resolveOtlpHeaders/parseEnvHeaders/resolveMetricReaderTiming— signal-generic endpoint + header resolution for traces/logs/metrics, percent-decoding viaparseKeyPairsIntoRecord, and metric reader timing with defaults, validation, and timeout-capped-at-interval.src/setup.tsMeterProviderwith aPeriodicExportingMetricReader+OTLPMetricExporter; register it globally unlessregister: false; addgetMeter()/meterProvidertoOtelHandle; shut the meter provider down alongside traces/logs. Replace the inline endpoint/header helpers withotlp-config, add per-signal headers, and include the metrics endpoint in the fetch self-exclusion set.package.json,bun.lock@opentelemetry/exporter-metrics-otlp-http+@opentelemetry/sdk-metrics; add themetricskeyword; refresh the package description.tests/otlp-config.test.tstests/setup.test.tsgetMeterdelegates to this setup's provider; theno-endpointcase now asserts a realSdkMeterProviderand a usable counter; reset the global meter registry between tests.tests/setup.scoped.test.tsregister: falseleaves the global meter provider untouched (andregister: trueregisters it), and thatgetMeter()resolves through the handle's own provider.tests/integration/*,.github/workflows/integration.yml,collector-config.yamlfile/metrics); the e2e test emits a counter and asserts name / value / monotonicity / attributes / shared resource off the collector's real output.README.md,docs/*docs/guides/metrics.mdx.Test plan
bun run test— 95 unit tests pass (9 new offline)bun run build— tsdown ESM (28.32 kB) + DTS (12.24 kB) build succeedsbun x ultracite check src tests— clean (22 files)bun run test:integration— real OTLP/HTTP round-trip including a counter assertion; requires Docker, runs in CI🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches core bootstrap (global/scoped providers, shutdown, OTLP config) and adds a new export pipeline; behavior is well-tested but mis-ordered instrument creation or env misconfiguration could still affect production telemetry.
Overview
Adds OTLP metrics as a third signal alongside traces and logs.
setupOtel()now builds aMeterProviderwith a periodic OTLP exporter when a metrics endpoint resolves, registers it globally by default (or keeps it private in scoped mode), andshutdown()drains metrics with the other providers.The
OtelHandlegainsgetMeter(name)andmeterProviderso apps create instruments from the setup-bound provider without relying on early globalmetrics.getMeter()calls.src/otlp-config.tscentralizes endpoint/header resolution for traces, logs, and metrics (includingOTEL_EXPORTER_OTLP_METRICS_*, per-signal headers, andOTEL_METRIC_EXPORT_INTERVAL/TIMEOUT). Fetch self-exclusion now includes the metrics exporter URL.Dependencies, docs (including
docs/guides/metrics.mdx), unit tests, and the Docker collector integration test are extended to assert metric export end-to-end.Reviewed by Cursor Bugbot for commit 983d0d3. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
otel.getMeter()plus meter-provider support in both global and scoped modes.