Skip to content
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

Use diagnostics_channel in layers #96

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ Router.prototype.handle = function handle(req, res, callback) {
var self = this
var slashAdded = false
var paramcalled = {}
var matchedLayer = false

// middleware and routes
var stack = this.stack
Expand Down Expand Up @@ -286,6 +287,14 @@ Router.prototype.handle = function handle(req, res, callback) {
: layer.params
var layerPath = layer.path

if (!matchedLayer) {
matchedLayer = true
req.layerStack = req.layerStack || []
} else {
req.layerStack.pop()
}
req.layerStack.push(layer)

// this should be done for the layer
self.process_params(layer, paramcalled, req, res, function (err) {
if (err) {
Expand Down
30 changes: 30 additions & 0 deletions lib/layer.js
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('router.layer.handle_request')
onHandleError = dc.channel('router.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,6 +78,15 @@ Layer.prototype.handle_error = function handle_error(error, req, res, next) {
return next(error)
}

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

try {
fn(error, req, res, next)
} catch (err) {
Expand All @@ -89,6 +111,14 @@ Layer.prototype.handle_request = function handle(req, res, next) {
return next()
}

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

try {
fn(req, res, next)
} catch (err) {
Expand Down
9 changes: 9 additions & 0 deletions lib/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ Route.prototype._methods = function _methods() {
*/

Route.prototype.dispatch = function dispatch(req, res, done) {
var matchedLayer = false
var idx = 0
var stack = this.stack
if (stack.length === 0) {
Expand Down Expand Up @@ -138,6 +139,14 @@ Route.prototype.dispatch = function dispatch(req, res, done) {
return done(err)
}

if (!matchedLayer) {
matchedLayer = true
req.layerStack = req.layerStack || []
} else {
req.layerStack.pop()
}
req.layerStack.push(layer)

if (err) {
layer.handle_error(err, req, res, next)
} else {
Expand Down
106 changes: 106 additions & 0 deletions test/diagnostics-channel.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@

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

var dc = require('diagnostics_channel');
var onHandleRequest = dc.channel('router.layer.handle_request');
var onHandleError = dc.channel('router.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', (req, res, next) => {
next();
}, 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

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

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

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

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

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

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. 👍

});
});