fix(logger): merge child logger options correctly

[shdq]

Description of your changes

Initial options (if any) now store in the initLogger class property when an instance of Logger creates. When a child creates out of the parent, createChild uses the parents' initial options to create a new instance of Logger.

• lodash.clonedeep dependency is no longer needed

How to verify this change

Related issues, RFCs

Issue number: #1163

PR status

Is this ready for review?: NO
Is it a breaking change?: NO

Checklist

• My changes meet the tenets criteria
• I have performed a self-review of my own code
• I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
• I have made corresponding changes to the documentation
• I have made corresponding changes to the examples
• My changes generate no new warnings
• The code coverage hasn't decreased
• I have added tests that prove my change is effective and works
• New and existing unit tests pass locally and in Github Actions
• Any dependent changes have been merged and published in downstream module
• The PR title follows the conventional commit semantics

Breaking change checklist

• I have documented the migration process
• I have added, implemented necessary warnings (if it can live side by side)

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[shdq]

Hey, @dreamorosi I have some questions to discuss. Could you help me figure this out, please?

• what about addContext? Should it be included in the child?
• some tests use createLogger helper, some just new Logger()? It is needed?
• it is possible to make initOption optional property and check for empty options objects.
• how to remove a dependency of the package in monorepo? using lerna exec ‘nom remove lodash.clonedeep' --scope=logger or manually cleanup?

[dreamorosi]

Hey, @dreamorosi I have some questions to discuss. Could you help me figure this out, please?

Definitely, and thank you for the PR 👍.

• what about addContext? Should it be included in the child?

I would say yes, just like all other attributes set by other means.

If later on there's demand to have this behavior customizable we'll add an option to createChild, but let's cross that bridge when we get there (if ever).

• some tests use createLogger helper, some just new Logger()? It is needed?

Go with new Logger(), that was a helper function that we introduced when initially creating the tests but I wouldn't mind removing it at some point in the future.

• it is possible to make initOption optional property and check for empty options objects.

If I understand the point correctly, and read it in the context of your current implementation, I think it should stay as not optional and default to {} since options defaults to that in the constructor when no options are passed. Correct me if I misread the question.

• how to remove a dependency of the package in monorepo? using lerna exec ‘nom remove lodash.clonedeep' --scope=logger or manually cleanup?

The command would be npm remove lodash.clonedeep @types/lodash.clonedeep -w packages/logger (from the project's root).

While we still have lerna in the repo, it is used only to version & publish the packages to npm (here). For all other operations we use npm workspaces.

[shdq]

I would say yes, just like all other attributes set by other means.

Ok, I'll make addContext, appendKeys, and addPersistentLogAttributes update initOptions inside addToPowertoolLogData and add some tests for it next.

If I understand the point correctly, and read it in the context of your current implementation, I think it should stay as not optional and default to {} since options defaults to that in the constructor when no options are passed. Correct me if I misread the question.

I thought not to include property in instances initialized without options or with an empty object. I tested it and decided, that not having an empty object in some instances of the Logger class is not worth the changes needed.

One other thing not connected to this PR: here #1153 defaultServiceName was moved to Utility, and it imports from @aws-lambda-powertools/commons, because of this in every package TS shows me errors:

Property 'getDefaultServiceName' does not exist on type 'Logger'.ts(2339)

If I import Utility directly from src, it disappears. Is it because these changes are not released in commons?

Thanks for the fast responses!

[dreamorosi]

Ok, I'll make addContext, appendKeys, and addPersistentLogAttributes update initOptions inside addToPowertoolLogData and add some tests for it next.

Nice, sounds sensible.

I thought not to include property in instances initialized without options or with an empty object. I tested it and decided, that not having an empty object in some instances of the Logger class is not worth the changes needed.

Agreed.

One other thing not connected to this PR: here #1153 defaultServiceName was moved to Utility, and it imports from @aws-lambda-powertools/commons, because of this in every package TS shows me errors:

Property 'getDefaultServiceName' does not exist on type 'Logger'.ts(2339)

If I import Utility directly from src, it disappears. Is it because these changes are not released in commons?

That's our bad. We should clarify this better in the CONTRIBUTE document.

You should be able to fix this by running npm run build -w packages/commons - this will rebuild the package which will now have the changes reflected.

From the way that npm workspaces work, the package is resolved locally, so if you were to peak inside package-lock.json you should see that the @aws-lambda-powertools/commons dependency is linked to the packages/commons folder (or at least it should be). Now, since this is a TS project, and also most probably because we haven't found a better way, the package actually links to the dist folder inside packages/commons and not whatever is in packages/commons/src. So when there are changes in commons from another PR, or if you make changes to it, you'll have to rebuild it before these changes are available in any other of the packages.

Hope this clarifies it!

[shdq]

Hey @dreamorosi.

I found a way to get rid of initOptions too. It seems to work smoothly. The best code is no code, right?

It's ready to review. If more tests are needed, let me know.

[dreamorosi]

@shdq: question: why do we merge and use the parentsPowertoolsLogData twice? Maybe worth documenting with a comment (inside the method, so we don't expose it in the docstring)

[shdq]

Constructor doesn't use lambdaContext, so in line 211 it extracts context (if any) and merges it with childPowertoolsLogData to not overwrite the child's options. Thanks for the question, I went through the code again and refactored it, removed extra work, and made it more readable – no comments needed:

const parentsLambdaContext = parentsPowertoolsLogData?.lambdaContext;
if (parentsLambdaContext) {
  childLogger.addToPowertoolLogData({ lambdaContext: parentsLambdaContext });
}

I will push it when I fix the test coverage.

[dreamorosi]

On Thursday I'm planning on making a new release, I'm mentioning this just in case you'd like this PR to be included - if not it'll be guaranteed to be in the next one in ~2 weeks

[shdq]

On Thursday I'm planning on making a new release, I'm mentioning this just in case you'd like this PR to be included - if not it'll be guaranteed to be in the next one in ~2 weeks

I think this PR is finished and will be in the next release 🤞

[dreamorosi]

Thank you @shdq for working on this, appreciate the effort as usual.

Integration tests were passing: https://github.com/awslabs/aws-lambda-powertools-typescript/actions/runs/3533218456