-
Notifications
You must be signed in to change notification settings - Fork 176
feat(logger): use async local storage for logger #4668
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
base: main
Are you sure you want to change the base?
Conversation
d8f7864 to
2073078
Compare
2073078 to
c128331
Compare
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.
Just a small question. Rest looks good from my side.
packages/logger/src/Logger.ts
Outdated
| } | ||
|
|
||
| return attributes; | ||
| return this.#attributesStore.getAllAttributes(); |
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.
Do we need to check #checkReservedKeyAndWarn here as it was before?
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.
Ah good catch!
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.
So looking into this a bit more, I was surprised that we didn't have a failing test case in light of this removal.
Upon investigation, I have found that #appendKeys already does this check but the deprecated setPersistentLogAttributes does not. However, we do not have a unit test that uses setPersistentLogAttributes to set a reserved keyword. Upon adding a test case that does this I now had a failing unit test.
Initially I thought that rather than adding reserved key filtering logic to setPersistentLogAttributes that I would instead centralise it in #createAdditionalAttributes as this method is called whenever any log statement is emitted in the createAndPopulateLogItem method. This centralisation would allow us to remove the check from #appendKeys too.
However, when I implemented this, the tests that check for reserved keys started failing with stack overflow errors. This makes sense because the #checkReservedKeyAndWarn function itself has a log statement in it. If you try to use it to do a check in createAndPopulateLogItem you will trigger an infinite loop of log statements.
What I have done instead is add a new function called filterReservedAttributeKeys and used it in both #appendKeys and setPersistentLogAttributes (even though the function is deprecated, it is still part of the public API for now) so that reserved keys will not be added to the store. I have also removed the #createAdditionalAttributes method entirely and we just call this.#attributesStore.getAllAttributes() directly now.
|



Summary
This PR adds support for using an async context (specifically from the InvokeStore package) in the Logger utility. This allows users to emit log statements that are isolated specifically to the current lambda invocation, isolated from any other executions.
Changes
LogAttributesStoreInvokeStorecontext: if they are then the log keys are stored in the current async context, otherwise we fallback to a plain instance wide object as per the current implementation.sequencetesting function to the common testing package.LogAttributesStoreand theLoggerclass as a whole.Issue number: closes #4667
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.