Skip to content

Use diagnostics_channel in layers #96

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.

Sign up for GitHub

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

Draft
Qard wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Use diagnostics_channel in layers #96

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7eba938
Use diagnostics_channel in layers
Qard Sep 17, 2020
231f3ee
Fix channel names
Qard Sep 18, 2020
bae6c12
Fix layerStack timing issue
Qard Sep 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions lib/layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,18 @@
var pathRegexp = require('path-to-regexp')
var debug = require('debug')('router:layer')

/**
* Diagnostic channels
* @private
*/
var onHandleRequest
var onHandleError
try {
var dc = require('diagnostics_channel')
onHandleRequest = dc.channel('express.layer.handle_request')
onHandleError = dc.channel('express.layer.handle_error')
} catch (err) { }

/**
* Module variables.
* @private
Expand All @@ -41,6 +53,7 @@ function Layer(path, options, fn) {
this.params = undefined
this.path = undefined
this.regexp = pathRegexp(path, this.keys = [], opts)
this.routingPath = path

// set fast path flags
this.regexp.fast_star = path === '*'
Expand All @@ -65,9 +78,23 @@ Layer.prototype.handle_error = function handle_error(error, req, res, next) {
return next(error)
}

req.layerStack = req.layerStack || []
Copy link

@mcollina mcollina Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend against creating a new array for every request if diagnostics_channel is not enabled.

Copy link
Author

@Qard Qard Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair. Seemed like a possibly useful feature outside of the diagnostics_channel stuff itself, but sounds like there's already another layer-tracking thing in discussion, so I'll take a closer look at that. :)

Copy link
Contributor

@dougwilson dougwilson Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May also need to consider what the expected behavior is when different versions of this module are mixed together.

req.layerStack.push(this)

if (onHandleError && onHandleError.shouldPublish()) {
onHandleError.publish({
error: error,
request: req,
response: res,
layer: this
})
}

try {
fn(error, req, res, next)
req.layerStack.pop()
} catch (err) {
req.layerStack.pop()
Copy link
Contributor

@dougwilson dougwilson Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this just be put into a finally block?

Copy link
Author

@Qard Qard Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That'd run after the next(...) though, which would interfere with subsequent routes, as far as I can tell. Could do that if the next(...) on the following line was in a process.nextTick(...), but that'd have a performance hit.

Copy link
Contributor

@dougwilson dougwilson Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. But isn't the one within the try already running after the next call? Like if the middleware sync calls next then it will run after, but if the middleware async calls next it will be before.

Copy link
Author

@Qard Qard Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, true. Probably a better place to put that code. 🤔

Copy link
Contributor

@dougwilson dougwilson Sep 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, it looks like adding anything non-trivial to your tests seems to highlight the weirdness here. For example, I added the cors module to the nested routes test like the following:

  it('nested routes have multiple layers with paths', function (done) {
    var outer = new Router();
    var inner = new Router();

    inner.get('/:name', function (req, res) {
      res.end();
    });

    outer.use('/hello', cors(), inner);

    function end() {
      assert.strictEqual(joinLayerStack(handleRequest.request.layerStack), '/hello/:name/');
      done();
    }

    outer.handle({ url: '/hello/world', method: 'GET', headers: {} }, { end, setHeader: noop }, noop);
  });

But it failed the test listing the path as /hello/hello/:name/, as I would surmise that it is performing the pop operation too late, after the middleware is getting iterated through (the more middleware added, the more times /hello appears).

Copy link
Author

@Qard Qard Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a possible fix for the layer timing issue. Not sure it's the best solution, but seems to work. Still unsure if the layerStack approach is the best way. Willing to accept any suggestions on better ways to track the overall routing state of the app. 😅

Copy link
Contributor

@dougwilson dougwilson Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yea. It seems seems to have issues with the latest change keeping track of the stack. For example adding inner.get('/world', (req, res, next) => next()) above inner.get produces the route as /hello/world/:name.

As far as a best solution... I think perhaps it may be better to maybe start from the top so we can understand what, exactly, we are trying to accomplish in this pull request? The PR title and description only mentions adding hooks for diagnostics_channel, but almost all the following discussion does not seem related to the actual diagnostics_channel usage, and rather adding a completely new feature, independent of the diagnostics_channel feature.

Should these two things be split apart into two PRs (first suggested at #96 (comment)) or can the description be updated to describe what the goal of this PR is? That may help better focus the conversation and code work.

Copy link
Author

@Qard Qard Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capturing routing information is the whole point of adding diagnostics_channel, so this PR doesn't really make sense without it. I'll update the description to elaborate on that need.

Copy link
Contributor

@dougwilson dougwilson Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I get it, but let's think iteratively here, if you are interested in getting things landed instead of waiting for the entire pie to be built. For example, folks want to construct the path even outside of diagnostics_channel, so having access to the path is an independent ask of adding diagnostics_channel support. In addition, if you didn't have that layer stack hanging off the req object, you still already have access to a whole bunch of information, including that the request is being handled, that an error occurred, what the http method is, the http path, etc.

Copy link
Author

@Qard Qard Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We get all that directly from http already. The only context we're really missing from express is the routing information. I only intended the layer tracking stuff in this to be used by diagnostics_channel. Maybe there are more general-purpose uses for that information, but I lack the context and the time to do anything about those right now. This PR was created primarily as a demonstration that the diagnostics_channel API is usable in more complex scenarios. It's not intended to be landed any time soon as the diagnostics_channel feature itself hasn't even landed yet. If you want something more complex from this, I'm probably not going to get to it for awhile.

next(err)
}
}
Expand All @@ -89,9 +116,22 @@ Layer.prototype.handle_request = function handle(req, res, next) {
return next()
}

req.layerStack = req.layerStack || []
req.layerStack.push(this)

if (onHandleRequest && onHandleRequest.shouldPublish()) {
onHandleRequest.publish({
request: req,
response: res,
layer: this
})
}

try {
fn(req, res, next)
req.layerStack.pop()
} catch (err) {
req.layerStack.pop()
next(err)
}
}
Expand Down
104 changes: 104 additions & 0 deletions test/diagnostics-channel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@

var Router = require('../')
, assert = require('assert');

var dc = require('diagnostics_channel');
var onHandleRequest = dc.channel('express.layer.handle_request');
var onHandleError = dc.channel('express.layer.handle_error');

function mapProp(prop) {
return function mapped(obj) {
return obj[prop];
};
}

function mapAndJoin(prop) {
return function (list) {
return list.map(mapProp(prop)).join('');
}
}

function noop() { }

describe('diagnostics_channel', function () {
var joinLayerStack = mapAndJoin('routingPath');
var handleRequest;
var handleError;

onHandleRequest.subscribe(function (message) {
handleRequest = message;
});

onHandleError.subscribe(function (message) {
handleError = message;
});

it('use has no layers with a path', function (done) {
var router = new Router();

router.use(function (req, res) {
res.end();
});

function end() {
assert.strictEqual(joinLayerStack(handleRequest.request.layerStack), '/');
done();
}

router.handle({ url: '/', method: 'GET' }, { end }, noop);
});

it('regular routes have a layer with a path', function (done) {
var router = new Router();

router.get('/hello/:name', function (req, res) {
res.end();
});

function end() {
assert.strictEqual(joinLayerStack(handleRequest.request.layerStack), '/hello/:name/');
done();
}

router.handle({ url: '/hello/world', method: 'GET' }, { end }, noop);
});

it('nested routes have multiple layers with paths', function (done) {
var outer = new Router();
var inner = new Router();

inner.get('/:name', function (req, res) {
res.end();
});

outer.use('/hello', inner);

function end() {
assert.strictEqual(joinLayerStack(handleRequest.request.layerStack), '/hello/:name/');
done();
}

outer.handle({ url: '/hello/world', method: 'GET' }, { end }, noop);
});

it('errors send through a different channel', function (done) {
var router = new Router();
var error = new Error('fail');

router.get('/hello/:name', function (req, res) {
throw error;
});

router.use(function (err, req, res, next) {
res.end();
});

function end() {
assert.strictEqual(joinLayerStack(handleRequest.request.layerStack), '/hello/:name/');
assert.strictEqual(handleError.error, error);
Copy link
Contributor

@dougwilson dougwilson Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this validate that the handle error layer stack is pointing to the / path for the error handler that was executed that called res.end()?

Copy link
Author

@Qard Qard Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we should be treating error handlers as top-level routes, that's an implementation detail. It makes more sense from the APM perspective to treat them as continuations of whatever route the error handler was reached from.

Copy link
Contributor

@dougwilson dougwilson Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, interesting. I was assuming your wanted the layer stack to be the current stack of layers--you didn't note that certain layers were going to be treated differently. That is likely a very important point that was left out of your explanation.

So you are saying that even an error handler that is itself a route would not be listed? I.e. app.get('/blog/:slug', (err, req, res, next) = {}) It seems like when you have a complex app where you have a lot of error handlers all defined at a lot of different paths, it may be useful to understand which of your error handlers was the one that ran the error handling, no?

Copy link
Contributor

@dougwilson dougwilson Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking a look, I'm not sure if this behavior is why the following will generate a layer stack showing the "route" is /hello/:name/hello/:val rather than showing it as /hello/:val when called as GET /hello/100:

    var router = new Router();

    router.get('/hello/:name', (req, res, next) => {
      if (!isNaN(Number(req.params.name))) next('route')
      else next()
    }, (req, res) => {
      res.end()
    });

    router.get('/hello/100', (req, res) => {
      res.end()
    })

done();
}

router.handle({ url: '/hello/world', method: 'GET' }, { end }, noop);
Copy link
Contributor

@dougwilson dougwilson Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as a general comment: I'm not sure if you planed to clean this up later, so apologies if so, just wanted to call out that we don't want to be passing in mock-like objects to the router handling methods; they should be the real Node.js HTTP objects like all the other tests so we validate that everything is working as new Node.js versions come out and these objects change over time.

Copy link
Author

@Qard Qard Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the changes are all copied and pasted from the PR I made directly to express, which apparently did tests differently. I'll clean up the tests at some point, when I get back to this. 👍

});
});