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

Give customProps access to the response #197

Merged
merged 7 commits into from
Jan 20, 2022

Conversation

travisbeck
Copy link
Contributor

This PR fixes the issues with #168 and also fixes the regression it introduced where customProps wouldn't be set until the response was sent (if you were using customReceiveMessage or just any other logging on your server).

The downsides are the customProps gets called twice, but it feels worth it to me personally.

logger.js Outdated
if (customPropBindings) {
fullReqLogger = fullReqLogger.child(customPropBindings)
}
fullReqLogger = fullReqLogger.child(customPropBindings)
Copy link
Member

Choose a reason for hiding this comment

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

Could you please restore these changes? This if gives us significant throughput. See #196 for more details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Fixed.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 113f86f into pinojs:master Jan 20, 2022
@spence-s
Copy link

spence-s commented May 5, 2022

@mcollina @travisbeck -- I don't understand why customProps child needs to be added in both the onResFinished function and the middleware function? - it causes any custom props to always run into the duplicate child logs problem. -- I think this is problematic as a user who wants to both use customProps and avoid the duplicate keys problem.

Since we are setting the child logger in the middleware - it should be the same instance in the onResFinished function and it shouldn't ever need to be set again, unless I am missing something!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants