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

Maintenance: add tests for child logger attributes #483

Closed
4 tasks
saragerion opened this issue Jan 18, 2022 · 4 comments · Fixed by #1286
Closed
4 tasks

Maintenance: add tests for child logger attributes #483

saragerion opened this issue Jan 18, 2022 · 4 comments · Fixed by #1286
Assignees
Labels
completed This item is complete and has been merged/shipped documentation Improvements or additions to documentation help-wanted We would really appreciate some support from community for this one logger This item relates to the Logger Utility

Comments

@saragerion
Copy link
Contributor

saragerion commented Jan 18, 2022

Description of the feature request

Problem statement

During a 1:1 feedback session it emerged that the current behavior of Logger.createChild might not be immediately clear to people approaching the library for the first time. Likewise, the current unit test cases, while reaching 100% coverage, might use some improvements to cover all cases.

The steps to complete this issue should be (in this order):

  • Assess the current implementation of both feature and tests
  • Compare the implementation with current documentation section
  • Report findings / proposed improvements (under this issue). The report should cover: 1/ are current unit tests covering all cases? If not, which cases could be added? 2/ does current documentation articulate clearly the behavior of createChild? If not, what would you add?
  • Implement any changes that might have emerged from point above

Summary of the feature

N/A

Code examples

Unit tests for the feature: link
Implementation for the feature: link

Benefits for you and the wider AWS community

Describe alternatives you've considered

Additional context

N/A

Related issues, RFCs

N/A

@saragerion saragerion added triage This item has not been triaged by a maintainer, please wait priority:medium labels Jan 18, 2022
@saragerion saragerion added this to the production-ready-release milestone Jan 18, 2022
@dreamorosi dreamorosi added the good-first-issue Something that is suitable for those who want to start contributing label Feb 21, 2022
@dreamorosi dreamorosi changed the title Logger: add tests for child logger attributes Feature (logger): add tests for child logger attributes May 12, 2022
@saragerion saragerion removed this from the production-ready-release milestone May 16, 2022
@dreamorosi dreamorosi added logger This item relates to the Logger Utility need-more-information Requires more information before making any calls discussing The issue needs to be discussed, elaborated, or refined tests PRs that add or change tests help-wanted We would really appreciate some support from community for this one and removed priority:medium triage This item has not been triaged by a maintainer, please wait labels Nov 13, 2022
@dreamorosi dreamorosi changed the title Feature (logger): add tests for child logger attributes Maintenance: add tests for child logger attributes Nov 14, 2022
@shdq
Copy link
Contributor

shdq commented Feb 13, 2023

Hello there! As part of the work on the childLogger @dreamorosi asked to look at this issue and fill in the gaps.

  • Assess the current implementation of both feature and tests
  • Report findings / proposed improvements (under this issue). The report should cover: 1/ are current unit tests covering all cases? If not, which cases could be added? 2/ does current documentation articulate clearly the behavior of createChild? If not, what would you add?

During two rounds of improvements (PR #1178 and PR #1267) implementation was changed and weak spots of unit tests were uncovered and necessary adjustments were added. Especially the test in the last PR covers all child logger attributes assesses against non-default values which was an issue with previous tests.

  • Compare the implementation with current documentation section

childLogger is slightly mentioned in docs under Using multiple Logger instances across your code. For people who came from powertools for python it can be confusing since there all state changes will be propagated bi-directionally between child and parent. Here in TypeScript after the creation, they live separate lives and don't influence each other. Only attributes passed to childLogger will be overwritten. I think it should be mentioned in the docs or even explicitly list all the attributes that can be overwritten.

I would also add an example with a child logger where it implicitly inherits some parent's attributes, like logFormatter from this issue #1264. I would also add general use cases for child loggers in lambda functions (need help with that).

@dreamorosi
Copy link
Contributor

Hey Sergei, thank you for looking into this and for the assessment.

I agree that perhaps we can improve this section of the docs.

Only attributes passed to childLogger will be overwritten. I think it should be mentioned in the docs or even explicitly list all the attributes that can be overwritten.

In regards to this I would say that as a user you can overwrite any of the settings that can also be set via constructor, which are the default ones, any persistent attributes, and the log formatter.

I would also add an example with a child logger where it implicitly inherits some parent's attributes

Maybe we could update the example in this section to not only show that you can overwrite but also that other attributes are passed down to the child. Maybe we could set service name and/or persistent attribute - similar to this:

import { Logger } from '@aws-lambda-powertools/logger';

// This logger has a service name, some persistent attributes
// and log level set to INFO
const logger = new Logger({
    serviceName: 'serverlessAirline',
    logLevel: 'INFO',
    persistentLogAttributes: { 
        aws_account_id: '123456789012',
        aws_region: 'eu-west-1',
    },
});

// This other logger inherits all the parent's attributes 
// but the log level, which is now set to ERROR
const childLogger = logger.createChild({
    logLevel: 'ERROR'
});

We should also modify the CloudWatch log excerpt (other tab).

like logFormatter from this issue #1264

I would add a "tip" box (see here) in the section that talks about custom log formatter instead saying something like: "When you set a custom formatter it is propagated/passed down to any children logger" (worded more nicely maybe)

I would also add general use cases for child loggers in lambda functions (need help with that).

We have another issue tracking the creation of a "recommended / best practices" section in the docs to cover this type of topic. I would defer this last point to that issue/discussion (perhaps we could add a comment under that issue linking to here to not forget), but I would leave it out for now as it's not strictly related to how the logger works.

What do you think?

@shdq
Copy link
Contributor

shdq commented Feb 13, 2023

Thank you for the proposals, I like them! So I'm going to make a PR, and let's continue with the discussion there.

@dreamorosi dreamorosi added documentation Improvements or additions to documentation confirmed The scope is clear, ready for implementation and removed discussing The issue needs to be discussed, elaborated, or refined tests PRs that add or change tests need-more-information Requires more information before making any calls good-first-issue Something that is suitable for those who want to start contributing labels Feb 13, 2023
@github-actions
Copy link
Contributor

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@github-actions github-actions bot added the pending-release This item has been merged and will be released soon label Feb 13, 2023
@dreamorosi dreamorosi moved this from Coming soon to Shipped in AWS Lambda Powertools for TypeScript Feb 27, 2023
@dreamorosi dreamorosi added completed This item is complete and has been merged/shipped and removed pending-release This item has been merged and will be released soon confirmed The scope is clear, ready for implementation labels Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completed This item is complete and has been merged/shipped documentation Improvements or additions to documentation help-wanted We would really appreciate some support from community for this one logger This item relates to the Logger Utility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants