Skip to content
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

Merged
merged 3 commits into from Apr 30, 2024

Conversation

JamieDanielson
Copy link
Member

Which problem is this PR solving?

  • auto-instrumentations-node is expected to include all instrumentations in js and js-contrib, but didn't yet have the newly created and published undici

Short description of the changes

  • add undici instrumentation to auto-instrumentations-node package

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.

Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.47%. Comparing base (dfb2dff) to head (42d4ad3).
Report is 93 commits behind head on main.

❗ Current head 42d4ad3 differs from pull request most recent head 1f61115. Consider uploading reports for the commit 1f61115 to get more accurate results

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     
Files Coverage Δ
...tapackages/auto-instrumentations-node/src/utils.ts 98.96% <100.00%> (+0.18%) ⬆️

... and 27 files with indirect coverage changes

@@ -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)
Copy link
Contributor

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.

Copy link
Member Author

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 😄

@blumamir
Copy link
Member

Thanks @JamieDanielson , can you please resolve the conflicts so this can merge?

@JamieDanielson
Copy link
Member Author

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 😃 )

@blumamir
Copy link
Member

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 👍

@blumamir blumamir merged commit 4463483 into open-telemetry:main Apr 30, 2024
15 checks passed
@dyladan dyladan mentioned this pull request Apr 29, 2024
@JamieDanielson JamieDanielson deleted the jamie.add-undici-to-auto branch April 30, 2024 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants