-
Notifications
You must be signed in to change notification settings - Fork 518
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
fix: Add support for setConfig in ioredis instrumentation #689
fix: Add support for setConfig in ioredis instrumentation #689
Conversation
The fix doesn't match to the description in the issue which tells that the problem is that It seem to work in your test but this is just because it does an additional enable/disable. There is no need to override |
@Flarna Thank you for the feedback, I have updated the instrumentation to use the latest config from |
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.
Thanks for contributing and fixing this issue.
Wrote few minor optional comments. Feel free to address them or resolve if you do not want to fix them as part of this PR.
plugins/node/opentelemetry-instrumentation-ioredis/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-ioredis/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-ioredis/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #689 +/- ##
==========================================
+ Coverage 96.31% 96.86% +0.55%
==========================================
Files 5 11 +6
Lines 515 701 +186
Branches 97 130 +33
==========================================
+ Hits 496 679 +183
- Misses 19 22 +3
|
moved traceConnection to instrumentation.ts refactored tests to use setConfig instead of new instrumentation
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.
LGTM
@mustafain117 You'll need to rebase the PR |
@vmarchaud Done |
we can't merge this as it is outdated, please iether update to latest or allow to edit |
@obecny Updated it |
Which problem is this PR solving?
Short description of the changes
IORedisInstrumentation