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(event-sources): cache parsed json in data class #909

Merged
merged 10 commits into from
Dec 21, 2021

Conversation

michaelbrewer
Copy link
Contributor

@michaelbrewer michaelbrewer commented Dec 19, 2021

Issue #, if available:

  • #844

Description of changes:

A micro optimization to cache the parsed json within the event source data class

Checklist

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

A micro optimization to cache the parsed json within the event source data class
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 19, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2021

Codecov Report

Merging #909 (7a623f2) into develop (cd15ee9) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #909      +/-   ##
===========================================
+ Coverage    99.90%   99.92%   +0.01%     
===========================================
  Files          118      118              
  Lines         5246     5255       +9     
  Branches       596      600       +4     
===========================================
+ Hits          5241     5251      +10     
  Misses           1        1              
+ Partials         4        3       -1     
Impacted Files Coverage Δ
...wertools/utilities/data_classes/active_mq_event.py 100.00% <100.00%> (ø)
.../utilities/data_classes/code_pipeline_job_event.py 100.00% <100.00%> (ø)
...lambda_powertools/utilities/data_classes/common.py 100.00% <100.00%> (ø)
...wertools/utilities/data_classes/rabbit_mq_event.py 100.00% <100.00%> (ø)
.../utilities/data_classes/cognito_user_pool_event.py 100.00% <0.00%> (+0.26%) ⬆️

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 cd15ee9...7a623f2. Read the comment docs.

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.

I'm fine with this solution - better than actual caching as UX and memory would've been tricky.

As we're gonna do it, let's revisit all other fields that do any sort of deserialisation -- Kinesis is one that does it heavy (json + b64) etc

@michaelbrewer
Copy link
Contributor Author

@heitorlessa ok. I will update others like this too.

@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 Dec 20, 2021
@michaelbrewer
Copy link
Contributor Author

@heitorlessa - ok i have added various singleton caching options.

@brysontyrrell
Copy link

This looks great! Looking forward to applying it.

@heitorlessa heitorlessa added the p1 label Dec 20, 2021
@heitorlessa
Copy link
Contributor

I was gonna suggest a decorator but given the amount of fields we're fine. If we get a feature request to optimize other fields that require expensive operation like decoding & unzipping CloudWatch Logs record, or decoding and JSON parsing Kinesis records, then we should refactor this to allow for growth.

Future note: If more fields are required, we should convert this into a reusable one-item @cache class decorator. This will make it easier to maintain and support any property we might want like a base64/gzip operation, e.g. @cache would keep the property name as key and its value in memory, cache["property_name"] = return_value

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.

Just one question on the usefulness of tests; happy to merge otherwise

@michaelbrewer
Copy link
Contributor Author

@heitorlessa - was thinking of a @cache decorator, might end up in the future or just use a builtin python3 feature in the future. it is minor right now right?

@heitorlessa
Copy link
Contributor

@heitorlessa - was thinking of a @cache decorator, might end up in the future or just use a builtin python3 feature in the future. it is minor right now right?

Like this but we don't need the threading safety I believe, as it'll compute once and more lightweight and faster than the LRU cache - once we deprecate older Python versions we can use it natively.

We would need to test whether that work for our data types tho, but should be easily adjustable as the logic is simple

https://github.com/python/cpython/blob/e9a01e231aae19fd1249368e477a60bc033b2646/Lib/functools.py#L955

@heitorlessa
Copy link
Contributor

Merging as is - I'll look into the hash dunder and decorator when Mypy is fully supported across the codebase

@heitorlessa heitorlessa added feature New feature or functionality area/event_sources labels Dec 21, 2021
@heitorlessa heitorlessa merged commit 9abeb32 into aws-powertools:develop Dec 21, 2021
@michaelbrewer michaelbrewer deleted the feat-844 branch December 23, 2021 21:23
heitorlessa added a commit to ran-isenberg/aws-lambda-powertools-python that referenced this pull request Dec 31, 2021
…tools-python into complex

* 'develop' of https://github.com/awslabs/aws-lambda-powertools-python: (24 commits)
  docs: consistency around admonitions and snippets (aws-powertools#919)
  chore(deps-dev): bump mypy from 0.920 to 0.930 (aws-powertools#925)
  fix(event-sources): handle dynamodb null type as none, not bool (aws-powertools#929)
  fix(apigateway): support @app.not_found() syntax & housekeeping (aws-powertools#926)
  docs: Added GraphQL Sample API to Examples section of README.md (aws-powertools#930)
  feat(idempotency): support dataclasses & pydantic models payloads (aws-powertools#908)
  feat(tracer): ignore tracing for certain hostname(s) or url(s) (aws-powertools#910)
  feat(event-sources): cache parsed json in data class (aws-powertools#909)
  fix(warning): future distutils deprecation (aws-powertools#921)
  docs(batch): remove leftover from legacy
  docs(layer): bump Lambda Layer to version 6
  chore: bump to 1.23.0
  docs(apigateway): add new not_found feature (aws-powertools#915)
  docs: external reference to cloudformation custom resource helper (aws-powertools#914)
  feat(logger): allow handler with custom kwargs signature (aws-powertools#913)
  chore: minor housekeeping before release (aws-powertools#912)
  chore(deps-dev): bump mypy from 0.910 to 0.920 (aws-powertools#903)
  feat(batch): new BatchProcessor for SQS, DynamoDB, Kinesis (aws-powertools#886)
  fix(parser): overload parse when using envelope (aws-powertools#885)
  fix(parser): kinesis sequence number is str, not int (aws-powertools#907)
  ...
heitorlessa added a commit to huonw/aws-lambda-powertools-python that referenced this pull request Dec 31, 2021
…tools-python into feature/905-datetime

* 'develop' of https://github.com/awslabs/aws-lambda-powertools-python:
  feat(feature_flags): support beyond boolean values (JSON values) (aws-powertools#804)
  docs: consistency around admonitions and snippets (aws-powertools#919)
  chore(deps-dev): bump mypy from 0.920 to 0.930 (aws-powertools#925)
  fix(event-sources): handle dynamodb null type as none, not bool (aws-powertools#929)
  fix(apigateway): support @app.not_found() syntax & housekeeping (aws-powertools#926)
  docs: Added GraphQL Sample API to Examples section of README.md (aws-powertools#930)
  feat(idempotency): support dataclasses & pydantic models payloads (aws-powertools#908)
  feat(tracer): ignore tracing for certain hostname(s) or url(s) (aws-powertools#910)
  feat(event-sources): cache parsed json in data class (aws-powertools#909)
  fix(warning): future distutils deprecation (aws-powertools#921)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or functionality p1 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.

None yet

4 participants