Add diagnostic channels #4429

YoannMa · 2023-03-02T14:45:59Z

Reimplementation of what @AdriVanHoudt made on the previous PR #4278

What changed since ?

Node 12 is no longer supported, there is no longer the issue of requiring a polyfill.
The API diagnostics_channel is stable since Node 16, The only part of the API we use, is identical in Node 14 but marked as experimental. Node 14 being EOL in less than 2 months, I don't really see a big issue there.

Following what Fastify does on their side https://github.com/fastify/fastify-diagnostics-channel, I added 3 more channels for the route, request and response.

I also added documentation of the exposed channels.

Questions :

Should we add more channels (onTimeout,onError) ?
Naming convention of the channel ?
I copied Fastify naming scheme, but I don't really have any strong opinions regarding the naming scheme we pick : hapi:server, hapi.onServer or hapi.server

Cc'ing @Qard and @vdeturckheim as they are probably interested in the outcome of this PR.

…response sent

cjihrig · 2023-03-02T17:54:58Z

API.md

+
+## Diagnostic Channels
+
+see : https://nodejs.org/docs/latest-v16.x/api/diagnostics_channel.html


Instead of linking to the Node docs, can you provide a small example. If you still want to link to the Node docs, I would recommend linking to latest so we don't have to keep updating the link.

Fixed the URL, added a bit of explaining and examples, do you feel like it's enough ?

I'm good with it. Let's give other folks a chance to weigh in.

test/channels.js

Co-authored-by: Colin Ihrig <[email protected]>

…c_channels

Qard · 2023-03-03T01:59:29Z

It's been awhile since I've used Hapi so I'll have to take another look at it when I have time to know for sure if this satisfies what APMs need. One thing that does seem to be missing is we'd like an event to track errors.

@rochdev Any other suggestions for events that could help us?

rochdev · 2023-03-09T16:14:40Z

@Qard As you mentioned, definitely an event for errors. I would say also an event for when the route is entered as opposed to just created, unless the route is already available on the request object by the time it's emitted. Events for when plugins/extensions are entered/exited would also be useful, although this could be out of scope of this PR.

YoannMa · 2023-03-09T17:18:33Z

Route is available on the request object (see https://github.com/hapijs/hapi/blob/master/API.md#request.route) so I believe the hook on route entered is not necessary.

Qard · 2023-03-09T21:52:35Z

I should also say, channel naming doesn't really matter as long as the chosen name is documented, and it includes the module name somehow to avoid possible collisions with other modules as diagnostics_channel is a global namespace.

It's also worth mentioning that I have an in-progress API to make the tracing use case easier with diagnostics_channel via nodejs/node#44943. While the API itself will not be available in all the supported versions of hapi for awhile yet, it does document the behaviour as a pattern which can be implemented manually with channels now, so long as you follow the guidelines of how it should behave. It could be worth a look to understand what things we capture and how that could be translated to channels introduced for hapi.

kanongil

Looks reasonable.

My main concern is that a throw in a registered channel handler can propagate to hapi. It seems that the current version guards against this, but a test to verify would be nice.

kanongil · 2023-03-10T11:49:23Z

lib/core.js

@@ -510,6 +511,8 @@ exports = module.exports = internals.Core = class {

            const request = Request.generate(this.root, req, res, options);

+            Channels.onRequestChannel.publish(request);


I would probably move the request._execute() info assignment to the request constructor to make this more representative of what the onRequest ext sees.

Or maybe move the publish to the _execute() method? (though that would mean it doesn't see the ones that are denied due to load).

If this assignment is in the request._execute(), perhaps it is to avoid executing it in case of heavy load ?

I would vote to keep the publish() here given we need another event when a request is routed. I could add an event early in the request._lifecycle() method ? Something like hapi.onRequestLifecycle ?

lib/channels.js

test/channels.js

API.md

kanongil · 2023-03-10T12:33:43Z

API.md

+const channel = DC.channel('hapi.onServer');
+
+channel.subscribe((server) => {
+    // Do something with the server
+    console.log(server.version);
+});


Hapi itself doesn't actually use any deprecated API, only these examples and the tests. I'm not a fan of using deprecated API to illustrate how to use, but since this is targeted at APM vendors they should known what they are doing anyway.

Maybe just have a single example at the top?

I don't mind doing another PR when Hapi drops support for Node 14 to replace those deprecated usage

kanongil · 2023-03-10T13:16:01Z

Route is available on the request object (see https://github.com/hapijs/hapi/blob/master/API.md#request.route) so I believe the hook on route entered is not necessary.

request.route won't be available yet when onRequest is triggered. The earliest it can be determined using public API, is with a onPreAuth hook, and it would have to contend with other hooks and not trigger when cookie processing fails.

kanongil · 2023-03-10T13:23:37Z

Regarding channels, I would advocate on limiting it until there is a concrete need. Eg. with just the onServer channel, APM's should be able to hook something that matches hapi.onRoute, hapi.onRequest, and hapi.onResponse.

Exposing just a hapi.onServer channel would also mean that there is zero runtime overhead when channels are not used.

Hapi already contains most of the hooks a basic APM need, provided they get the Server object before the user can call methods on it. I don't see why we need to make it any easier for them.

Qard · 2023-03-10T22:02:17Z

My main concern is that a throw in a registered channel handler can propagate to hapi. It seems that the current version guards against this, but a test to verify would be nice.

The diagnostics_channel API has guarded against this since the beginning. It's not possible for a throw in a subscriber to ever impact the publisher.

Exposing just a hapi.onServer channel would also mean that there is zero runtime overhead when channels are not used.

It's specifically designed to have zero overhead when a channel is not used. The publish function is a no-op until the channel has subscribers, and if it ever returns to zero subscribers it becomes a no-op again.

kanongil

Looking at it again I think it would work better if the channels are triggered before the equivalent user-facing hook. This way the event handlers cannot modify the object before the APM sees it, making it a more "pure" signal.

Test to verify this would also be nice.

YoannMa · 2023-03-19T17:38:18Z

Added both hapi.onRequestLifecycle and hapi.onError channels.

I'm not really sure about the name hapi.onRequestLifecycle, if you have some ideas for another name, I'm listening.

kanongil · 2023-03-21T14:38:08Z

I would much prefer that this PR does not add any more channel hooks. Ideally each hook gets a PR to allow simpler reviews. Eg. i have comments regarding hapi.onError, but I don't want to complicate the review further.

Personally, I would just include the bare minimum (ie. hapi.onServer) for an initial PR. As stated in #4429 (comment), it should be enough to get started. For instance, looking at the elastic-apm-node APM it is the only hook it will need. The remaining hooks are handled using the public API.

Qard · 2023-03-21T16:58:03Z

There's typically greater overhead to needing to hook into some public API like that though, and a greater risk of it changing between versions. APMs would generally prefer that data be published directly. While having only the hapi.onServer event is better than nothing, it's not actually what we want and we wouldn't usually even need the hapi.onServer event if we have the other events.

feat: add diagnostic channel for server, route, request creation and …

6680441

…response sent

cjihrig reviewed Mar 2, 2023

View reviewed changes

YoannMa and others added 3 commits March 2, 2023 20:51

style: Apply suggestions from code review

2606bb5

Co-authored-by: Colin Ihrig <[email protected]>

style: remove empty line

890062d

doc: update documentation to add exemple and description to diagnosti…

e046e26

…c_channels

YoannMa marked this pull request as ready for review March 2, 2023 20:02

kanongil reviewed Mar 10, 2023

View reviewed changes

fix: apply change asked in review

cf9c82d

kanongil reviewed Mar 12, 2023

View reviewed changes

YoannMa added 4 commits March 15, 2023 13:43

fix: move channels event before user-facing hooks

5eb9907

test: add test insuring request is not routed on hapi.onRequest

35ea6ef

feat: add hapi.onRequestLifecycle and hapi.onError diagnostic channels

f21651d

doc: change hapi.onError channel description

a246101

style: missing semi

34f3f35

kanongil added the feature New functionality or improvement label Nov 6, 2023

kanongil changed the title ~~feat: add diagnostic channel for server, route, request creation and response sent~~ Add diagnostic channels Nov 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add diagnostic channels #4429

Add diagnostic channels #4429

YoannMa commented Mar 2, 2023

cjihrig Mar 2, 2023

YoannMa Mar 2, 2023

cjihrig Mar 2, 2023

Qard commented Mar 3, 2023

rochdev commented Mar 9, 2023

YoannMa commented Mar 9, 2023 •

edited

Loading

Qard commented Mar 9, 2023

kanongil left a comment

kanongil Mar 10, 2023

YoannMa Mar 15, 2023

kanongil Mar 10, 2023

YoannMa Mar 19, 2023

kanongil commented Mar 10, 2023 •

edited

Loading

kanongil commented Mar 10, 2023 •

edited

Loading

Qard commented Mar 10, 2023 •

edited

Loading

kanongil left a comment •

edited

Loading

YoannMa commented Mar 19, 2023

kanongil commented Mar 21, 2023 •

edited

Loading

Qard commented Mar 21, 2023


		## Diagnostic Channels

		see : https://nodejs.org/docs/latest-v16.x/api/diagnostics_channel.html

		@@ -510,6 +511,8 @@ exports = module.exports = internals.Core = class {

		const request = Request.generate(this.root, req, res, options);

		Channels.onRequestChannel.publish(request);

Add diagnostic channels #4429

Are you sure you want to change the base?

Add diagnostic channels #4429

Conversation

YoannMa commented Mar 2, 2023

cjihrig Mar 2, 2023

Choose a reason for hiding this comment

YoannMa Mar 2, 2023

Choose a reason for hiding this comment

cjihrig Mar 2, 2023

Choose a reason for hiding this comment

Qard commented Mar 3, 2023

rochdev commented Mar 9, 2023

YoannMa commented Mar 9, 2023 • edited Loading

Qard commented Mar 9, 2023

kanongil left a comment

Choose a reason for hiding this comment

kanongil Mar 10, 2023

Choose a reason for hiding this comment

YoannMa Mar 15, 2023

Choose a reason for hiding this comment

kanongil Mar 10, 2023

Choose a reason for hiding this comment

YoannMa Mar 19, 2023

Choose a reason for hiding this comment

kanongil commented Mar 10, 2023 • edited Loading

kanongil commented Mar 10, 2023 • edited Loading

Qard commented Mar 10, 2023 • edited Loading

kanongil left a comment • edited Loading

Choose a reason for hiding this comment

YoannMa commented Mar 19, 2023

kanongil commented Mar 21, 2023 • edited Loading

Qard commented Mar 21, 2023

YoannMa commented Mar 9, 2023 •

edited

Loading

kanongil commented Mar 10, 2023 •

edited

Loading

kanongil commented Mar 10, 2023 •

edited

Loading

Qard commented Mar 10, 2023 •

edited

Loading

kanongil left a comment •

edited

Loading

kanongil commented Mar 21, 2023 •

edited

Loading