-
Notifications
You must be signed in to change notification settings - Fork 580
a plan for redis
instrumentation
#2867
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
Comments
I support this plan. 👍
I think this could be helpful - though I feel redis@3 should be old enough by now for us to drop it without replacement, it still seems to steadily sit at about 1M downloads per week. I'll let you make the call if the extra effort is worth it.
Q: can redis@4 ever be double-wrapped? IIUC the instrumentation that comes last is the only one that gets to wrap, all others loose out as
Having one version that does this could be helpful. So IIUC the order of things would be:
|
Ah, good point. You are right. |
The maintainers discussed this this morning and @blumamir mentioned that he has a merge on the go (not quite a draft PR yet) that would basically do the first "Alternatives considered". I don't know the exact set of steps, but the result would be a instrumentation-redis with support for redis 2,3,4, and 5. |
BTW the argument in favour of keeping support for redis 2 and 3 is that they still have large numbers of npm downloads. https://www.npmjs.com/package/redis?activeTab=versions
I guess we'll never know what percentage of npm download numbers are CIs running silently ("If a tree falls in the forest..."). |
Currently we have:
@opentelemetry/instrumentation-redis
which instruments redis@2 (last release 2017-08-08) and redis@3 (last release 2021-04-20), and@opentelemetry/instrumentation-redis-4
which instruments redis@4 (active)Proposal
tl;dr: We move to one
@opentelemetry/instrumentation-redis
that instruments redis@4 and redis@5. We drop support for instrumenting older versions of redis.Details for the legacy package:
@opentelemetry/instrumentation-redis-legacy
. If not, the "archive/..." code is replaced with just a README.md similar to the archived "browser-extension-autoinjection".Details for the current
@opentelemetry/instrumentation-redis-4
package:@opentelemetry/instrumentation-redis
.@opentelemetry/auto-instrumentations-node
updates to this new release and removes@opentelemetry/instrumentation-redis-4
-- effectively dropping support for redis@2 and redis@3.@opentelemetry/instrumentation-redis-4
package in npm is marked deprecated with a pointer to the new package and issue explaining the move.@opentelemetry/instrumentation-redis
package.@opentelemetry/redis-common
package is deprecated, moved to the "archive/" dir, and its (small) code is moved into the modern instr-redis package.Open questions:
@opentelemetry/instrumentation-redis-legacy
so that it is relatively straightforward for a user to install both the legacy and modern redis instrumentations in the same app (without having the use npm package aliases) to instrument old and modern versions of redis?Troubleshooting:
redis@4
. We could consider a final release of instr-redis-4 that disables itself by default.auto-instrumentations-node
and that havegetNodeAutoInstrumentations({'@opentelemetry/instrumentation-redis-4': ...})
will have that config no longer apply. We could updategetNodeAutoInstrumentations
to support the old...-redis-4
configuration key with a deprecation warning for a period of time.Alternatives considered
instrumentation-redis
to support redis 2, 3, and 4 by merging in the code in "plugins/node/opentelemetry-instrumentation-redis-4". I think this would be straightforward. However, given how old redis 2 and 3 are, it seemed like a waste of time. Redis 3 is four years old at this point. Is there value in supporting it? In addition, some of the same coordination with auto-instrumentation-node above would need to be done. I'm not totally opposed to this alternative.The text was updated successfully, but these errors were encountered: