Conversation
Add diagnostics_channel TracingChannel instrumentation for commands and connections. Wraps sendCommand, connect, pipeline, and MULTI with traceCommand/traceConnect alongside existing OTel metrics. - Sanitize args before emission using OTel redis-common rules - Include clientId from identity system in trace context - Add .catch(noop) for pipeline/MULTI to prevent unhandled rejections - Gate on hasSubscribers for zero-cost when no APM subscribes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cover all serialization subsets (args=0, 1, 2, -1, default), edge cases (empty args, AUTH, HELLO, unknown commands, prefix matching like SETEX), case insensitivity, and non-string arg stringification. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Matches the SQL parameterization convention used by db.query.text across APM tools (Sentry, Datadog, OTel). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace inline createRecordOperationDuration/createRecordBatchOperationDuration closures in sendCommand, _executePipeline, and _executeMulti with TracingChannel subscriptions in OTelCommandMetrics. - OTelCommandMetrics subscribes to node-redis:command and node-redis:batch channels - Command filtering (include/exclude) handled in the start subscriber - Batch metrics use new node-redis:batch channel wrapping pipeline/multi - Zero inline OTel metric closures remain in core client code - Noop classes no longer need closure methods - Tests rewritten to verify filtering through TC events Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
No longer needed — when command metric group is disabled, no TC subscriptions are created, so hasSubscribers is false and traceCommand skips TracingChannel entirely. Zero cost without noops. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace every OTelMetrics.instance.* call in core code with publish(CHANNELS.*, factory) point events. All observability data now flows through diagnostics_channel: - TracingChannel for async lifecycles (command, batch, connect) - Plain dc.channel for point events (connection ready/closed, errors, maintenance, pubsub, cache, command replies) Core code has zero OTelMetrics imports. The publish() function uses factory callbacks + hasSubscribers checks for zero-cost when no APM subscribes. Channel names are centralized in CHANNELS const map with a type-safe ChannelEvents interface. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace 6 individual metric classes (OTelConnectionBasicMetrics,
OTelConnectionAdvancedMetrics, OTelResiliencyMetrics,
OTelClientSideCacheMetrics, OTelPubSubMetrics, OTelStreamMetrics)
with a single OTelChannelSubscribers class that subscribes to
diagnostics_channel events.
- All metric recording now happens via channel subscriptions
- Noop classes eliminated — when a metric group is disabled, no
subscription is created, zero overhead
- noop-metrics.ts reduced to a single NoopOTelMetrics shell
- IOTelMetrics interface simplified to just { commandMetrics }
- recordCommandReplyMetrics removed — pubsub out and stream lag
metrics now handled by channel subscribers on node-redis:command:reply
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
These tests only verified that dc.channel().publish() doesn't crash, which is a Node.js guarantee, not our responsibility. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…bscriptions - Add TRACE_COMMAND, TRACE_BATCH, TRACE_CONNECT to CHANNELS map - Replace raw dc.subscribe() on sub-channel strings with dc.tracingChannel().subscribe() for proper TracingChannel usage - Replace all remaining string literals in metrics.ts with CHANNELS.* Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove types that are no longer implemented by any class after the channel subscriber refactor: - RecordClientErrorContext, CommandReplyMetricHandler, ClientErrorOrigin, MetricErrorType, ConnectionCloseReason, CscResult, CscEvictionReason, ErrorCategory (type aliases) - METRIC_INSTRUMENT_TYPE (unused const) - RedisArgument import (no longer needed in types.ts) - Dead re-exports from opentelemetry/index.ts Const maps (CONNECTION_CLOSE_REASON, CSC_RESULT, etc.) are kept as they're used by tests and may be useful for external consumers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Export dc from tracing.ts and import it in metrics.ts instead of loading it independently in both modules. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Clarifies the distinction: BatchOperationContext is the batch operation as a whole (MULTI/PIPELINE), BatchCommandTraceContext is a single command within a batch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
No subscriber existed for this event. The original redis#3110 code only recorded at wait end, not start. Remove the speculative event. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The original code only decremented db.client.connection.count when the socket was previously ready. Our refactoring lost this gate in destroySocket(), which could decrement below zero if destroy was called before the socket became ready. Add wasReady to the ConnectionClosedEvent so the subscriber can guard correctly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Export CHANNELS map, ChannelEvents type map, and all individual
event types so APM libraries can subscribe with proper typing:
import { CHANNELS, type CommandTraceContext } from 'redis';
dc.subscribe(CHANNELS.ERROR, (ctx: ClientErrorEvent) => { ... });
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace CONNECTION_WAIT_END point event with TRACE_CONNECTION_WAIT TracingChannel. Pool wait is an async operation (start when no idle client, end when one becomes available) — TracingChannel captures the full lifecycle so APMs can create spans or measure duration. Core code emits lifecycle events only. Timing is computed by subscribers (OTel uses start/asyncEnd to measure duration). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use number | undefined everywhere instead of mixing with number?. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Eliminates repetitive ternary pattern. TracingChannelContextMap maps channel names to their context types so getTracingChannel infers the correct type without explicit generics. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tead metrics.ts no longer imports dc directly. All channel acquisition goes through the exported helpers which handle caching and undefined checks. dc remains private to tracing.ts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace traceCommand, traceBatch, traceConnect, traceConnectionWait with a single trace(CHANNELS.TRACE_*, fn, contextFactory) function. Channel name resolves the context type via TracingChannelContextMap. Also adds cache for getTracingChannel to avoid re-acquiring on every call, matching getChannel's caching pattern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace separate #subscriptions and #tracingChannels arrays with a single #unsubscribers list of cleanup closures. Simpler destroy(), no need to store channel/handler pairs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Inline the single remaining noop as an object literal in the default #instance. No noop classes or files remain. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove duplicate COMMAND_REPLY publish from sendCommand — typed commands already publish via _executeCommand. Also merge the two separate COMMAND_REPLY subscribers (pubsub out + streaming) into a single #subscribeCommandReply method. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When the acquire timeout fires, rejectWait() is called so the TracingChannel error channel fires — APMs see the timeout as an error instead of an orphaned span that never closes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When OTel is not initialized or disabled, skip instrument registration and subscriber creation entirely. No noop instruments needed — if there are no subscribers, channels are never fired. Removes 139-line noop-meter.ts and all noop fallback branches from instrument creation helpers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- COMMAND_REPLY in _executeCommand now passes sanitizeArgs(parser.redisArgs) instead of raw args, preventing sensitive values from leaking to subscribers - Pool connection wait trace promise gets .catch(noop) to prevent unhandled rejection if a tracing subscriber throws Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When pool.destroy() is called with tasks still queued, call rejectWait() on each pending task so the TracingChannel error channel fires and APM subscribers close their spans cleanly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Tracing test: check isOpen before destroying already-closed client - OTel E2E tests: rewrite 5 tests that called removed methods (resiliencyMetrics.recordClientErrors, streamMetrics.recordStreamLag) to publish via channels instead - Connection closed metric: split subscriber for basic (connection count) and advanced (close reason) so tests enabling only one group see correct metrics Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The original code always recorded wait time, even for 0ms waits when a client was available. The TracingChannel refactor only traced the "no client, must wait" path. Add trace for immediate availability so the metric is always emitted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
logaretm
force-pushed
the
feat/tracing-channel-v2
branch
from
March 26, 2026 05:43
362e1f9 to
c7d824c
Compare
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
1 participant
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.
Draft — Visual comparison of TracingChannel changes on top of redis#3110
This PR shows the diff of the TracingChannel refactoring on top of the OTel metrics PR (redis#3110).
What changed
Core code no longer calls OTel metrics directly. Instead it describes what's happening through
diagnostics_channel:trace()— wraps async operations (commands, batches, connections, pool waits) viaTracingChannelpublish()— emits point events (errors, cache hits, pubsub, maintenance) viadc.channel()OTel metrics subscribe to these channels externally. Any APM (Sentry, Datadog, custom) can do the same — zero additional surgery to core code.
Key numbers
tracing.tsinfrastructure (serves all consumers)OTelMetricsimports in core codetrace()andpublish()🤖 Generated with Claude Code