hapijs / good

hapi process monitoring
Other
525 stars 160 forks source link

Support route path in request event object #451

Closed krambox closed 8 years ago

krambox commented 8 years ago

I provide a pull request to extend the request event object with the root path.

With this path information, it is possible to provide a route specific event handling and interpreation. We use this information, to extend logging and monitoring informations (with good-bunyan and influx) with the hapi route.

arb commented 8 years ago

You already have the path. Why do you need route too?

Also, you can pass in options now at the route level that will be attached to object payloads: see https://github.com/hapijs/good/blob/master/test/monitor.js#L876-L928

krambox commented 8 years ago

The path is not sufficient for us. We make a trill down analysis in Kibana / Grafana based on the service - method - route triple. The options extension is complicated for us because we need to extend all routes in our various HapiJS micro services. With this patch every good extension can use this information. Currently we use good-bunyan with a custom response formatter function. There may be still other opportunities to extend the event, without patching to good module?

arb commented 8 years ago

In your existing setup, what would the difference be between path and route? What are the differing values?

krambox commented 8 years ago

Typically, the routes have in our scenario parameters such as

server.route ({
     method: 'GET',
     path: '/hello/{user}',
...
});

The good response event would contain for a GET http://localhost/hello/Adam path: '/hello/Adam' and as route value route: '/hello/{user}' (request.route.path)

arb commented 8 years ago

Do you need this data to do some kind of filter or are you actually trying to log this information somewhere?

krambox commented 8 years ago

I need the root.path to log and analyze in Kibana. For example: (The donate diagram shows to sum of the request durations for 5 micro services (inner circle), refined by route und then bei method)

kibana route path

arb commented 8 years ago

While I still think you can do this with some regular expressions in your analytics tool, I don't see any real harm in adding another field.

Please rebase this into a single commit.

krambox commented 8 years ago

Ok,i rebased this into one commit.

krambox commented 8 years ago

I adapt the test/utils.js test and extend an test in test/monitor.js. With the help from a friendly colleague, we reduced the commit mess to an single commit.

lock[bot] commented 4 years ago

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.