-
-
Notifications
You must be signed in to change notification settings - Fork 125
feat: add request layer lifecycle TracingChannels #196
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
base: master
Are you sure you want to change the base?
Changes from 6 commits
95e23ba
858399c
9f1aea7
dea81ae
1f6bc25
1ad1b9c
28b3213
90ed7e2
758030e
9c9b4a7
67c3851
6aa0575
5e80078
da0da14
8fd857f
337e212
232143b
10a47bc
cc5deb7
9aa666a
04b2f72
97da6d8
583c8c4
d0d8f7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |
| * @private | ||
| */ | ||
|
|
||
| const dc = require('node:diagnostics_channel') | ||
| const isPromise = require('is-promise') | ||
| const pathRegexp = require('path-to-regexp') | ||
| const debug = require('debug')('router:layer') | ||
|
|
@@ -25,6 +26,20 @@ const deprecate = require('depd')('router') | |
| const TRAILING_SLASH_REGEXP = /\/+$/ | ||
| const MATCHING_GROUP_REGEXP = /\((?:\?<(.*?)>)?(?!\?)/g | ||
|
|
||
| /** | ||
| * TracingChannel setup. | ||
| * @private | ||
| */ | ||
|
|
||
| const requestChannel = dc.tracingChannel && dc.tracingChannel('express:request') | ||
|
|
||
| /** | ||
| * Check if the channel has subscribers. | ||
| */ | ||
| function shouldTrace (ch) { | ||
| return ch && ch.hasSubscribers !== false | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we support Node 18.0.0, users on those versions will get true, because (undefined !== false) === true. https://nodejs.org/api/diagnostics_channel.html#diagnostics_channeltracingchannelnameorchannels
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep this is a trade off I suppose. I think we should then check for all channels own subscribers in that case. |
||
| } | ||
|
|
||
| /** | ||
| * Expose `Layer`. | ||
| */ | ||
|
|
@@ -111,23 +126,12 @@ Layer.prototype.handleError = function handleError (error, req, res, next) { | |
| return next(error) | ||
| } | ||
|
|
||
| try { | ||
| // invoke function | ||
| const ret = fn(error, req, res, next) | ||
|
|
||
| // wait for returned promise | ||
| if (isPromise(ret)) { | ||
| if (!(ret instanceof Promise)) { | ||
| deprecate('handlers that are Promise-like are deprecated, use a native Promise instead') | ||
| } | ||
|
|
||
| ret.then(null, function (error) { | ||
| next(error || new Error('Rejected promise')) | ||
| }) | ||
| } | ||
| } catch (err) { | ||
| next(err) | ||
| } | ||
| const layer = this | ||
| invokeWithTrace(function () { | ||
| return fn(error, req, res, next) | ||
| }, function () { | ||
| return { req, res, layer } | ||
| }, next) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -147,16 +151,37 @@ Layer.prototype.handleRequest = function handleRequest (req, res, next) { | |
| return next() | ||
| } | ||
|
|
||
| try { | ||
| // invoke function | ||
| const ret = fn(req, res, next) | ||
| // Skip tracing for route dispatch wrappers (this.route is only set on | ||
| // the internal layer that calls route.dispatch). The actual user handlers | ||
| // inside the route are traced individually. | ||
| if (this.route) { | ||
| return invokeWithTrace(function () { | ||
| return fn(req, res, next) | ||
| }, null, next) | ||
| } | ||
|
|
||
| // wait for returned promise | ||
| if (isPromise(ret)) { | ||
| if (!(ret instanceof Promise)) { | ||
| deprecate('handlers that are Promise-like are deprecated, use a native Promise instead') | ||
| } | ||
| const layer = this | ||
| invokeWithTrace(function () { | ||
| return fn(req, res, next) | ||
| }, function () { | ||
| return { req, res, layer } | ||
| }, next) | ||
| } | ||
|
|
||
| /** | ||
| * Invoke a handler function, optionally wrapping it in TracingChannel. | ||
| * The ctxFactory is only called when tracing is active, ensuring zero | ||
| * allocation overhead when no subscribers are registered. | ||
| * @private | ||
| */ | ||
|
|
||
| function invokeWithTrace (exec, ctxFactory, next) { | ||
| try { | ||
| const ret = (ctxFactory && shouldTrace(requestChannel)) | ||
| ? requestChannel.tracePromise(function () { return handlePromise(exec()) }, ctxFactory()) | ||
| : handlePromise(exec()) | ||
|
|
||
| if (ret) { | ||
| ret.then(null, function (error) { | ||
| next(error || new Error('Rejected promise')) | ||
| }) | ||
|
|
@@ -166,6 +191,20 @@ Layer.prototype.handleRequest = function handleRequest (req, res, next) { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * If the return value is a promise, validate it and return it. | ||
| * @private | ||
| */ | ||
|
|
||
| function handlePromise (ret) { | ||
| if (isPromise(ret)) { | ||
| if (!(ret instanceof Promise)) { | ||
| deprecate('handlers that are Promise-like are deprecated, use a native Promise instead') | ||
| } | ||
| return ret | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Check if this route matches `path`, if so | ||
| * populate `.params`. | ||
|
|
||
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.
Why is
express:requestchannel used here?I think it would be better to use a dedicated channel for router to allow consumers to decide on what to receive and how to handle it.
I think router component can be also used without express.
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 point, I don't think it has to be
expressprefixed but it needs a unique prefix for sure which is why I wanted some opinions on what it should be. My concern isrouter:requestis too generic and can easily conflict with other names in the ecosystem.I had this in the PR description:
WDYT? Maybe
pillarjs:router:request?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.
There seems to be a guideline.
So I guess the string to use would be
pillarjs.router.requestastracing:prefix and event name postfix is added bydc.tracingChannel().Uh oh!
There was an error while loading. Please reload this page.
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.
Doesn't seem like there is a consistent standard, docs do suggest that but others just went with
lib:something:actionformats.Anyways I have no strong opinions here and I think as long as the name is unique enough, it is arbitrary. I renamed it to your suggestion 👍
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 think it should just be
router.requestand notpillarjs.router.request. I say this because pillarjs is more like the organization name and that could change, and we might move this package to the expressjs organization (not saying we’ll do it now, but it could happen, so we shouldn’t be tied to the GitHub organization name).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.
Okay, I understand the concern. In that case, instead of using pillarjs, it’s better to use express, since it’s officially under the expressjs project.
How does Fastify handle it?
Uh oh!
There was an error while loading. Please reload this page.
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.
So should we go for
express.requestorexpress.router.request? To me its arbitrary but perhaps you have a strong opinion?Fastify uses
fastify.request.handleras seen here.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 think
express.requestis a bit too generic. There might be other parts in express describing the request lifecycle where the router component is not included.e.g.
express.request.routerorexpress.router.request- tending towardsexpress.request.routerlike fastify and hierarchic ordering.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.
Went with
express.router.requestsince it describes the action better "Express's router is handling a request".But happy to change to whatever that is acceptable to get this through.