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

responseTime to use request responded. Closes #530 #531

Merged
merged 1 commit into from Dec 30, 2017

Conversation

@kevinmstephens
Copy link
Contributor

kevinmstephens commented Nov 9, 2016

Hapi 15 docs:

responded - request response timestamp (0 is not responded yet).

This will use the diff of responded to received as the response event responseTime. Fallback to Date.now() behavior when responded=0 or just in case any old Hapi versions don't set this.

No test added b/c I didn't see any similar testing of event flags. Glad to add one if needed.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Nov 10, 2016

I would like to still have the time of the tail when the log is on the tail event though...

@kevinmstephens

This comment has been minimized.

Copy link
Contributor Author

kevinmstephens commented Nov 10, 2016

@AdriVanHoudt I think we should add a new flag for that. Could be tailTime and be the same logic as what responseTime currently is.

Does that work or do you have any other suggestions?

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Nov 10, 2016

I'd be OK with that.... that's technically a breaking change though since we're changing what responseTime means... maybe add a new field for request.info.responded and leave responseTime alone for now and then in a subsequent major release, change it around?

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Nov 10, 2016

And then document that responseTime includes tail time when enabled? Works for me.
And why not just a major with all the PR's that are open @arb?

@kevinmstephens

This comment has been minimized.

Copy link
Contributor Author

kevinmstephens commented Nov 10, 2016

@arb glad to leave that field alone but actually I would recommend we change responseTime to be what it seems like it should be (the actual response time).

As-is it's not intuitively named and I as well as my peers have been confused here and had to look into the code to figure out that oh, it's really the time including tail. Why is it named responseTime?

@kevinmstephens

This comment has been minimized.

Copy link
Contributor Author

kevinmstephens commented Nov 10, 2016

I'll fix the PR to be non-breaking for now. If you end up deciding to do a major please give me a chance to switch the flag names though.

@kevinmstephens kevinmstephens force-pushed the kevinmstephens:response-time branch from 2def80d to 7529118 Nov 10, 2016
@kevinmstephens

This comment has been minimized.

Copy link
Contributor Author

kevinmstephens commented Nov 10, 2016

I've added the breaking change here. I will open another PR with the non-breaking alternative.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Dec 21, 2017

Hey @kevinmstephens are you still interested in landing this? I'd like to have it part of the 8.0.0-rc1 release.

@arb arb added this to the 8.0.0-rc1 milestone Dec 21, 2017
@kevinmstephens

This comment has been minimized.

Copy link
Contributor Author

kevinmstephens commented Dec 21, 2017

@arb So I see in V17 of Hapi that tail is no longer supported. Does that render the tailTime feature N/A or should we support hapi releases < 17?

The change of using the more accurate this.responseTime = request.info.responded - request.info.received; seems relevant so I would still like to make that change.

If you can let me know on the support for tails I can fix this PR up accordingly.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Dec 21, 2017

I'm really not planning on supporting < 17 anymore only security updates. You're right though, hapi 17 lost tails, so that change we can just nix.

@kevinmstephens kevinmstephens force-pushed the kevinmstephens:response-time branch 2 times, most recently from 13e97a6 to a893b34 Dec 22, 2017
@kevinmstephens kevinmstephens force-pushed the kevinmstephens:response-time branch from a893b34 to c10ed58 Dec 22, 2017
@kevinmstephens

This comment has been minimized.

Copy link
Contributor Author

kevinmstephens commented Dec 22, 2017

@arb Ready for review.

@kevinmstephens

This comment has been minimized.

Copy link
Contributor Author

kevinmstephens commented Dec 22, 2017

I was checking to be sure request.responded would always be set and from what I can tell - yes. There is only one place in the hapi code that emits the response event and you can see here that responded is set right before this event is emitted.

https://github.com/hapijs/hapi/blob/657be02213f2961ac13b7853db5d9219ed75a520/lib/request.js#L361-L385

@arb
arb approved these changes Dec 30, 2017
@arb arb merged commit ce6cc96 into hapijs:master Dec 30, 2017
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.