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

feat(logger): type log record in LambdaPowertoolsFormatter with TypedDict #2419

Merged
merged 19 commits into from
Jun 14, 2023

Conversation

erikayao93
Copy link
Contributor

Issue number:
#2401

Summary

Changes

Added types.py file with TypedLog class

User experience

New typing has not been implemented into any other files, so user experience is not impacted.

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

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.

@erikayao93 erikayao93 requested a review from a team as a code owner June 7, 2023 22:48
@erikayao93 erikayao93 requested review from heitorlessa and removed request for a team June 7, 2023 22:48
@boring-cyborg boring-cyborg bot added the logger label Jun 7, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 7, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Jun 7, 2023

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

@github-actions github-actions bot added the feature New feature or functionality label Jun 7, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2023

Codecov Report

Patch coverage: 94.44% and project coverage change: -0.02 ⚠️

Comparison is base (decee59) 97.16% compared to head (70e9c2a) 97.15%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2419      +/-   ##
===========================================
- Coverage    97.16%   97.15%   -0.02%     
===========================================
  Files          155      156       +1     
  Lines         7067     7099      +32     
  Branches       515      518       +3     
===========================================
+ Hits          6867     6897      +30     
- Misses         157      158       +1     
- Partials        43       44       +1     
Impacted Files Coverage Δ
aws_lambda_powertools/logging/types.py 93.10% <93.10%> (ø)
aws_lambda_powertools/logging/formatter.py 100.00% <100.00%> (ø)
...ws_lambda_powertools/logging/formatters/datadog.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 7, 2023
@leandrodamascena leandrodamascena linked an issue Jun 8, 2023 that may be closed by this pull request
2 tasks
@heitorlessa
Copy link
Contributor

Looking...

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.

Great start Erika!! Made comments on how to make this change production ready, and links to help you navigate intricacies of structured typing.

Please do shout if you get stuck

aws_lambda_powertools/logging/types.py Outdated Show resolved Hide resolved
aws_lambda_powertools/logging/types.py Show resolved Hide resolved
aws_lambda_powertools/logging/types.py Outdated Show resolved Hide resolved
aws_lambda_powertools/logging/types.py Outdated Show resolved Hide resolved
aws_lambda_powertools/logging/types.py Outdated Show resolved Hide resolved
aws_lambda_powertools/logging/types.py Outdated Show resolved Hide resolved
examples/logger/src/bring_your_own_formatter.py Outdated Show resolved Hide resolved
examples/logger/src/bring_your_own_formatter.py Outdated Show resolved Hide resolved
examples/logger/src/bring_your_own_formatter.py Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 8, 2023
@boring-cyborg boring-cyborg bot added the internal Maintenance changes label Jun 9, 2023
@heitorlessa
Copy link
Contributor

Looking into this today and sending the fixes along w/ comments before merging

@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Jun 14, 2023
@heitorlessa
Copy link
Contributor

I've pushed the changes to fix Mypy and bring LogRecord as a TypeAlias for Dict | PowertoolsLogRecord - waiting for CI before merging.

The only unrelated change I noticed here was the pre-commit hook for the markdownlint-cli which I suspected it came from an offline chat with @seshubaws due to NodeJS setup -- always use git sha whenever changing a pre-commit or GitHub Action, otherwise it opens the door for abuse since v0.34.0 is a mutable git tag.

THANK YOU so much for going through this change @erikayao93. Much appreciated.


Changes

  • Simplify conditional imports to ease reading and future updates
  • Bring LogRecord TypeAlias
  • Update LambdaPowertoolsFormatter and DatadogLogFormatter to use LogRecord
    • constructor: json_serializer/deserializer
    • method: serialize
  • Fix bring_your_own_formatter.py docs highlighting
  • Confirm conditional imports work across all versions in CI
  • Fix pre-commit hook to use immutable image
    • markdownlint-cli
    • actionlint (tech debt)
    • pre-commit hooks (tech debt)

Unrelated changes

  • Pay old tech debt
    • use | shortcut for Union types in formatter.py
    • use FQDN imports in formatter.py
    • Update Union type in formatter to |
    • immutable pre-commit actionlint
    • immutable pre-commit pre-commit hooks

@heitorlessa heitorlessa changed the title feat(logger): Add typing for log parameter in LambdaPowertoolsFormatter feat(logger): type log record in LambdaPowertoolsFormatter with TypedDict Jun 14, 2023
@heitorlessa heitorlessa merged commit 928e43a into aws-powertools:develop Jun 14, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Jun 14, 2023

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

sthulb pushed a commit that referenced this pull request Jun 19, 2023
…Dict (#2419)

Co-authored-by: erikayao93 <[email protected]>
Co-authored-by: Heitor Lessa <[email protected]>
Co-authored-by: Leandro Damascena <[email protected]>
rafaelgsr pushed a commit to rafaelgsr/aws-lambda-powertools-python that referenced this pull request Jun 30, 2023
…Dict (aws-powertools#2419)

Co-authored-by: erikayao93 <[email protected]>
Co-authored-by: Heitor Lessa <[email protected]>
Co-authored-by: Leandro Damascena <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation feature New feature or functionality internal Maintenance changes logger size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Add typing for log parameter in LambdaPowertoolsFormatter
4 participants