-
Notifications
You must be signed in to change notification settings - Fork 146
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(logger): add clear state functionality #902
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.
LGTM!
Could we add an e2e test ? |
Hi @flochaz, thanks for your suggestion. To test this functionality, I will need to send different payloads to sequential invocations. Option 1 - I'd need to tweak the way we pass the payload to SEQUENTIAL invocations so that we can have custom payloads in different invocations. Option 2 - Easy workaround: call this function twice. |
📊 Package size report 0.9%↑
🤖 This report was automatically generated by pkg-size-action |
@@ -63,12 +63,15 @@ export const generateUniqueName = (name_prefix: string, uuid: string, runtime: s | |||
export const invokeFunction = async (functionName: string, times: number = 1, invocationMode: 'PARALLEL' | 'SEQUENTIAL' = 'PARALLEL', payload: FunctionPayload = {}): Promise<InvocationLogs[]> => { | |||
const invocationLogs: InvocationLogs[] = []; | |||
|
|||
const promiseFactory = (): Promise<void> => { | |||
const promiseFactory = (index?: number): Promise<void> => { |
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.
If you have multiple lambda invocations that are SEQUENTIAL in your e2e scenario, you can now know which invocation is it in the Lambda event payload
📊 Package size report 0.9%↑
🤖 This report was automatically generated by pkg-size-action |
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.
LGTM
e2e test are passing, thanks for running them:
https://github.com/awslabs/aws-lambda-powertools-typescript/actions/runs/2536933581
* feat: add business logic for clear state * chore: removed temporary code * feat: business logic + unit tests for decorator and middleware * chore: format TS * chore: typo in logger docs * chore: lint fix quotes * chore: typo in logger docs * tests(logger): rename test scenario for clear state * chore: exclude non-null rule in decorator * build(deps): revert package-lock.json * test(logger): clear state e2e tests
Description of your changes
The Logger utility is commonly initialized in the global scope, outside the handler function.
When you attach persistent log attributes through the
persistentLogAttributes
constructor option or via theappendKeys
,addPersistentLogAttributes
methods, this data is attached to the Logger instance.Due to the Lambda Execution Context reuse, this means those persistent log attributes may be reused across invocations.
This PR introduces a new flag that developers can enable to "clear the state" across invocations.
If developers want to make sure that persistent attributes added inside the handler function code are not persisted across invocations, they can now set the parameter
clearState
astrue
in theinjectLambdaContext
middleware or decorator.How to verify this change
See Contributing guide for the docs build.
Related issues, RFCs
#482
PR status
Is this ready for review?: YES
Is it a breaking change?: NO
Checklist
Breaking change checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.