add diagnostics_channel publish at initialization time #3279

bengl · 2021-08-24T04:06:05Z

Adds a diagnostics_channel publish at ~~load~~ initialization time. This is sufficient for APM purposes, since access to the fastify object is all we need in order to add hooks with addHook. This change enables us to instrument fastify without any of the following:

User involvement. (Apart from the usual with APM tools.)
Monkey-patching.
Intercepting loading. (:tada: :partying_face:)

Since diagnostics_channel is not always available (e.g. on older versions of Node.js), this is wrapped in a try-catch, but I'm happy to switch to another method of feature detection (or pulling in the polyfill) if desired.

Checklist

run npm run test ~~and npm run benchmark~~
tests ~~and/or benchmarks~~ are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

climba03003 · 2021-08-24T04:19:50Z

I do not think it is the right approach, you are passing the fastify prototype to diagnostics channel. Are you sure you need the prototype instead of instance? If yes, I think document and example for this usage is needed.

simon-id · 2021-08-24T07:57:18Z

fastify.js

+
+try {
+  const dc = require('diagnostics_channel')
+  dc.channel('fastify:loaded').publish(fastify)


Since https://github.com/fastify/fastify-diagnostics-channel already exists, we should at least use the same syntax, ie: fastify.loaded.

Yeah I'm fine with using a . here.

I would prefer if we checked with hasSubscribers.

mcollina · 2021-08-24T08:04:09Z

I think we might aim to integrate fastify-diagnostic-channel in fastify proper for next / v4. Wdyt @bengl ?

fastify.js

zekth

I'm -1 on this as diagnostics_channel are only available in node 15 and 16 this would slow down the startup time of any instance. In the case of serverless this is quite a change. Is there a reason why not to use https://github.com/fastify/fastify-diagnostics-channel ? This plugin might just need an addition for the onReady hook : https://www.fastify.io/docs/latest/Hooks/#onready

simon-id · 2021-08-24T12:37:51Z

This plugin might just need an addition for the onReady hook

This would not fit the use case as [the onReady hook] cannot change the routes or add new hooks.

zekth · 2021-08-24T12:54:18Z

This plugin might just need an addition for the onReady hook

This would not fit the use case as [the onReady hook] cannot change the routes or add new hooks.

Fair point. One solution would be to use semver check on process.version but this might lead to issues with some environments. I don't think there is a pretty solution to fit with all node versions. Like @climba03003 said if prototype is needed rather than instance this is the way to implement it.

After checking the https://nodejs.org/en/about/releases/ schedule, 16 will be active in few months. Landing this in semver minor and integrating https://github.com/fastify/fastify-diagnostics-channel in v4 is a good plan to me.

jsumners · 2021-08-24T12:59:53Z

Is there a reason why not to use fastify/fastify-diagnostics-channel ?

For reference, this is as much discussion as we have had about the specific API -- #2697

I am of two opinions:

This should be in core
This should be in core after it is stable

I can be convinced of opinion 1. We have a non-trivial amount of clout/interaction with Node core. So even if the API changes, we'd at least have a heads up. However, I suspect the registration API is unlikely to change, and this PR only makes use of that part of the API.

If the, I am certain, negligible start up impact is really such a concern, we can do feature detection in a different manner. @bengl has already stated he is up for such a change. Frankly, it can be as simple as:

const nodeMajor = /v(\d+)\..+/.exec(process.version)[1]
if (parseInt(nodeMajor, 10) >= 15) {
  // do it
}

Or if the regex is still too expensive:

const nodeMajor = parseInt(process.version.slice(1).split('.').shift(), 10)
if (nodeMajor >= 15) {
  // do it
}

bengl · 2021-08-24T14:28:04Z

@climba03003 Yes, I think you're right and this should be done perhaps at the end of the constructor. I'll make that change.

@mcollina

I think we might aim to integrate fastify-diagnostic-channel in fastify proper for next / v4. Wdyt @bengl ?

The thing is, fastify already has a pretty good system for hooks at lifecycle events, and adding a diagnostics_channel publish at each of those events would add a second layer of dispatch on top of that, which seems like unnecessary overhead. That's why fastify-diagnostic-channel isn't included in that PR, and it's why this PR opts for just a single diagnostics_channel hook that we can use to install fastify hooks. That being said, a rewrite of fastify hooks based on diagnostics_channel could be beneficial for everyone, since it would be less dispatching code in fastify, and more direct access to the lifecycle events for APMs.

@jsumners @zekth You both mentioned lack of support in earlier Node.js versions. Note that a polyfill exists, and we intend to include it as a dependency of our APM library. For that reason, I'd prefer not doing a Node.js version check.

jsumners · 2021-08-24T14:29:46Z

@jsumners @zekth You both mentioned lack of support in earlier Node.js versions. Note that a polyfill exists, and we intend to include it as a dependency of our APM library. For that reason, I'd prefer not doing a Node.js version check.

To be clear, I am not concerned about the start up time impact.

Eomm · 2021-08-24T14:41:44Z

I understand this change walk to a "near zero configuration" installation for users, but I would like to propose something like:

const dc = require('diagnostics_channel')
const fastify = require('fastify')({ 
  logger: true,
  tracing: {
      loaded: dc.channel('whatever')
  }
})

The in fastify core we could just write

options.tracing?.loaded?..publish(fastify)

Is this overengenieering?
This could be improved in future to trace some internal components (such as schema compiling)

RafaelGSS · 2021-08-24T15:32:42Z

I understand this change walk to a "near zero configuration" installation for users, but I would like to propose something like:
const dc = require('diagnostics_channel')
const fastify = require('fastify')({ 
  logger: true,
  tracing: {
      loaded: dc.channel('whatever')
  }
})
The in fastify core we could just write
options.tracing?.loaded?..publish(fastify)
Is this overengenieering?
This could be improved in future to trace some internal components (such as schema compiling)

I think that it hit the same problem for APM vendors that require a user action to run the tracing, if we follow this line the vendors could create a plugin for fastify that register fastify-diagnostics-channel under the hood, so I don't think that's doable.

I'm +1 in adding diagnostics_channel in fastify core. When the PR is ready we can just run our startup benchmarks to identify before land.

And I also would like to see a usage example as @climba03003 mentioned.

mcollina · 2021-08-24T15:54:54Z

I'm -1 on this as diagnostics_channel are only available in node 15 and 16 this would slow down the startup time of any instance.

@zekth could you measure this?

mcollina · 2021-08-24T15:56:24Z

I'm generically +1 with this approach, anything that makes the APM vendors do less monkeypatching is a good a thing for me.

zekth · 2021-08-24T16:20:49Z

I'm -1 on this as diagnostics_channel are only available in node 15 and 16 this would slow down the startup time of any instance.

@zekth could you measure this?

I can do something. But as i stated in my comment (not clear i confess): #3279 (comment)

I'm +1 on this.

mcollina · 2021-08-25T13:35:19Z

@jsumners I don't think diagnostics_channel will get stabilized before it is validated by APMs... which is part of what this PR is about. I don't think we will ever receive significant breakage from it - the API is simple and stable.

@bengl could you update this PR so we can land it?

bengl · 2021-08-25T13:41:14Z

@mcollina Yes, hopefully I will have some time today. 😅

jsumners · 2021-08-25T14:24:56Z

@jsumners I don't think diagnostics_channel will get stabilized before it is validated by APMs... which is part of what this PR is about. I don't think we will ever receive significant breakage from it - the API is simple and stable.

I understand. I'm not blocking this.

RafaelGSS

Could you add a note in our docs with this new support out-of-box?

bengl · 2021-08-26T03:02:43Z

Updated the PR taking into account most of the recommendations, and added a quick bit of documentation with a contrived example/use-case.

Happy to change the feature detection (or whatever else) if that's necessary to land, but as stated before, I'd like to steer clear from using Node.js versions, to make sure the polyfill can be used. Other than that, this should be ready to land, unless there are further concerns.

test/diagnostics-channel.test.js

docs/Hooks.md

Eomm

add suggestions for consistency

Eomm · 2021-08-26T07:14:14Z

docs/Hooks.md

+
+Currently, one
+[`diagnostics_channel`](https://nodejs.org/api/diagnostics_channel.html) publish
+event, `'fastify:initialized'`, happens at initialization time. The Fastify


Suggested change

event, `'fastify:initialized'`, happens at initialization time. The Fastify

event, `'fastify.initialized'`, happens at initialization time. The Fastify

Eomm · 2021-08-26T07:14:25Z

docs/Hooks.md

+```js
+const tracer = /* retrieved from elsehwere in the package */
+const dc = require('diagnostics_channel')
+const channel = dc.channel('fastify:initialized')


Suggested change

const channel = dc.channel('fastify:initialized')

const channel = dc.channel('fastify.initialized')

fastify.js

test/diagnostics-channel.test.js

docs/Hooks.md

bengl · 2021-08-26T12:30:06Z

Added the changes requested by @mcollina and @Eomm.

Are folks happy with the name of the channel? Currently it's fastify.initialized, but other options could be:

fastify.onInitialized
fastify.init
fastify.onInit
all lower-case version of any of the above.
any of the above, but s/./:/

Keep in mind that there may be additional channels in the future.

mcollina

lgtm

jsumners · 2021-08-26T13:22:18Z

Added the changes requested by @mcollina and @Eomm.

Are folks happy with the name of the channel? Currently it's fastify.initialized, but other options could be:

fastify.onInitialized

fastify.init

fastify.onInit

all lower-case version of any of the above.

any of the above, but s/./:/

Keep in mind that there may be additional channels in the future.

Given that this registration happens at the onset of initialization, I think it better to name it fastify.initialization. I can see why "initialized" would also be acceptable as a reference to the core object having been initialized from the passed in options. But the real initialization, e.g. defining routes and hooks (which the APM is really concerned about), is yet to happen.

Eomm

LGTM

Ci is failing due to the coverage regression

We could add a test with proxyquire to simulate the diagnostic_channel a reach the 100%

zekth

Minor typo in here. otherwise looks good!

zekth · 2021-08-26T19:09:21Z

docs/Hooks.md

+channel.subscribe(function ({ fastify }) {
+  fastify.addHook('onRequest', (request, reply, done) => {
+    const span = tracer.startSpan('fastify.request')
+    spans.set(reqest, span)


Suggested change

spans.set(reqest, span)

spans.set(request, span)

bengl · 2021-08-27T03:17:12Z

@jsumners name is now fastify.initialization.

@Eomm test now uses proxyquire to cover all the code paths, regardless of Node.js version.

@zekth type is now fixed.

docs/Hooks.md

Co-authored-by: James Sumners <james@sumners.email>

RafaelGSS

LGTM.

github-actions · 2022-08-28T00:27:46Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

climba03003 requested a review from mcollina August 24, 2021 04:20

simon-id reviewed Aug 24, 2021

View reviewed changes

jsumners reviewed Aug 24, 2021

View reviewed changes

fastify.js Outdated Show resolved Hide resolved

zekth requested changes Aug 24, 2021

View reviewed changes

RafaelGSS reviewed Aug 25, 2021

View reviewed changes

bengl force-pushed the bengl/add-dc branch from 4aa6a75 to d460b26 Compare August 26, 2021 02:20

bengl changed the title ~~add diagnostics_channel publish at load time~~ add diagnostics_channel publish at initialization time Aug 26, 2021

add diagnostics_channel publish at initializaiton time

1c887a9

bengl force-pushed the bengl/add-dc branch from d460b26 to 1c887a9 Compare August 26, 2021 02:29

docs for diagnostic channel hook

50e2726

mcollina reviewed Aug 26, 2021

View reviewed changes

test/diagnostics-channel.test.js Outdated Show resolved Hide resolved

docs/Hooks.md Show resolved Hide resolved

Eomm reviewed Aug 26, 2021

View reviewed changes

PR review

1892aa9

mcollina approved these changes Aug 26, 2021

View reviewed changes

Eomm reviewed Aug 26, 2021

View reviewed changes

zekth requested changes Aug 26, 2021

View reviewed changes

more PR review

2e21fff

bengl force-pushed the bengl/add-dc branch from e4e4093 to 2e21fff Compare August 27, 2021 03:15

jsumners approved these changes Aug 27, 2021

View reviewed changes

docs/Hooks.md Outdated Show resolved Hide resolved

docs/Hooks.md Outdated Show resolved Hide resolved

zekth approved these changes Aug 27, 2021

View reviewed changes

mcollina and others added 2 commits August 27, 2021 12:58

Update docs/Hooks.md

53b4e6a

Co-authored-by: James Sumners <james@sumners.email>

Update docs/Hooks.md

6d9d2b9

Co-authored-by: James Sumners <james@sumners.email>

RafaelGSS approved these changes Aug 27, 2021

View reviewed changes

mcollina merged commit a04f4c9 into fastify:main Aug 27, 2021

fralonra mentioned this pull request Oct 4, 2021

Update: 92e4e19...c872f0b fastify/docs-chinese#250

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2022

	event, `'fastify:initialized'`, happens at initialization time. The Fastify
	event, `'fastify.initialized'`, happens at initialization time. The Fastify

	const channel = dc.channel('fastify:initialized')
	const channel = dc.channel('fastify.initialized')

add diagnostics_channel publish at initialization time #3279

add diagnostics_channel publish at initialization time #3279

Conversation

bengl commented Aug 24, 2021 • edited

Checklist

climba03003 commented Aug 24, 2021 • edited

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

mcollina commented Aug 24, 2021

zekth left a comment

Choose a reason for hiding this comment

simon-id commented Aug 24, 2021

zekth commented Aug 24, 2021

jsumners commented Aug 24, 2021

bengl commented Aug 24, 2021

jsumners commented Aug 24, 2021

Eomm commented Aug 24, 2021

RafaelGSS commented Aug 24, 2021

mcollina commented Aug 24, 2021

mcollina commented Aug 24, 2021

zekth commented Aug 24, 2021

mcollina commented Aug 25, 2021

bengl commented Aug 25, 2021

jsumners commented Aug 25, 2021

RafaelGSS left a comment

Choose a reason for hiding this comment

bengl commented Aug 26, 2021

Eomm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bengl commented Aug 26, 2021

mcollina left a comment

Choose a reason for hiding this comment

jsumners commented Aug 26, 2021

Eomm left a comment

Choose a reason for hiding this comment

zekth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bengl commented Aug 27, 2021

RafaelGSS left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 28, 2022

bengl commented Aug 24, 2021 •

edited

climba03003 commented Aug 24, 2021 •

edited