Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
Adds OpenTelemetry distributed tracing support to the client package by creating spans from existing TracingChannel events, including optional connection-related spans and command filtering.
Changes:
- Introduces
OTelTracingto subscribe toTracingChannellifecycle events and emit OTel spans with Redis/db semantic attributes. - Extends observability configuration/types to include
tracingsettings and addsdb.operation.batch.sizeattribute key. - Adds unit tests covering span creation, parent-child relationships, errors, filtering, and lifecycle.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/client/package.json | Adds OTel tracing-related devDependencies used by new tracing tests. |
| packages/client/lib/opentelemetry/types.ts | Adds TracingConfig and new batch-size attribute constant; wires tracing into ObservabilityConfig. |
| packages/client/lib/opentelemetry/tracing.ts | Implements OTelTracing span creation and context propagation via bindStore. |
| packages/client/lib/opentelemetry/tracing.spec.ts | Adds unit tests for span behavior (commands, batch, connection spans, errors, lifecycle). |
| packages/client/lib/opentelemetry/index.ts | Initializes tracing when enabled and re-exports tracing config/entrypoints. |
| package-lock.json | Updates lockfile for newly added devDependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "@node-rs/xxhash": "1.7.6", | ||
| "@opentelemetry/api": "^1.9.0", | ||
| "@opentelemetry/context-async-hooks": "^2.6.1", | ||
| "@opentelemetry/sdk-metrics": "^2.2.0", | ||
| "@opentelemetry/sdk-trace-base": "^2.6.1", |
There was a problem hiding this comment.
The new devDependencies @opentelemetry/context-async-hooks and @opentelemetry/sdk-trace-base require Node ^18.19.0 || >=20.6.0 (per their package engines), but this package advertises engines.node: >= 18. This can produce engine warnings/errors for contributors/CI pinned to Node 18.0–18.18; consider either bumping the supported Node engine range (e.g. to >=18.19) or using OTel devDependency versions compatible with Node 18.0.
| api: typeof import('@opentelemetry/api'); | ||
| config?: TracingConfig; | ||
| }) { | ||
| if (OTelTracing.#initialized) return; |
There was a problem hiding this comment.
OTelTracing.init ignores config.enabled and silently no-ops when called multiple times, which is inconsistent with OpenTelemetry.init / OTelMetrics.init (they throw on re-init) and makes the enabled flag in TracingConfig misleading. Consider either (a) honoring config.enabled === false inside OTelTracing.init and throwing on double-init for consistency, or (b) removing enabled from TracingConfig and keeping the current behavior intentionally.
| if (OTelTracing.#initialized) return; | |
| if (OTelTracing.#initialized) { | |
| throw new Error('OTelTracing has already been initialized'); | |
| } | |
| // If tracing is explicitly disabled via config, mark as initialized | |
| // but do not create an instance or subscribe to any channels. | |
| if (config?.enabled === false) { | |
| OTelTracing.#initialized = true; | |
| return; | |
| } |
960fab5 to
f870361
Compare
Adds built-in OpenTelemetry distributed tracing (spans) alongside the existing metrics implementation. Spans are created by subscribing to the same TracingChannels that metrics use, keeping the core code unchanged. Context propagation uses the bindStore hack on OTel's internal AsyncLocalStorage so that parent-child span relationships work automatically (e.g. MULTI > individual commands). Span coverage: - Command spans (SET, GET, etc.) with db.query.text, db.namespace - Batch spans (MULTI/PIPELINE) with db.operation.batch.size - Connection + connection wait spans (opt-in via config) - Error attributes: error.type, db.response.status_code All attributes follow OTel semantic conventions for database spans. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
01f56ea to
57a67c5
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
TracingChanneleventsbindStoreon OTel's internalAsyncLocalStorage— enables automatic parent-child span relationships (e.g. MULTI → child command spans)db.operation.name,db.query.text,db.namespace,error.type,db.response.status_code,db.operation.batch.size)Spans emitted
node-redis:command{COMMAND}(e.g.SET)CLIENTnode-redis:batchMULTI/PIPELINECLIENTnode-redis:connectredis connectCLIENTenableConnectionSpansnode-redis:connection:waitredis connection:waitINTERNALenableConnectionSpansConfig
Context propagation
Uses the same
bindStoreworkaround fromotel-tracing-channel— accesses OTel's internalAsyncLocalStorageviacontext._getContextManager()._asyncLocalStorageand binds it to each TracingChannel'sstartsub-channel. Falls back to WeakMap-based span tracking when ALS is not accessible.