-
Notifications
You must be signed in to change notification settings - Fork 616
feat(instrumentation-redis)!: consolidate redis v2,3 and redis v4 instrumentation to one package #2915
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(instrumentation-redis)!: consolidate redis v2,3 and redis v4 instrumentation to one package #2915
Conversation
…strumentation now
… against earlier redis v4 versions
Also drop unused secondary redis v4 install:
"redis-v4": "npm:[email protected]",
… -> src/v2-v3/instrumentation.ts for a smaller PR diff
…he isntr and test files for git --follow history
|
Moved back to draft, because I need to cope with redisTypes usage in test/v2-v3/redis.test.ts now that the currently installed redis is no longer redis@3, so the types cause a |
|
Grrr, why can I not do this? (This is a rhetorical question.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer this testing approach since is more aligned to the improvements I'm planning to do in #2866
My only concern would be how we could lure users to this consolidated instrumentation so they don't get stuck into the last version of @opentelemetry/instrumentation-redis-4. How many time will pass until users notice they haven't been updating in a while and go check the README? Could we be more "noisy" about this change?
plugins/node/opentelemetry-instrumentation-redis/src/v2-v3/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-redis/test/v2-v3/redis.test.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-redis/test/v4/redis.test.ts
Outdated
Show resolved
Hide resolved
One of the followup tasks is to set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks great @trentm thank you for bringing it to the finish line 💯
|
we should also remove redis-4 from the component owner also |
|
and here :) |
It was merged into the `@opentelemetry/instrumentation-redis` package (as of version 0.50.0), which already has an entry here. See the deprecation notice at https://www.npmjs.com/package/@opentelemetry/instrumentation-redis-4 Refs: open-telemetry/opentelemetry-js-contrib#2915
|
The "follow-ups" are complete. |
This PR builds on Amir's #2878 (the instr-redis consolidation).
summary of diffs to Amir's
npm testjust tests against the version of redis currently installed andnpm run test-all-versionshandles testing against older versions of redis. (See my argument for this in a section below. commit 8a52338)gitto notice that src/instrumentation.ts was moved to src/v2-v3/instrumentation.ts. This makes for a smaller, clearer diff and **clearer history tracking withgit log --follow ...later. (commit 9b0fc9e)DEFAULT_CONFIGhandling in src/v4/instrumentation.ts, which I believe is no longer necessary (commit f46e166). Amir's PR had already dropped theDEFAULT_CONFIGhandling for src/v2-v3/instrumentation.ts.instrumentation-redis-4from auto-instrumentations-node and release-please: necessary given that I removed the package. (commit d2fa3a2) I'm not sure if this is a "breaking" change for auto-instrumentations-node. It certainly is something we should point out in the changelog / PR description.why the test changes
I moved all testing against redis v2 and v3 to
npm run test-all-versions. Testing against older versions of the target module is whattest-all-versionsis for.This, then, makes
npm testonly test against the currently installed version of the module being instrumented (redis v4). IMO this is whatnpm testshould be doing in all our packages.npm testshould not change the versions of things installed, which is what happens in Amir's PR (note the "invalid" version innpm lsafter runningnpm test):One current exception (the only one?) to this is the instrumentation-mongodb tests. Testing of multiple versions of mongodb was added to
npm testin #2681 to help with coverage numbers. A better answer is to get coverage data fromtest-all-versionruns. @david-luna is working on this. (Note that the testing of mongodb@3 was dropped fromnpm testas a workaround for flaky tests a while back: #2757 (comment))With my changes in this PR, to do local dev testing against redis@3, one would need to:
I think this is reasonable. There will be vanishly little dev on the legacy v2-v3 instrumentation.
follow-ups
If we go with this PR and once it is released:
npm deprecate ...for the old package. Dan did this, see the deprecation notice at: https://www.npmjs.com/package/@opentelemetry/instrumentation-redis-4slurp in the utils from theAmir correctly pointed out that instrumentation-ioredis also usesredis-commonpackage and deprecate / remove it.redis-common, so there is no driving need to drop it.pkg:instrumentation-redis-4label (2025-07-02).Deprecate the JS
@opentelemetry/instrumentation-redis-4package opentelemetry.io#7279 to do this.