-
Notifications
You must be signed in to change notification settings - Fork 852
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(grpc-instrumentation): migrate grpc to instrumentation #1656 #1744
feat(grpc-instrumentation): migrate grpc to instrumentation #1656 #1744
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1744 +/- ##
==========================================
- Coverage 92.30% 92.16% -0.15%
==========================================
Files 163 167 +4
Lines 5510 5845 +335
Branches 1188 1256 +68
==========================================
+ Hits 5086 5387 +301
- Misses 424 458 +34
|
@vmarchaud should we wait until you add grpc-js too ? |
@obecny I prefer to split this into two PR |
whatever will be easier. |
dac7eed
to
1e1eef4
Compare
4a9f735
to
d14b40c
Compare
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.
few issue with status code, other than that lgtm
|
||
private _getInternalPatchs() { | ||
return [ | ||
new InstrumentationNodeModuleFile<GrpcInternalClientTypes>( |
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: split this into 2 new internal function for better readability for example
private _getInternalPatchs() {
return [
this.patchClientVerTill1_6,
this.patchClientVerAbove1_6,
]
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.
Since both patches were using the same duplicated code, i prefered to declare both onPatch
and onUnPatch
and just re-use them. PTAL
d14b40c
to
d249f3d
Compare
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.
Looks good thanks
8adc920
to
41f11d8
Compare
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
…metry#1656 (open-telemetry#1744) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
…metry#1656 (open-telemetry#1744) Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
I needed few modifications of the base instrumentation to got the grpc plugin, they should be backward compatible except for the
InstrumentationModuleFile
constructor where i addedsupportedVersions
as the second arguemnt to matchInstrumentationNodeModuleFile
.As asked here, i think it would make sense to have both grpc/grpc-js in the same instrumentation, so i added the test suite from the
grpc-utils
package into this one while waiting to migrategrpc-js
.