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(auto-instrumentation-node): add undici instrumentation #2131
feat(auto-instrumentation-node): add undici instrumentation #2131
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2131 +/- ##
==========================================
- Coverage 90.97% 90.47% -0.50%
==========================================
Files 146 147 +1
Lines 7492 7594 +102
Branches 1502 1575 +73
==========================================
+ Hits 6816 6871 +55
- Misses 676 723 +47
|
@@ -179,6 +179,7 @@ registerInstrumentations({ | |||
- [@opentelemetry/instrumentation-redis](https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/plugins/node/opentelemetry-instrumentation-redis) | |||
- [@opentelemetry/instrumentation-restify](https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/plugins/node/opentelemetry-instrumentation-restify) | |||
- [@opentelemetry/instrumentation-socket.io](https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/plugins/node/instrumentation-socket.io) |
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.
My alphabetical spidey-sense tingled here: I think this list is missing instr-tedious.
Anyway, obviously not something this PR needs worry about.
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.
Oof good catch! Yes that's a good follow-up though, will take a look once this merges. At least the instrumentation is in there so that's probably the most important part 😄
Thanks @JamieDanielson , can you please resolve the conflicts so this can merge? |
I think this is good to go, but unsure if I should hold off for a maintainer to merge as they are assigned component owners (I am but a lowly approver 😃 ) |
Not sure about the formalities, but since it already has 3 approves and is open for more than a week, I think it's safe to merge 👍 |
Which problem is this PR solving?
Short description of the changes
Note: As I understand it, we haven't had an official "release" of undici instrumentation, but we recently found that new packages like this still got published on npm due to a misconfiguration in our workflows.
Also, I couldn't recall if there was a reason to delay adding this to the package (like since it's still so new), but figured I'd open the PR either way because I didn't see it otherwise.