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

Record response time at response end instead of response start #165

Merged
merged 1 commit into from
Mar 20, 2020
Merged

Record response time at response end instead of response start #165

merged 1 commit into from
Mar 20, 2020

Conversation

bobwei
Copy link

@bobwei bobwei commented May 19, 2018

Should record response time at response end instead of response start so that we can record whole response time for streaming response.

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Can you add this as an option instead of changing the behavior?

@dougwilson
Copy link
Contributor

Also the docs need to be updated (though since this should be a new option, add new docs). Here is the current docs:

:response-time[digits]
The time between the request coming into morgan and when the response headers are written, in milliseconds.

@bobwei
Copy link
Author

bobwei commented May 19, 2018

Hi @dougwilson , thanks for your suggestion !

@dougwilson dougwilson dismissed their stale review November 5, 2018 03:12

changed made to pr

@dougwilson dougwilson self-assigned this Nov 5, 2018
@dougwilson
Copy link
Contributor

Hi @bobwei sorry I never came back to this PR. I just realized that instead of landing this as a flag that changes the behavior of an existing token, we should modify the PR to just add a new token with the behavior, which same the same ultimate outcome, but the benefit of being able to include both of these times in the output if that's desired.

@dougwilson
Copy link
Contributor

I have just written all the code to move this to a :total-time token. If you are agreeable on this approach, I can squash this into your PR here and merge as your PR still.

@bobwei
Copy link
Author

bobwei commented Nov 18, 2018

Hi @dougwilson, sure, please proceed.

@pjsg
Copy link

pjsg commented Oct 23, 2019

Can this be merged please?

@bobwei
Copy link
Author

bobwei commented Oct 23, 2019

Go go

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rebased the changes and they are ready to ship

@dougwilson dougwilson merged commit aa718d7 into expressjs:master Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants