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

Support route path in request event object #451

Merged
merged 1 commit into from Mar 17, 2016
Merged

Support route path in request event object #451

merged 1 commit into from Mar 17, 2016

Conversation

@krambox
Copy link

krambox commented Mar 11, 2016

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

This comment has been minimized.

Copy link
Contributor

arb commented Mar 11, 2016

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

This comment has been minimized.

Copy link
Author

krambox commented Mar 11, 2016

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

This comment has been minimized.

Copy link
Contributor

arb commented Mar 11, 2016

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

@krambox

This comment has been minimized.

Copy link
Author

krambox commented Mar 11, 2016

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

This comment has been minimized.

Copy link
Contributor

arb commented Mar 12, 2016

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

@krambox

This comment has been minimized.

Copy link
Author

krambox commented Mar 12, 2016

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

This comment has been minimized.

Copy link
Contributor

arb commented Mar 12, 2016

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 krambox force-pushed the krambox:master branch from 3973a2f to 5eaad2c Mar 12, 2016
@krambox

This comment has been minimized.

Copy link
Author

krambox commented Mar 12, 2016

Ok,i rebased this into one commit.

@arb arb self-assigned this Mar 13, 2016
@arb arb added the feature label Mar 13, 2016
@arb arb added this to the 7.0.0 milestone Mar 13, 2016
@@ -66,6 +66,7 @@ describe('utils', () => {
},
method: 'POST',
path: '/',
route: '/',

This comment has been minimized.

Copy link
@arb

arb Mar 14, 2016

Contributor

This isn't right. The route object of a request should be more than just a string according to https://github.com/hapijs/hapi/blob/master/API.md#route-public-interface

@krambox krambox force-pushed the krambox:master branch 2 times, most recently from dc999f3 to 2ddbc6f Mar 15, 2016
@postmann postmann force-pushed the krambox:master branch from 7832942 to c26cfab Mar 17, 2016
@krambox

This comment has been minimized.

Copy link
Author

krambox commented Mar 17, 2016

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.

arb added a commit that referenced this pull request Mar 17, 2016
Support route path in request event object
@arb arb merged commit 7b8b138 into hapijs:master Mar 17, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.