feat(instrumentation-redis): add support for redis@^4.0.0

[blumamir]

Which problem is this PR solving?

• fixes redis instrumentation - support v4 #834

Short description of the changes

The new version splits the functionality into sub-packages.
I added patches for the relevant function in the subpackage: @node-redis/client:

• patch function extendWithCommands, to allow access to executor which is the function that records spans upon command execution
• patch RedisClientMultiCommand.exec function to correctly handle transactions and MULTI commands.
• patch RedisClient.multi function to allow setting the client options on the multi command object to allow recording network attributes for transactions.

I added support for all existing config options.
Added tests for v4 + added it to test all versions and made sure it's green
Since v4 supports node >= 12, the package.json and CI actions were updated accordingly. This might be breaking for users of redis 2-3 with node < 12.

Checklist

• Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.

[codecov]

Codecov Report

Merging #982 (5723101) into main (4b36b75) will decrease coverage by 2.44%.
The diff coverage is 80.23%.

❗ Current head 5723101 differs from pull request most recent head e72dd3f. Consider uploading reports for the commit e72dd3f to get more accurate results

@@            Coverage Diff             @@
##             main     #982      +/-   ##
==========================================
- Coverage   95.91%   93.46%   -2.45%     
==========================================
  Files          13       18       +5     
  Lines         856     1179     +323     
  Branches      178      236      +58     
==========================================
+ Hits          821     1102     +281     
- Misses         35       77      +42     

Impacted Files	Coverage Δ
...try-instrumentation-redis-4/src/instrumentation.ts	79.76% <79.76%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts	97.95% <100.00%> (ø)
...opentelemetry-instrumentation-redis-4/src/utils.ts	100.00% <100.00%> (ø)
...e/opentelemetry-instrumentation-redis/src/utils.ts	93.10% <0.00%> (ø)
...metry-instrumentation-redis/src/instrumentation.ts	93.47% <0.00%> (ø)

[rauno56]

If the implementation doesn't share a lot of code I'd split it into a totally separate package. Do you know of any drawbacks that approach might have?

[blumamir]

If the implementation doesn't share a lot of code I'd split it into a totally separate package. Do you know of any drawbacks that approach might have?

I support this too.

The drawbacks I can spot:

• Does not enforce consistent types - e.g. the instrumentation config is declared in two different packages. Future fixes and features could be easily added to just one and make them diverge. Not sure how much that actually matters though.
• Might be slightly more complicated for consumers - they will have to check versions in the app and choose the right package to use. I guess it shouldn't be a big deal to wrap them both into a single package that installs both and expose them as a single package that covers all versions.
• Extra mental step in maintaining these packages - need to support two packages instead of one.

Benefits:

• Both variants will have tests in CI
• We can state different node version support for each package (redis 3 supports node >=8 but v4 supports node >= 12)
• Smoother development - correct version is found in devDependencies
• Each instrumentation can depend on the right types for the versions it instruments
• Less confusion for the codecov tool - will see tests that cover both patches for old and new versions, as opposed to just running it on a single set.

What do you think a name for a package such as this should be? @opentelemetry/instrumentation-redis-2(covers both 2 and 3), @opentelemetry/instrumentation-redis-4 (covers current 4 and maybe future 5,6,...)?

[rauno56]

Naming things - the hardest part.

I don't have a strong preference. If had to come up with good heuristics on this, those would be:

1. Don't change the one that's there if possible;
2. Make what's included as clear as possible;
3. Make the default clear.

Based on that, I'd leave @opentelemetry/instrumentation-redis to include instrumentation for versions 2 and 3(violates my point nr 3, but anything else would violate nr 1) and @opentelemetry/instrumentation-redis-4 to include one for versions 4 and up until they make another very significant API change.

[blumamir]

Naming things - the hardest part.

I don't have a strong preference. If had to come up with good heuristics on this, those would be:

1. Don't change the one that's there if possible;
2. Make what's included as clear as possible;
3. Make the default clear.

Based on that, I'd leave @opentelemetry/instrumentation-redis to include instrumentation for versions 2 and 3(violates my point nr 3, but anything else would violate nr 1) and @opentelemetry/instrumentation-redis-4 to include one for versions 4 and up until they make another very significant API change.

Like, I'll update the PR to express that.
Thanks

[blumamir]

If the implementation doesn't share a lot of code I'd split it into a totally separate package. Do you know of any drawbacks that approach might have?

Done. There are now 2 packages: @opentelemetry/instrumentation-redis, @opentelemetry/instrumentation-redis-4

[rauno56]

Nice! 👍 Sorry for taking so long to review.

Check the test failures in CI. Other than that. LGTM.

[blumamir]

redis package released version 4.1.0 10 days ago (on 02 May 2022) which changed the internal function names and the internal package names.

This currently breaks the tests (and support) for this new version.
I haven't yet checked what should be fixed in the instrumentation for this.
In the meantime, updating here about the status. Hope I can submit a fix soon.

[blumamir]

@habmic @rauno56 @dyladan
After investigating the change, I found out that in version 4.1.0 the internal package namespace in npm changed from @node-redis/client to @redis/client, and the a patched function changed it's name from extendWithCommands to attachCommands.

I udpate the PR accordingly and now test-all-versions is green again for all versions on my local setup (node 16) and CI is green for the latests version (4.1.0) except for node 12.

For node 12 it seems to fail because of an issue with redis package itself, documented here, here and here. The package itself does not publish the supported node version anywhere in a clear way, but I found this text in the changelog of the relevant subpackage of the latest version:

Drop support for node 12

I am going to remove the tests for redis 4 for node 12

[blumamir]

CI is now green.
Feel free to review again after the latest changes. This PR is ready to merge as far as I am concerned.

[rauno56]

@blumamir, feel free to resolve the conflicts and merge right away.