-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat(RequestLogging): Add createContextStorage
#153
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pulling this together 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
src/requestLogging/requestLogging.ts
Outdated
/** | ||
* Returns context fields for the passed Koa context | ||
*/ | ||
export type ContextFields = (ctx: Koa.Context) => Record<string, unknown>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth aligning this closer with RequestLogging.createMiddleware
:
export type ContextFields = (ctx: Koa.Context) => Record<string, unknown>; | |
export type ContextFields = (ctx: Koa.Context, fields: Fields) => Record<string, unknown>; |
This would make it easier to extend the context without having to manually invoke the baseline contextFields
:
createLoggerContextStorage((ctx, fields) => ({
...fields,
additionalField: getAdditionalField(ctx),
}));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooo yep that would make sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 Nice one!
fyi I don't have perms in this repo 😆 @72636c |
createContextStorage
FYI, this doesn't show as a breaking change on renovate PRs. https://github.com/SEEK-Jobs/cm-rr-service/pull/167 Because BREAKING CHANGE wasn't in a commit message. |
rip thanks for letting us know. Should be relatively okay though since Node 10 is no longer maintained |
BREAKING CHANGE: The minimum version of Node.js has been bumped up to 12.17
This adds functionality to allow for a root logger instance to retain request based context.
This means you can simply import the logger in any file and the request context will just be there when you call
logger.info
,logger.error
or any other method.This means you will no longer need to create a child logger to pass around the codebase in order to retain request context.
This implementation uses the Node.js AsyncLocalStorage instance to track logger context.
There is a performance hit of ~1.75% on Node 16.2.0+ according to this which is negligible