building5 / sails-hook-bunyan

Bunyan integration for Sails
MIT License
5 stars 3 forks source link

Adding req_id generation #3

Closed JeffAshton closed 9 years ago

JeffAshton commented 9 years ago

As recommended by the guys a Joyent ( http://trentm.com/talk-bunyan-in-prod/#/21 ), tagging requests with a unique id makes it dead simple to query for all the logs pertaining to an individual request.

leedm777 commented 9 years ago

I always thought of this as something that should be outside of sails-hook-bunyan. We do this, but we have a different middleware that adds the id to the request, and a custom serializer that includes req.id when serializing the request.

leedm777 commented 9 years ago

@JeffAshtonCT OTOH, after some thought, logging req with every message seems overkill. Might be preferable to log the req just once, and have every other message just reference the request id. Thoughts?

JeffAshton commented 9 years ago

Logging the request at the beginning and the response at the end makes sense. But only if the request id is present in all messages within.

At the same time, having the request info in every message is really nice too for searching. If I wanted to search for a particular error in /example /buggy/route, then the request info is great to have. Might make sense to just have a config for this. But if no one is asking, hold off.

I like what this guy is proposing too https://brandur.org/request-ids#multiple), of tracking multiple request ids, effectively producing a service call stack.

I feel request id makes sense in this project as it's designed for the benefit of logging. I can't think of a good reason to not do it, which is why i opted for it defaulting to true. Will likely add support for X-Request-Id over the weekend and send you another pull request.

leedm777 commented 9 years ago
  1. Fix/add items noted above
  2. Rebase against master and squash into a single commit

Thanks for the improvement!

JeffAshton commented 9 years ago

I started looking at options for supporting a couple different scenarios,

1) attach a unique id to every request 2) support heroku style (https://devcenter.heroku.com/articles/http-request-id), where a request id can be specified via X-REQUEST-ID header, otherwise generated 3) support request id stack via X-REQUEST-ID header, as proposed by https://brandur.org/request-ids#multiple

I think it should be this project's goal to setup the Logger on every request (which you have), and make it easy to assign that request id to the Logger options. Played with a couple different configuration scenarios, and here's my proposal

module.exports.log = {

    /** If true, a child logger is injected on each request */
    injectRequestLogger: true,

    /**
     * Gets or generates a unique id for the request, and attaches it
     * to the request logger's options. If no id is returned, then the
     * request logger is unmodified.
     *
     * The default provider returns a new UUID v4 and attaches
     * it to req.id
     * 
     * Note: Only called if injectRequestLogger is true.
     */
    requestIdProvider: function( req ) {

        if( ! req.id ) {
            req.id = uuid.v4();
        }
        return req.id;
    },

    /** Name of the request id property assigned to the request logger */
    requestIdProperty: 'req_id'
}

This design should support all 3 use cases above, and give developer's joyent's recommended practice by default. Thoughts?

JeffAshton commented 9 years ago

Opened new pull request with squashed changes. I like how the requestIdProvider turned out. Makes it easy to do the recommended minimal id per request, but still gives the flexibility to extend outside this project.