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

feat(RequestLogging): Add createContextStorage #153

Merged
merged 17 commits into from
May 21, 2022

Conversation

samchungy
Copy link
Contributor

@samchungy samchungy commented May 16, 2022

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

@samchungy samchungy marked this pull request as ready for review May 16, 2022 08:46
@samchungy samchungy requested a review from a team as a code owner May 16, 2022 08:46
Copy link
Member

@72636c 72636c left a 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 🙇

src/requestLogging/README.md Show resolved Hide resolved
Copy link
Member

@72636c 72636c left a comment

Choose a reason for hiding this comment

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

🚀

/**
* Returns context fields for the passed Koa context
*/
export type ContextFields = (ctx: Koa.Context) => Record<string, unknown>;
Copy link
Member

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:

Suggested change
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),
}));

Copy link
Contributor Author

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

src/requestLogging/requestLogging.ts Show resolved Hide resolved
@samchungy samchungy changed the title feat: add new logger context tracking methods feat: add logger context storage functionality May 20, 2022
Copy link
Member

@72636c 72636c left a comment

Choose a reason for hiding this comment

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

🚀 Nice one!

@samchungy
Copy link
Contributor Author

fyi I don't have perms in this repo 😆 @72636c

@72636c 72636c changed the title feat: add logger context storage functionality feat(RequestLogging): Add createContextStorage May 21, 2022
@72636c 72636c merged commit e4a98d4 into seek-oss:master May 21, 2022
@katiejng
Copy link

katiejng commented May 25, 2022

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.

@samchungy
Copy link
Contributor Author

samchungy commented May 25, 2022

FYI, this doesn't show as a breaking change on renovate PRs. #153

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

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