Skip to content
Closed
53 changes: 53 additions & 0 deletions examples/trace/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
var debug = require('debug')('trace-example');
var express = require('../../');

var app = express();

// Add a middleware that runs trace function.
app.use(function(req, res, next){
res.start = Date.now()
res.id = random();
res.trace('start', new Date());
next();
});

// Add another middleware that runs trace after an asynchronous call.
app.use(function(req, res, next){
res.trace('wait:before');
setTimeout(function () {
res.trace('wait:after');
next();
}, Math.random() * 100);
})

// Add a route to show request traces.
app.get('/', function(req, res, next){
res.trace('user.id', random());
res.trace('some.event', random(), random());
res.trace('another.event', random(), random());
res.set('X-Response-Time', (Date.now() - res.start) + 'ms');
res.send('Hello world!');
next();
});

// Add an after request processing middleware that runs trace.
app.use(function(req, res, next){
res.trace('finish');
});

// Configure tracer.
app.instrument(function(options){
debug(options.res.id, options.event, options.args.join(' '));
});

// Helpers to generate random ids.
function random(){
return Math.random().toString(36).slice(2);
};


/* istanbul ignore next */
if (!module.parent) {
app.listen(3000);
console.log('Express started on port 3000');
}
62 changes: 62 additions & 0 deletions lib/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ app.init = function init() {
this.cache = {};
this.engines = {};
this.settings = {};
this.tracers = [];

this.defaultConfiguration();
};
Expand Down Expand Up @@ -98,6 +99,10 @@ app.defaultConfiguration = function defaultConfiguration() {
this.response.__proto__ = parent.response;
this.engines.__proto__ = parent.engines;
this.settings.__proto__ = parent.settings;

this.instrument(function callParentTracer(options) {
parent._callTracers(options.res, options.event, options.args);
});
});

// setup locals
Expand Down Expand Up @@ -617,6 +622,63 @@ app.listen = function listen() {
return server.listen.apply(server, arguments);
};

/*
* Set a tracer at the application level. Tracer will be activated when a
* tracing call will be fired by a response.
* The tracer function takes an option object as argument with the given
* attributes:
*
* - app: current application
* - res: response that fires the tracer
* - req: request related to above response
* - event: string describing the event to trace
* - date: when the tracing occurs
* - args: allow to freely add information
*
* @param {function} The tracer to set.
* @return {app} for chaining.
* @public
*/

app.instrument = function(tracer){
if (tracer === undefined || typeof tracer !== 'function')
throw new TypeError('instrument expects a function');
this.tracers.push(tracer);
return this;
};

/*
* Call all tracers set on the application. Add context information:
* running app, request, response and date. The date is the when the tracing
* was fired (now).
*
* @param {Response} Response that fires the tracing event.
* @param {Event} The event to trace.
* @param {Array} Arguments to transmit to the tracker.
*
* @private
*/

app._callTracers = function(res, event, args){
var date = new Date();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we really need a date? Would there ever be such a thing as a delayed tracing call?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I thought that people who wanted to trace something would be interested to automatically have date for their traces. This line generates it for them.

But if you consider that it is the tracer's job, it can be removed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was merely wondering what the purpose was, and I think you answered it well enough.

var app = this;
var tracers = this.tracers;
var length = tracers.length;

var options = {
app: app,
res: res,
req: res.req,
event: event,
date: date,
args: args
};

for (var i = 0; i < length; i++) {
tracers[i](options);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we want to do any kind of special handling if one of these tracers throws an error, even if that is just to continue to execute all the tracers?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good point but I don't know what is best. Do you know what is the best practice in this case?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you know what is the best practice in this case?

I'm not sure, because that is a question that has to do with what exactly the usage of app.instrument is supposed to be. Something needs to be done, and I was hoping you would know, since you made the PR :) I can help decide, but need to understand a lot better how this PR would be used in practice, etc. You know, the code here is being built for someone to use, and those users' needs are what this needs to cater towards. I don't really have the need, so it's hard for me to answer this question unless you can help educate me on the needs :)

Copy link
Copy Markdown
Author

@frankrousseau frankrousseau Nov 13, 2016

Choose a reason for hiding this comment

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

I made that PR two years ago based on a open issue. At that time, everyone looked to be fine with the current implementation. So I propose we don't change anything for the moment.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If Koa is doing this using a module, why not do that with Express? For example, make a module that works just like the Koa one:

var app = express()
require('express-trace')(app)

}
};

/**
* Log error using console.error.
*
Expand Down
28 changes: 28 additions & 0 deletions lib/response.js
Original file line number Diff line number Diff line change
Expand Up @@ -1051,6 +1051,34 @@ function sendfile(res, file, options, callback) {
file.pipe(res);
}

/**
* Run every tracers set at the application level for given events
* and arguments. To set a tracer on the application use the `instrument`
* method.
*
* @param {function} event The event to trace.
* @return {ServerResponse} for chaining
* @public
*/

