search

hapijs / good

hapi process monitoring
Other
525 stars 160 forks source link

responseTime to use request responded. Closes #530 #531

Closed kevinmstephens closed 6 years ago

kevinmstephens commented 7 years ago

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 commented 7 years ago

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

kevinmstephens commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

arb commented 6 years ago

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

kevinmstephens commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

@arb Ready for review.

kevinmstephens commented 6 years ago

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

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.