feat: add instrumentation for undici versions >=5 <7 and global fetch API

[david-luna]

Which problem is this PR solving?

Node's fetch global API is not instrumented therefore any outgoing request made with it are not going to be traced.
Ref: open-telemetry/opentelemetry-js#4333

The fetch API is using undici under the hood and diagnostic channels emit info also when fetch is used. So this instrumentation works for:

• apps using fetch
• apps using undici
• and apps using both

Short description of the changes

• Add new instrumentation for undici package
• Add tests for fetch and undici

[codecov]

Codecov Report

Merging #1951 (e10ac98) into main (dfb2dff) will decrease coverage by 0.16%.
Report is 35 commits behind head on main.
The diff coverage is 98.36%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1951      +/-   ##
==========================================
- Coverage   90.97%   90.82%   -0.16%     
==========================================
  Files         146      148       +2     
  Lines        7492     7672     +180     
  Branches     1502     1537      +35     
==========================================
+ Hits         6816     6968     +152     
- Misses        676      704      +28     

Files	Coverage Δ
...rumentation-undici/src/enums/SemanticAttributes.ts	100.00% <100.00%> (ø)
plugins/node/instrumentation-undici/src/undici.ts	98.36% <98.36%> (ø)

... and 8 files with indirect coverage changes

[pichlermarc]

Thanks for working on this 🚀
(some comments from a very quick pass, detailed review to follow 🙂)

[JamieDanielson]

Thanks so much for working on this! I tested it out locally and it works great 🎉

I've added some suggestions for a few typos (those should be one-click-commit), and also I'd like to rethink the semantic attributes approach and instead use the semantic-convention package. It will then use some older conventions, yes, but it will be aligned with the other instrumentations. Then when we're ready we can update the semantic convention package for all instrumentations.

[JamieDanielson]

Hm, I'm not sure it's a good idea to set semantic attributes here in this instrumentation instead of using the semantic convention package. Is there a reason we are doing this? I would suggest using the @opentelemetry/semantic-conventions package instead. For example, here is a section in the http instrumentation using the semantic attributes.

[david-luna]

the motivation is to use the latest version of semantic conventions and the way (as agreed in the SIG). The current version available miss most of the attributes:

• http.request.method
• url.full
• url.path
• url.query
• url.scheme
• server.address
• server.port
• user_agent.original
• network.peer.address
• network.peer.port
• ...

We could import semantic conventions for a couple of attributes but the rest are not available yet therefore we need to have it in this package and replace them when we finally get semconv updated

[pichlermarc]

Yes, we did this with a couple of packages already where we needed names that were not available in @opentelemetry/semantic-conventions yet. As David said, the package is very outdated but we can't re-generate it until @opentelemetry/[email protected], as it's already stable.

I agree that it's not optimal, but let's do it like this for now where we add it to the instrumentation package then follow-up once the @opentelemetry/[email protected] situation is solved.

[JamieDanielson]

Okay, sorry I missed that conversation about adding them here. My bigger concern really is about the http semantic conventions that changed, and trying to be consistent with other instrumentations and help end users. For example, look at this trace that contains both http and undici instrumentation. The http instrumentation uses the older semantic conventions, and the undici uses the newer. This means the same fields have different attribute names, which can be difficult for folks to deal with and compare in their backend. For the attributes that are the same, can we use the old ones and/or introduce the environment variable for http/dup to let folks get both?

[pichlermarc]

Okay, sorry I missed that conversation about adding them here. My bigger concern really is about the http semantic conventions that changed, and trying to be consistent with other instrumentations and help end users. For example, look at this trace that contains both http and undici instrumentation. The http instrumentation uses the older semantic conventions, and the undici uses the newer. This means the same fields have different attribute names, which can be difficult for folks to deal with and compare in their backend. For the attributes that are the same, can we use the old ones and/or introduce the environment variable for http/dup to let folks get both?

Aaah, I see. I completely agree that the inconsistency is a major headache. However, I it's in accordance with the semantic conventions for a new instrumentation to only emit new attributes. If we start requiring new instrumentations to also do that, we'll be burning a lot of our time doing so.

I think the better way forward to solve the issue would be to actually update the HTTP instrumentation to allow dual-emitting of old and new attributes as defined here.

[david-luna]

I'd volunteer on update the HTTP instrumentation. I wonder if undici instrumentation should be blocked until HTTP has the dual mode

[pichlermarc]

@david-luna IMO it should not block this PR as it does not make the package part of the auto-instrumentation package. Users already explicitly opt-in to the new semconv based telemetry by installing and using the undici-instrumentation directly.

[GraemeF]

Seems a little unclear - how about This means the full response body is received within the span if that's correct, or This means the full response body has been received when the instrumentation ends the span

(typo in reponse as well)

[david-luna]

thanks for your feedback :) I'm inclined to replace it with the 2nd option you gave

[pichlermarc]

A final round of nits; thanks for working on this. 🙂

[trentm]

LGTM. Nice work.

[pckilgore]

Thanks to everyone who put time into this. Can't wait to let it rip.

[Lp-Francois]

Thanks so much for this feature @david-luna 🙏 🥳

Just tested it with our micro-services and it works great 😍