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

fix(logger-utils): ensure external loggers doesn't propagate logs on config copy #1075

Merged

Conversation

mploski
Copy link
Contributor

@mploski mploski commented Mar 10, 2022

Issue #, if available:

#1073

Description of changes:

Ensure external loggers doesn't propagate logs to root logger by disabling Propagate attribute. Previously event was emitted in external logger ( with powertools handler) and in root handler IF propagation was set to True. This change ensure log is only emitted by external logger handler.

Also, during debugging noticed that we don't exclude aws_lambda_powertools from handler modification since this is root powertools package we might treat it as internal package and skip applying powertools logger handler. @heitorlessa please correct me if I'm wrong here

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement 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.

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 10, 2022
@mploski mploski marked this pull request as draft March 10, 2022 14:01
@mploski mploski changed the title Ensure external loggers doesnt propagate logs fix(logger): Ensure external loggers doesn't propagate logs Mar 10, 2022
@github-actions github-actions bot added the bug Something isn't working label Mar 10, 2022
@mploski mploski force-pushed the fix/existing-loggers-duplicated-logs branch from dfb79b6 to 51ee897 Compare March 10, 2022 14:13
@mploski mploski requested a review from heitorlessa March 10, 2022 14:14
@mploski
Copy link
Contributor Author

mploski commented Mar 10, 2022

@heitorlessa please review

@mploski mploski marked this pull request as ready for review March 10, 2022 14:15
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

Additional test to ensure we don't break child loggers

aws_lambda_powertools/logging/utils.py Outdated Show resolved Hide resolved
@mploski mploski force-pushed the fix/existing-loggers-duplicated-logs branch from e550eaa to 692dcd6 Compare March 14, 2022 15:18
@mploski mploski force-pushed the fix/existing-loggers-duplicated-logs branch from 692dcd6 to 3d3bfc8 Compare March 14, 2022 15:25
@codecov-commenter
Copy link

codecov-commenter commented Mar 15, 2022

Codecov Report

Merging #1075 (3c4df97) into develop (565d0a5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #1075   +/-   ##
========================================
  Coverage    99.96%   99.96%           
========================================
  Files          119      119           
  Lines         5373     5376    +3     
  Branches       613      613           
========================================
+ Hits          5371     5374    +3     
  Partials         2        2           
Impacted Files Coverage Δ
aws_lambda_powertools/logging/utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 565d0a5...3c4df97. Read the comment docs.

@heitorlessa
Copy link
Contributor

@mploski made a performance change (package_logger as constant over logger instance), removed an unnecessary test, and repurposed that child logger test to cover the rule of not touching child loggers.

I'll merge and make a release once you're okay with it

@heitorlessa
Copy link
Contributor

In another PR, it's worth cleaning up the test functions/fixtures as I noticed they're duplicated from functional/test_logger - in this case, it's easier to have a logger utils for our tests and easily reuse via import (low priority)

@mploski
Copy link
Contributor Author

mploski commented Mar 15, 2022

@mploski made a performance change (package_logger as constant over logger instance), removed an unnecessary test, and repurposed that child logger test to cover the rule of not touching child loggers.

I'll merge and make a release once you're okay with it

We need to also cover one blind spot to not break passing keys between child and parent. We may either prohibit from passing child logger as source to this method our automatically decapsulate parent logger and exclude it from modification to avoid setting propagate=False as it breaks things.

Added fix for this specific part in latest commit. Please review it and if it works for you we can merge it

@heitorlessa
Copy link
Contributor

Awesome, checking as soon as I finish one last meeting

@mploski mploski merged commit 694d68f into aws-powertools:develop Mar 15, 2022
@heitorlessa heitorlessa changed the title fix(logger): Ensure external loggers doesn't propagate logs fix(logger-utils): ensure external loggers doesn't propagate logs on config copy Mar 16, 2022
@mploski mploski deleted the fix/existing-loggers-duplicated-logs branch March 16, 2022 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants