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: emit diagnostics_channel events upon routing request #5252
base: main
Are you sure you want to change the base?
Conversation
/cc @Qard |
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.
Left a few suggestions. Basically try/finally to more reliably ensure end events happen. Should probably also just put a catch in there too to be sure any unexpected errors get reported too.
lib/handleRequest.js
Outdated
reply[kReplyIsError] = true | ||
reply.send(err) | ||
channels.end.publish(store) |
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.
Rather than putting the end
in all the branches you could use try/finally. That also ensures the event still fires if something throws.
lib/wrapThenable.js
Outdated
@@ -28,6 +33,8 @@ function wrapThenable (thenable, reply) { | |||
reply.send(err) | |||
} | |||
} | |||
|
|||
if (channels) channels.asyncEnd.publish(store) |
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.
Should do try/finally here too.
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.
given dc-polyfill, I think we can land this on main if it does not introduce overhead when disabled.
fastify.js
Outdated
} catch (e) { | ||
// This only happens if `diagnostics_channel` isn't available, i.e. earlier | ||
// versions of Node.js. In that event, we don't care, so ignore the error. | ||
const dc = require('dc-polyfill') |
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.
move this to the top of the file
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.
Also, I'd rather it be named at least diagnostics
instead of dc
in order to avoid having to lookup what "dc" means.
inside |
What about the diagnostics_channel plugin? |
Clarifying my last comment. How does this PR relate to https://github.com/fastify/fastify-diagnostics-channel ? Seems like this PR makes the plugin partially redundant? |
Definitely overlaps with the plugin, though this is more limited to just producing a trace around a request whereas the plugin gives a bit more control. This might benefit from a few more events to cover the other use cases provided by the plugin. The main benefit I see for this existing rather than just the plugin is that if it lives directly in the library it enables automatic instrumentation tracers to listen without producing side effects while adding the plugin could have user-visible behaviour. There's also probably performance advantages to going direct rather than going through the plugin. |
I think instrumentation hooks such as these should be in core. They provide a way for other instrumentations to do things via a specific plugin or wrapping package. If we were trying to add such hooks via some third party library, like dtrace, then my opinion would be different. But the API used in this PR is meant to be a core API. |
@Uzlopak I think you're asking about the removal of the If so, the If anyone is curious please checkout the dc-polyfill docs for more info. Ultimately |
It is a core API. The dc-polyfill API just enables supporting the newer features beyond the limited backporting window of Node.js core. With dc-polyfill we had it tested and working all the way back to Node.js 12. It uses the core API where available though, so it should always be a drop-in replacement for using the core API as if using the current version of Node.js. As for the plugin versus embedding in the library, I much favour embedding in the library as the diagnostics_channel API (and dc-polyfill on top of it) are aimed at very high performance and as close to zero cost as possible when nothing is listening. Abstracting that with a plugin in the middle adds a bunch of unnecessary overhead as fastify then loses the context of what is ignorable as the plugin barrier isn't designed to forward that information. I created diagnostics_channel and it is quite specifically my intent that it be used directly in libraries in this way as it gets us as close to the source as possible feed of diagnostics data, eliminating a ton of instrumentation cost in the process and making gathering instrumentation insights much, much easier in the process. The existing model of monkey-patching everything is highly unreliable and very unapproachable to an average developer to be able to use as a way to gain insight about their applications. I intended for diagnostics_channel to encourage library developers to start thinking about exposing diagnostics data directly and thinking about the performance profile of their code more. |
@mcollina I'm now grabbing the payload within That said I'm not sure if there are expectations on how long |
I wonder if we should integrate then the whole diagnostics-channel stuff from the plugin into core. |
lib/reply.js
Outdated
@@ -66,6 +67,7 @@ function Reply (res, request, log) { | |||
this[kReplyTrailers] = null | |||
this[kReplyHasStatusCode] = false | |||
this[kReplyStartTime] = undefined | |||
this[kReplyPayload] = undefined |
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.
Only concern is when the payload is stream
.
How can the subscriber safely consume the stream
for logging?
The same problem exist when I implement the Trailer
feature and it is not resolved until now.
Reference #4373
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.
I can see so many ways this can end badly long term. Can you avoid storing the payload inside reply?
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.
The latest commit no longer attaches the payload value to the Reply
instance: 325ba54
I think you can target main for this. |
Agreed |
4b3a0cb
to
1b00702
Compare
lib/handleRequest.js
Outdated
@@ -158,7 +163,6 @@ function preHandlerCallback (err, request, reply) { | |||
if (result !== null && typeof result.then === 'function') { | |||
wrapThenable(result, reply, channels, store) | |||
} else { | |||
store.result = result |
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.
This line becomes redundant as the kReplyOnSend
callback sets the value.
This PR is now based on main. Ended up squashing due to conflicts. |
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.
Good work! I think a few edge cases are missing:
-
reply.callNotFound()
// this will execute the not found handler - Fastify error handlers (nested)
- routes with
setImmediate(() => reply.send('hello world))
- async routes with
setImmediate(() => reply.send('hello world)); return reply
The reason why I'm asking is that it will definitely pass through reply.send()
multiple times, and both synchronously and asynchronously.
lib/wrapThenable.js
Outdated
@@ -5,28 +5,37 @@ const { | |||
kReplyHijacked | |||
} = require('./symbols') | |||
|
|||
function wrapThenable (thenable, reply) { | |||
function wrapThenable (thenable, reply, channels, store) { |
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.
I don't think this function should be modified, I would recommend to keep the change between preHandlerCallback
and reply.send()
.
Specifically, I would emit asyncStart
and asyncEnd
within reply.send()
, as there are multiple ways to call that function (synchronous and asynchronous)
@mcollina So far it seems that by returning a simple value from a sync request handler that value is used as the response. Or, if a request handler returns a promise, the resolved value from that promise is used as the response. Or, if at any time |
Yes. The error flow resets all this for the error handler. |
Should this PR also integrate the plugins behaviour? Or should this be a follow up? |
Can you clarify? |
@mcollina I think @Uzlopak is asking if I should copy all of the code from the existing |
A follow up would be good, this would be massive already after the fixes I mentioned. |
@mcollina Just so I know if I'm on the right track, do you think I need to refactor a bunch of Fastify to achieve those changes? |
No, but possibly change the approach you have taken with this PR. |
325ba54
to
0ffca42
Compare
a181582
to
8e1b54b
Compare
@mcollina I modified the code to check for the presence of listeners on any of the DC channels. If there aren't any listeners then the code skips the store object creation paths and also skips the main
tlhunter/diagnostics-channel
|
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.
LGTM, just some minor nits, but feel free to ignore them. 😄
lib/handleRequest.js
Outdated
|
||
if (result !== undefined) { | ||
if (result !== null && typeof result.then === 'function') { | ||
if (store) store.async = true |
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.
Might as well put this inside wrapThenable
if you're passing in store
already anyway. 🤔
lib/wrapThenable.js
Outdated
// try-catch allow to re-throw error in error handler for async handler | ||
try { | ||
reply.send(err) | ||
} catch (err) { | ||
reply.send(err) | ||
} |
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.
Could probably just raise this try/catch into a merge with the outer try/finally.
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.
lgtm
Can you run a final bench? |
@mcollina I ran it a couple times and the results are always about the same:
|
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.
Looks good to me. Just a note for upstream API design.
What i really dont understand is that according to @Qard the publish function is a noop when no subscriber exist. So then why do we even have to check for hasSubscribers? |
The hasSubscribers helper is for the publisher to be able to skip message construction if they are creating a new object for the message. If they are just passing an existing object it's not necessary, but here we are creating a new object. |
It was to address Matteo's object creation concern: #5252 (comment) |
FYI, I'll probably have @Qard address any follow up concerns with this PR. |
Can we get the helper function added to |
The PR for it is here: DataDog/dc-polyfill#10. Needs some work yet as tests were failing on some versions though. Haven't yet identified why as I've been busy with other things for a bit. 😅 |
Any updates on this? It would be good to land it for v5. |
In v5, our minimum supported version will be latest v18. |
I'm back from parental leave and will be working on landing this PR again. It looks like @Qard was working on landing a change in dc-polyfill but hit a roadblock. I'll look at his PR and see if I can help out there. Other than that it looks like things are good to go. I did just do a rebase on main and it looks like there are some failures about package.version mismatching fastify's version, not sure if that's related to this change though. |
8d204d6
to
69a421d
Compare
I had a few questions for @Qard but he just went on vacation so I'll try to figure things out without him. Here's a paper trail of what's going on:
If I can merge the dc-polyfill PR today then I can update this Fastify PR and AFAICT it'll be ready to land. |
@mcollina @jsumners, The backported functionality has landed in dc-polyfill as of v0.1.5 and this PR has been updated to make use of the new helper method. I believe this PR is good to go! Here's an updated benchmark: This PR
Main
|
646b799
to
c5d68d4
Compare
This adds support for emitting diagnostics channel events for a request. It's an extension of #5105.
The
dc-polyfill
package provides a polyfill of thediagnostics_channel
module for older versions of Node.js. This is why the existing diagnostics channel call has been removed from a conditional infastify.js
. Note thatdc-polyfill
falls back to the existing diagnostics channel if present. That's why the tests are requiring diagnostics channel directly instead of the polyfill to show that it works as expected.Here's some things I'm wondering about:
reply.send()
Benchmark results
main
this change
Checklist
npm run test
andnpm run benchmark
and the Code of conduct