fix(instrumentation-redis): remove redis types from public API

[pichlermarc]

Which problem is this PR solving?

Having both @opentelemetry/instrumentation-redis and redis@^4.0.2 (common when using @opentelemetry/autoinstrumentations-node) installed causes typescript compilation errors.

Fixes #1323

Short description of the changes

• This PR fixes the above problem by not using the removed Callback type in the public API.
• Also updates the link to command.js in the comment.
• Removes @types/redis as a dependency and moves it to devDependencies instead

[codecov]

Codecov Report

Merging #1424 (474ad7d) into main (5f180aa) will decrease coverage by 0.18%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1424      +/-   ##
==========================================
- Coverage   96.13%   95.96%   -0.18%     
==========================================
  Files          14       17       +3     
  Lines         906     1066     +160     
  Branches      197      217      +20     
==========================================
+ Hits          871     1023     +152     
- Misses         35       43       +8     

Impacted Files	Coverage Δ
...metry-instrumentation-redis/src/instrumentation.ts	93.47% <100.00%> (ø)

... and 2 files with indirect coverage changes

[Flarna]

The change itself is ok but it doesn't solve the problem that the redis instrumentation exposes the redis types.
The use of typeof redisTypes in a protected method and on the base class still causes that @types/redis is required to be a dependency instead a devDependency.

Could you replace typeof redisTypes at these places by any or similar and update package.json?

[pichlermarc]

The change itself is ok but it doesn't solve the problem that the redis instrumentation exposes the redis types. The use of typeof redisTypes in a protected method and on the base class still causes that @types/redis is required to be a dependency instead a devDependency.

Could you replace typeof redisTypes at these places by any or similar and update package.json?

Thanks for the feedback, I updated the PR with these changes (b86dc5e) 🙂

I feel like I still lack some understanding of the subject, though:

• not using Callback from redis fixed the issue in my local testing setup
  • As I understand this fixes the problem as Callback does not exist in redis 4, so not using it in the public API works with the types shipped with redis@^4.0.0.
  • As we're now not using anything from the @types/redis package, I was under the impression that it should be safe to use it in this way as type-arguments.

We're doing something similar in the express instrumentation (depending on @types/express using it in the instrumentation class and in one of its protected methods) - would that need to be subject to the same changes? I'd like to provoke some of the issues caused by using types in this manner in a local app so I can understand the problem a bit better 🙂

[Flarna]

Using the types in the user facing API is always a time bomb. We had this in several instrumentations in the past. You may fix one problem by removing Callback but other problems remain.

If it is in the user facing API it needs to be in the dependencies otherwise users using typescript see issues because of missing types.
If it is in the dependencies it's a specific version which is likely not that one used in actual application. Might again cause all sorts of issues.
If types are included in the package itself it's even worse as instrumentation has to depend on a specific version of the module it instruments.

The only safe variant is to keep the user facing API free of types of 3rd party modules. see also here.

[Flarna]

You might want to update description a bit as it's now a bit more then do not use Callback.

[pichlermarc]

@Flarna got it, thanks! 🙂