feat(redis): consolidate redis v2,3 and redis v4 instrumentation to one package #2878

blumamir · 2025-06-07T15:54:59Z

related: #2867

This PR is a preparation for adding support to redis@5 in a followup PR #2830

Motivation

merge redis and redis-4 into one package which users can install without thinking about the redis version they are using.
align with the rest of the repo which uses one npm package per instrumentation.
limit the number of packages we need to handle so that we don't grow them when new versions are added.
the name redis-4 is a bit confusing since it's not obvious that 4 is the version. this pattern doesn't play well when a single package instrument multiple versions.

Change

Create a wrapper RedisInstrumentation which installs both v2-v3 and v4 existing instrumentations. In a followup PR it will be trivial to also add support to v5 which again had major internal refactor not trivial to support as part of v4.

allows to conveniently manage each target version range as different InstrumentationBase as we do now (with potentially specific patches, types, utils, tests.
This PR only takes care of merging existing implementation into the single @opentelemetry/instrumentaiton-redis without any other improvements or refactors. instrumentation logic is remain as is.
I have tested it when both redis (new) and redis-4 (deprecated) instrumentations are registered together, and the spans are only emitted once. This is due to instrumentation checking isWrapped on the patched function and unwrapping before applying a new patch. While this works, it means that only the last instrumentation to patch will apply. I suggest that after a migration period we will re-publish redis-4 with empty implementation to signal users to stop using it and migrate to the maintained package.

…st objects

codecov · 2025-06-07T16:17:42Z

Codecov Report

Attention: Patch coverage is 67.28972% with 105 lines in your changes missing coverage. Please review.

Project coverage is 88.75%. Comparing base (11a94b0) to head (3945aab).
Report is 32 commits behind head on main.

Files with missing lines	Patch %	Lines
...instrumentation-redis/src/v2-v3/instrumentation.ts	17.14%	58 Missing ⚠️
...ry-instrumentation-redis/src/v4/instrumentation.ts	82.08%	36 Missing ⚠️
...metry-instrumentation-redis/src/instrumentation.ts	74.28%	9 Missing ⚠️
...pentelemetry-instrumentation-redis/src/v4/utils.ts	86.66%	2 Missing ⚠️

❌ Your patch status has failed because the patch coverage (67.28%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2878      +/-   ##
==========================================
- Coverage   89.68%   88.75%   -0.94%     
==========================================
  Files         187      190       +3     
  Lines        9059     9311     +252     
  Branches     1858     1903      +45     
==========================================
+ Hits         8125     8264     +139     
- Misses        934     1047     +113

Files with missing lines	Coverage Δ
...telemetry-instrumentation-redis/src/v2-v3/utils.ts	`21.05% <ø> (ø)`
...pentelemetry-instrumentation-redis/src/v4/utils.ts	`86.66% <86.66%> (ø)`
...metry-instrumentation-redis/src/instrumentation.ts	`76.92% <74.28%> (-14.86%)`	⬇️
...ry-instrumentation-redis/src/v4/instrumentation.ts	`82.08% <82.08%> (ø)`
...instrumentation-redis/src/v2-v3/instrumentation.ts	`17.14% <17.14%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

blumamir · 2025-06-08T14:19:40Z

plugins/node/opentelemetry-instrumentation-redis/package.json

    "cross-env": "7.0.3",
    "nyc": "17.1.0",
    "redis": "3.1.2",
+    "redis-v4": "npm:[email protected]",


reviewer note: this is needed for the v4 tests.
Since redis version is 3 by default, then npm run compile would fail when trying to import RedisClientType if not pulled into node_modules somehow.

When running the tests, tav redis 4.7.1 would take care of updating the redis to make the test pass, but compile is run before that

blumamir · 2025-06-08T14:21:00Z

plugins/node/opentelemetry-instrumentation-redis/package.json

+    "test": "npm run test-v2-v3 && npm run test-v4",
+    "test-v2-v3": "tav redis 3.1.2 npm run test-v2-v3-run",
+    "test-v4": "tav redis 4.7.1 npm run test-v4-run",
+    "test-v2-v3-run": "nyc mocha --no-clean --require '@opentelemetry/contrib-test-utils' 'test/v2-v3/*.test.ts'",
+    "test-v4-run": "nyc mocha --no-clean --require '@opentelemetry/contrib-test-utils' 'test/v4/*.test.ts'",


reviewer note:

this is aligned with @opentelemetry/instrumentation-mongodb

IMO we should be leaving testing of multiple versions of the target library to test-all-versions tests -- that's what test-all-versions is for. I think the mongodb testing change was misguided.

I have an alternative PR that builds on yours that shows what I mean: #2915
That PR also has a few other changes. PTAL.

blumamir · 2025-06-08T14:24:51Z

plugins/node/opentelemetry-instrumentation-redis/test/v2-v3/redis.test.ts

+const instrumentation = registerInstrumentationTesting(
+  new RedisInstrumentation()
+);


reviewer note:

Now that we have more than one ".test.ts" file, without this we get multiple errors in tests because the version that create the "patch" (first one) might be different than the one that the test is using. Migrating to the contrib-test-utils framework avoid these issues by registering only a single instrumentation instance and making sure it is used consistently

It also remove a lot of boiler plate code and makes the tests file lighter

blumamir · 2025-06-08T14:27:48Z

plugins/node/opentelemetry-instrumentation-redis/test/v2-v3/redis.test.ts

      });
    });

-    describe('Removing instrumentation', () => {


reviewer note:

This was tricky to support in test after the refactor, and since "disable" is not a feature that we recommend users, and cannot work in some cases, I removed it from here

blumamir · 2025-06-08T14:29:52Z

plugins/node/opentelemetry-instrumentation-redis/src/types.ts

-// exported from
-// https://github.com/redis/node-redis/blob/v3.1.2/lib/command.js
-export interface RedisCommand {
-  command: string;
-  args: string[];
-  buffer_args: boolean;
-  callback: (err: Error | null, reply: unknown) => void;
-  call_on_write: boolean;
-}


reviewer note:

this is technically a breaking change (since it was publicly exported by the package and now it is not). but it's only used internally and there is no reason for any user to consume it for anything.

How about we put the ! marker on the PR title, then, and add a section to the PR description that mentions the remote-but-technically-possible breaking change?

trentm · 2025-06-26T20:33:31Z

plugins/node/opentelemetry-instrumentation-redis-4/README.md

@@ -1,5 +1,7 @@
 # OpenTelemetry redis Instrumentation for Node.js

+> ⚠️ **DEPRECATED**: The support for `redis@4` instrumentation is now part of `@opentelemetry/instrumentation-redis`. please use it instead.


Suggested change

> ⚠️ **DEPRECATED**: The support for `redis@4` instrumentation is now part of `@opentelemetry/instrumentation-redis`. please use it instead.

> ⚠️ **DEPRECATED**: The support for `redis@4` instrumentation is now part of `@opentelemetry/instrumentation-redis` (as of v0.50.0). Please use it instead.

trentm · 2025-06-26T20:41:16Z

plugins/node/opentelemetry-instrumentation-redis/src/types.ts

-// exported from
-// https://github.com/redis/node-redis/blob/v3.1.2/lib/command.js
-export interface RedisCommand {
-  command: string;
-  args: string[];
-  buffer_args: boolean;
-  callback: (err: Error | null, reply: unknown) => void;
-  call_on_write: boolean;
-}


How about we put the ! marker on the PR title, then, and add a section to the PR description that mentions the remote-but-technically-possible breaking change?

trentm · 2025-06-26T21:13:52Z

plugins/node/opentelemetry-instrumentation-redis/src/v4/instrumentation.ts

+
+  override setConfig(config: RedisInstrumentationConfig = {}) {
+    super.setConfig({ ...DEFAULT_CONFIG, ...config });
+  }


I believe you can drop the DEFAULT_CONFIG handling in this file, as you already did for src/v2-v3/instrumentation.ts:

diff --git a/plugins/node/opentelemetry-instrumentation-redis/src/v4/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-redis/src/v4/instrumentation.ts index 5ef319df..38f444ca 100644 --- a/plugins/node/opentelemetry-instrumentation-redis/src/v4/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-redis/src/v4/instrumentation.ts @@ -48,19 +48,11 @@ interface MutliCommandInfo { commandArgs: Array<string | Buffer>; } -const DEFAULT_CONFIG: RedisInstrumentationConfig = { - requireParentSpan: false, -}; - export class RedisInstrumentationV4 extends InstrumentationBase<RedisInstrumentationConfig> { static readonly COMPONENT = 'redis'; constructor(config: RedisInstrumentationConfig = {}) { - super(PACKAGE_NAME, PACKAGE_VERSION, { ...DEFAULT_CONFIG, ...config }); - } - - override setConfig(config: RedisInstrumentationConfig = {}) { - super.setConfig({ ...DEFAULT_CONFIG, ...config }); + super(PACKAGE_NAME, PACKAGE_VERSION, config); } protected init() {

trentm · 2025-06-27T16:38:22Z

plugins/node/opentelemetry-instrumentation-redis/package.json

+    "test": "npm run test-v2-v3 && npm run test-v4",
+    "test-v2-v3": "tav redis 3.1.2 npm run test-v2-v3-run",
+    "test-v4": "tav redis 4.7.1 npm run test-v4-run",
+    "test-v2-v3-run": "nyc mocha --no-clean --require '@opentelemetry/contrib-test-utils' 'test/v2-v3/*.test.ts'",
+    "test-v4-run": "nyc mocha --no-clean --require '@opentelemetry/contrib-test-utils' 'test/v4/*.test.ts'",


IMO we should be leaving testing of multiple versions of the target library to test-all-versions tests -- that's what test-all-versions is for. I think the mongodb testing change was misguided.

I have an alternative PR that builds on yours that shows what I mean: #2915
That PR also has a few other changes. PTAL.

blumamir · 2025-07-02T17:47:15Z

replaced with #2915

blumamir added 6 commits May 28, 2025 07:39

refactor(instrumentation-redis): support multi versions as seprate in…

f44b587

…st objects

Merge remote-tracking branch 'upstream/main' into redis-consolidation

e776bc6

fix(redis): unit tests for new setup

2329836

chore: add redis-4 impl to redis inst

65b09d0

test: fix redis tests with v4

ca617a5

fix: redis docs to include consolidated version

4ecf3a8

blumamir requested a review from a team as a code owner June 7, 2025 15:55

blumamir marked this pull request as draft June 7, 2025 15:55

github-actions bot assigned blumamir Jun 7, 2025

github-actions bot added the pkg:instrumentation-redis label Jun 7, 2025

blumamir added 3 commits June 7, 2025 18:56

chore: lint:fix

537cd34

chore: fix compile

de9f42e

chore: fix description in package.json

b99ef55

blumamir added 4 commits June 8, 2025 14:10

fix: running tav

5279606

fix(redis): v4 test compile

63638da

chore: resolve lint with knip ignore

1ec39b4

chore: add v prefix when using multiple versions

5b2c10b

blumamir commented Jun 8, 2025

View reviewed changes

chore: add deprecation warning to redis-4

3945aab

github-actions bot added the pkg:instrumentation-redis-4 label Jun 8, 2025

blumamir marked this pull request as ready for review June 9, 2025 08:23

This was referenced Jun 12, 2025

chore(deps-dev): bump redis from 4.7.0 to 5.5.6 in /packages/opentelemetry-node elastic/elastic-otel-node#817

Closed

chore: improve test workflow and coverage reports #2866

Merged

trentm mentioned this pull request Jun 26, 2025

feat(instrumentation-redis)!: consolidate redis v2,3 and redis v4 instrumentation to one package #2915

Merged

4 tasks

trentm reviewed Jun 27, 2025

View reviewed changes

blumamir closed this Jul 2, 2025

		@@ -1,5 +1,7 @@
		# OpenTelemetry redis Instrumentation for Node.js

		> ⚠️ DEPRECATED: The support for `redis@4` instrumentation is now part of `@opentelemetry/instrumentation-redis`. please use it instead.

feat(redis): consolidate redis v2,3 and redis v4 instrumentation to one package #2878

feat(redis): consolidate redis v2,3 and redis v4 instrumentation to one package #2878

Uh oh!

Conversation

blumamir commented Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Change

Uh oh!

codecov bot commented Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trentm Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trentm Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blumamir commented Jul 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

blumamir commented Jun 7, 2025 •

edited

Loading

codecov bot commented Jun 7, 2025 •

edited

Loading

trentm Jun 27, 2025 •

edited

Loading

trentm Jun 27, 2025 •

edited

Loading