Undici prototype #921

bizob2828 · 2021-09-23T17:12:44Z

Proposed Release Notes

Added instrumentation for the undici http client. The instrumentation is behind a feature flag. To enable, add the following into your config { feature_flag: { undici_instrumentation: true } }. The support for undici client is Node.js 16.x as it takes advantage of the diagnostics_channel. Lastly, you must be using v4.7.0+ of the undici client for any of the instrumentation to work.

Links

Closes #730

Details

You can use this repo undici-http-app and follow the README to get it up and running. As you can see the spans for httpbin.org are identical and the traces are properly nested.

Http trace

Undici trace

7/133294611-c0c14ccf-e9d1-44e4-91d0-4367fb22fa89.png">

…ture parity with the existing http-outbound instrumentation. Wrapped code behind feature_flag

michaelgoin

Awesome. Thanks for getting this initial prototype all fleshed out.

It is a bummer some of the nesting varies (connection chain and setTimeout not properly nested). Looking through things, I'm wondering if we could potentially pull off getting that correct too. Might take slightly more plumbing that the parent/current active segments but seems potentially feasible. Or if not, perhaps we can leverage some naming to help. That way you understand which request had the slow connection, etc.

Left some general feedback and conversational comments but I expect we can discuss/add issues/do follow-up PRs to address things separate from this PR. The proto doesn't need to land perfect.

That being said, might want to do some of the usages of shim instead of agent.tracer directly. Ideally, we get to where our instrumentation only uses the (or a) instrumentation (shim) API.

lib/instrumentation/undici.js

michaelgoin · 2021-09-28T17:03:09Z

lib/instrumentation/undici.js

+    const segment = agent.tracer.createSegment(
+      name,
+      recordExternal(url.host, 'undici'),
+      request[SYMBOLS.PARENT_SEGMENT]
+    )


Suggested change

const segment = agent.tracer.createSegment(

name,

recordExternal(url.host, 'undici'),

request[SYMBOLS.PARENT_SEGMENT]

)

const segment = shim.createSegment(

name,

recordExternal(url.host, 'undici'),

request[SYMBOLS.PARENT_SEGMENT]

)

this was copied from http-outbound. they are accessing the same thing right? i can def update but why does http-outbound access it via agent.tracer?

Because http-outbound has not yet been converted to use the shim API. Same for the standard http instrumentation.

They are accessing the same thing but ideally, if you go through a proper instrumentation API, some things can change behind the scenes without breaking.

Really ideally, I'd love to get away from just about anything have a handle on the 'agent' itself and potentially the tracer too (other than special cases). The agent is in extreme want of more passing of dependencies vs the agent to do everything.

In the end, this should work fine but I don't love mixing and matching usage of the 'shim' and the 'tracer'. The usage of shimmer and direct usage of 'tracer' pre-date the attempt at a instrumentation api in the shim.

*obviously the shim has many limitations and doesn't seem to support all the needed use cases yet. i'd also like to use apollo and this and maybe fastify as it evolves to build a strong map for how we might evolve the 'shim' api into something that better supports using just what you need and composing functionality. and you know, maybe we actually have something called 'instrumentationApi' or some such and not a 'shim' :P

michaelgoin · 2021-09-28T17:07:01Z

lib/instrumentation/undici.js

+      outboundHeaders[NEWRELIC_SYNTHETICS_HEADER] = transaction.syntheticsHeader
+    }
+
+    if (agent.config.distributed_tracing.enabled) {


If aws-sdk ever supported unidici, we'd likely need to build in a check for SHIM_SYMBOLS.DISABLE_DT but I'm guessing there's a lot that would need to be tweaked at that point.

we won't be able to ever support this use case. the headers are already parsed into a string. we can't save a reference to the symbol and get to it ever. Whereas in the http.request instrumentation use case we're parsing before it converts headers from object to string

We don't have to use a symbol though.

true. we could def add a key/value and then strip in here later. i'll file ticket to address this

michaelgoin · 2021-09-28T17:14:22Z

lib/instrumentation/undici.js

+      urlString += port === 80 ? request.path : `:${port}${request.path}`
+    }
+
+    const url = new URL(urlString)


There is some New Relic business logic applied via deeper method calls in urltils.scrubAndParseParameters(request.path). I'm not sure if this particular parsing happens to hit all of those or not. It is obviously nice to use the more desired mechanism at least up front but we may or may not need to apply more logic.

scrub and parseParameters are the functions doing the manual work. Not saying we need to use this but would like to ensure the business logic is consistent.

yea i opted to not use that because with new URL we get all of this for free now. I can file a ticket to update the urlutils to leverage as much built in Node.js functionality as possible