res.trace = function(event) {
var args;
switch (arguments.length) {
case 0:
throw new Error('No event defined!');
case 1:
args = [];
break;
case 2:
args = [arguments[1]];
break;
default:
args = [].slice.call(arguments, 1);
}
this.app._callTracers(this, event, args);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would there be any benefit to scheduling the tracers run to a new call stack, rather than in the middle of the request trying to be measured?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sorry, I don't get it. Can you provide more details (and examples eventually)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm, I'm not sure how to elaborate since I don't fully understand the purpose of the tracers. Can you help me understand what kinds of things people will be putting the tracers? The example provided in this PR shows only a single tracer simply doing debug(options.res.id, options.event, options.args.join(' '));. Is that the only thing people will do, or are they expected to add more complex tasks there?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

FWIW The main use case for me is tracing request times as given in the example https://github.com/expressjs/express/pull/2437/files#diff-65b72b264c47d33fec6edef30f143501R8
So it's quite important that the times measured reflect the actual time taken.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @No9 if that is the use-case, why do we need to have multiple tracers? Can you elaborate on the measurement aspect? I see you pointed to the line res.start = Date.now(), but that is in the middleware, so I'm not sure how that would be affected?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @No9 wouldn't the use-case outlined at the top of #2191 be better handled with the router events being worked on in PR pillarjs/router#49 ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@No9 Could you try using my branch to see if the current state answers to your needs?

Copy link
Copy Markdown

@No9 No9 Nov 13, 2016

Choose a reason for hiding this comment

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

@dougwilson Oh that is interesting and it looks exactly like what I am looking for thanks - I will have a look during the week.
I also see overall tracing in node is being discussed at the moment nodejs/node#9304

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

There are several discussions in this thread, let's put numbers on it to make things easier to follow:

1) Multiple tracers

I think we should keep it that way. People may need different levels of tracing and it doesn't make the code much more complex.

2) processNextTick

Not sure to understand the need of processNextTick here. I suppose that people expect an immediate execution of the tracing and processNextTick could confuse them. Can you give me an example?

3) Tracing middlewares

Tracing is very specific to developed application. In which case 3rd party middlewares trace something?

5) Documentation

I will care of it. I will see what could be done for the following use cases:

  • Basic debug calls
  • Response time measurement
  • Generate Chrome Trace outputs

If I can't find time to do it, I will notify you.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re docs @frankrousseau I created a tracking issue a while back: expressjs/expressjs.com#729. If there questions or issues specific to how/what needs docs, please put them there. Thanks!

return this;
};

/**
* Stringify JSON, like JSON.stringify, but v8 optimized.
* @private
Expand Down
23 changes: 23 additions & 0 deletions test/app.instrument.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@

var express = require('../');

describe('app', function(){
describe('.instrument(fn)', function(){
it('should add function to the tracer list', function(){
var app = express();
function debug (options) {
console.log(options.event);
}
app.instrument(debug);
debug.should.be.equal(app.tracers[0]);
});

it('should throw err if tracer is not a function', function(){
var app = express();

(function(){
app.instrument('notatracer');
}).should.throw('instrument expects a function');
});
})
})
98 changes: 98 additions & 0 deletions test/res.trace.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@

var express = require('../');
var request = require('supertest');
var assert = require('assert');

describe('res', function(){
describe('.trace(event)', function(){
it('should call all tracers set at application level', function(done){
var app = express();

app.instrument(function(options){
assert.equal(options.app, app)
assert.equal(options.req.url, '/');
assert.equal(options.res.id, '1');
assert.equal(options.event, 'one:event');
assert.equal(options.args[0], 'info');
});

app.use(function(req, res, next){
res.id = '1';
res.trace('one:event', 'info');
next();
});

app.get('/', function(req, res){
res.send('ok');
});

request(app)
.get('/')
.expect(200, done);
})
})

describe('.trace(event) with parents', function(){
it('should call parent tracers too', function(done){

var app = express();
var appParent1 = express();
var appParent2 = express();

var parentTraces1 = [];
var parentTraces2 = [];

appParent1.instrument(function(options){
assert.equal(options.req.id, '1');
parentTraces1.push(options.event + ':parent1');
});

appParent2.instrument(function(options){
assert.equal(options.req.id, '1');
parentTraces2.push(options.event + ':parent2');
});

appParent1.use('/childof1', app);
appParent2.use('/childof2', app);
app.instrument(function(options){
assert.equal(options.event, 'one:event');
assert.equal(options.args[0], 'info');
});

app.use(function(req, res, next){
req.id = '1';
next();
});

app.get('/', function(req, res){
res.trace('one:event', 'info');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In this test, isn't info also emitted? I don't see anything verifying that, so we should either verify or just remove the argument here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That link is going to a different test than the one I'm commenting on here.

res.send('ok');
});

request(appParent1)
.get('/childof1')
.expect(200, function(err){
if (err) return done(err);

assert.equal(parentTraces1.length, 1);
assert.equal(parentTraces2.length, 1);
assert.equal(parentTraces1[0], 'one:event:parent1');
assert.equal(parentTraces2[0], 'one:event:parent2');

request(appParent2)
.get('/childof2')
.expect(200, function(err){
if (err) return done(err);

assert.equal(parentTraces1.length, 2);
assert.equal(parentTraces2.length, 2);
assert.equal(parentTraces1[0], 'one:event:parent1');
assert.equal(parentTraces2[0], 'one:event:parent2');
assert.equal(parentTraces1[1], 'one:event:parent1');
assert.equal(parentTraces2[1], 'one:event:parent2');
done();
});
});
});
})
});