-
Notifications
You must be signed in to change notification settings - Fork 544
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: missing telemetry in mysql2 when importing only promise API #2662
fix: missing telemetry in mysql2 when importing only promise API #2662
Conversation
plugins/node/opentelemetry-instrumentation-mysql2/test/mysql.test.prom.ts
Fixed
Show fixed
Hide fixed
plugins/node/opentelemetry-instrumentation-mysql2/test/mysql.test.prom.ts
Fixed
Show fixed
Hide fixed
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2662 +/- ##
=======================================
Coverage 90.95% 90.96%
=======================================
Files 172 172
Lines 8119 8137 +18
Branches 1646 1649 +3
=======================================
+ Hits 7385 7402 +17
- Misses 734 735 +1
|
268adf4
to
74b7ea9
Compare
plugins/node/opentelemetry-instrumentation-mysql2/test/mysql.test.ts
Dismissed
Show dismissed
Hide dismissed
plugins/node/opentelemetry-instrumentation-mysql2/test/mysql.test.ts
Dismissed
Show dismissed
Hide dismissed
74b7ea9
to
b9093ae
Compare
This comment was marked as outdated.
This comment was marked as outdated.
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.
Overall looks good, thanks for chasing this down. 🙌
Just one comment nit and one blocker around addSpanProcessor
.
Due to the re-format and change in nesting, the diff here initially looked more daunting than it actually was 🙂
plugins/node/opentelemetry-instrumentation-mysql2/test/mysql.test.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-mysql2/test/mysql.test.ts
Outdated
Show resolved
Hide resolved
yeah, sorry about that, but I had to find a way to use different before/after blocks. I'll take care of the suggestions, thank you! |
…est.ts Co-authored-by: Marc Pichler <[email protected]>
7a44323
to
1f82b74
Compare
instrumentation.enable(); | ||
instrumentation.disable(); | ||
|
||
mysqlTypesPromReload = await import('mysql2/promise'); |
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.
nit: by importing the types at the top of the file you can avoid loading twice the module which may improve perf (although not noticeable) and avoid any possible odd scenario. Actually I found quite misfortunate to name mysqlTypes
and mysqlTypesProm
the imported modules. The types
in the names is quite misleading since it's the actual module object
…improve readability Update plugins/node/opentelemetry-instrumentation-mysql2/test/mysql.test.ts
ac6eed1
to
56bd9dd
Compare
Which problem is this PR solving?
See #2590
Short description of the changes
Patch
lib/connection.js
that is always imported