-
Notifications
You must be signed in to change notification settings - Fork 547
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
feat: mysql2 instrumentation #521
feat: mysql2 instrumentation #521
Conversation
If patched modules are close, i think we should just add support for mysql2 inside |
Codecov Report
@@ Coverage Diff @@
## main #521 +/- ##
==========================================
+ Coverage 94.77% 94.82% +0.05%
==========================================
Files 152 157 +5
Lines 9342 9862 +520
Branches 921 957 +36
==========================================
+ Hits 8854 9352 +498
- Misses 488 510 +22
|
f703b66
to
1913815
Compare
Yes... It would be totally fine, unless... Types. |
Not sure to understand why types are a problem ? |
We don't have a good precedent of 2-package instrumentation - I mainly don't want to start that discussion here. To answer your question: Because those do not match between the two packages. Not saying it's not doable, it absolutely is with unions and whatnot, but transforming the code to work with |
FYI we have both http (that handle https too) and grpc (that handle grpc-js and grpc c++) that have this behavior and we don't have any issue with it. |
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
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
Thanks for pointing this out, @vmarchaud. I'll look into that at some point! :) |
cc @open-telemetry/javascript-approvers would appreciate some review so we can ship this with the next release :) thanks |
Which problem is this PR solving?
Adds autoinstrumentation for
mysql2
.Short description of the changes
This is pretty much the same as
opentelemetry-instrumentation-mysql2
, but I simplified it by only wrappingConnection.prototype
. This exact code might actually work formysql
as well.