Great. Just wanted to ensure the functionality was equivalent so we didn't accidentally break something.

lib/instrumentation/undici.js

michaelgoin · 2021-09-28T17:23:20Z

lib/instrumentation/undici.js

+   * @param {Object} params
+   * @param {Function} params.connector function to connect to socket
+   */
+  diagnosticsChannel.channel('undici:client:beforeConnect').subscribe(({ connector }) => {


Part of me wonders if we could create the external here as well to capture proper nesting..

perhaps. the only thing I see we couldn't get is the host if it is http vs https. i can dig a little

this won't work. to end the segment we need access to the request object. all the connection hooks just pass along key/values and the connector. I'm going to keep this as is

sounds good. i do think we'll want something better before going GA, whatever that looks like, but i don't want to derail this PR.

I'm not sure I understand what better would be. We're subscribing to all the necessary hook points to achieve feature parity with our existing http outbound and http.agent logic. Undici has specifically kept the client and request separate. The client is doing the socket connections(undici:client:beforeConnect, undici:client:connected, and undici:client:connectError) and also adding common headers(undici:client:sendHeaders)

Traditionally, our external calls encompass the time to connect AND the request itself.

In your example above, for example, establishing the connection was the slowest part.

In the unidici trace, the connection was detatched from the external span. While not the end of the world, if you make multiple request to different endpoints it is not obvious which things match to which ones. Which endpoint made you slow and that those are related as far as the call chain is concerned.

Our visual representation to users need-not match the underlying implementation.

ah ok. i wasn't thinking in those terms. I guess for every request you'd get a undici.Client.connect but depending one could be slower than the other. I think we should wait to even log tickets here until we get this in the wild and have users give us feedback though

lib/instrumentation/undici.js

test/unit/instrumentation/undici.test.js

…agent.tracer.*

michaelgoin

👍 🎉

bizob2828 · 2021-09-28T19:48:43Z

Awesome. Thanks for getting this initial prototype all fleshed out.

It is a bummer some of the nesting varies (connection chain and setTimeout not properly nested). Looking through things, I'm wondering if we could potentially pull off getting that correct too. Might take slightly more plumbing that the parent/current active segments but seems potentially feasible. Or if not, perhaps we can leverage some naming to help. That way you understand which request had the slow connection, etc.

I honestly think the nesting is now correct. Our nesting previously was due to how we were instrumenting the http module. The way I view it is you try to connect to socket, you're on socket(that's one segment that's done), then you make the http request and get that back. You don't end the connection after the http request is there. On the flipside I could see how both are right-ish and that we just keep as is 😄

bizob2828 added 3 commits September 23, 2021 13:00

Added undici instrumentation via diagnostic channel events to get fea…

b2289e3

…ture parity with the existing http-outbound instrumentation. Wrapped code behind feature_flag

added unit tests for undici work

f2c0e8f

fixes after rebase from main

c2ac099

michaelgoin reviewed Sep 28, 2021

View reviewed changes

fixed trace error when DT/CAT are disabled, updated to use shim.* vs …

5c061cd

…agent.tracer.*

michaelgoin approved these changes Sep 28, 2021

View reviewed changes

bizob2828 merged commit 781ae9e into newrelic:main Sep 28, 2021

bizob2828 mentioned this pull request Sep 28, 2021

Allow undici to understand a header to disable DT #935

Closed

github-actions bot mentioned this pull request Oct 12, 2021

Release v8.5.0 #948

Merged

bizob2828 deleted the undici-prototype branch August 28, 2024 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Undici prototype #921

Undici prototype #921

bizob2828 commented Sep 23, 2021

michaelgoin left a comment

michaelgoin Sep 28, 2021

bizob2828 Sep 28, 2021

michaelgoin Sep 28, 2021

michaelgoin Sep 28, 2021

michaelgoin Sep 28, 2021

bizob2828 Sep 28, 2021

michaelgoin Sep 28, 2021

bizob2828 Sep 28, 2021

michaelgoin Sep 28, 2021

bizob2828 Sep 28, 2021

michaelgoin Sep 28, 2021

michaelgoin Sep 28, 2021

bizob2828 Sep 28, 2021

bizob2828 Sep 28, 2021

michaelgoin Sep 28, 2021

bizob2828 Sep 28, 2021

michaelgoin Sep 28, 2021

bizob2828 Sep 28, 2021

michaelgoin left a comment

bizob2828 commented Sep 28, 2021

Undici prototype #921

Undici prototype #921

Conversation

bizob2828 commented Sep 23, 2021

Proposed Release Notes

Links

Details

michaelgoin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelgoin left a comment

Choose a reason for hiding this comment

bizob2828 commented Sep 28, 2021