search

cloud-custodian / cel-python

Pure Python implementation of the Common Expression Language
Apache License 2.0
99 stars 20 forks source link

Updated all logging to be lazy-loaded #36

Closed eandersson closed 1 year ago

eandersson commented 2 years ago

The current logging isn't lazy-loaded. This means that all statements are evaluated even with logging disabled. This can have pretty a measurable impact on performance.

linux-foundation-easycla[bot] commented 2 years ago

CLA Signed

The committers listed above are authorized under a signed CLA.

nosammai commented 2 years ago

Testing with the provided benchmark, this makes a huge difference in performance:

Before fix:

❯ python3.8 benches/large_resource_set.py TagAssetBenchmark
Filter    : resource["IamInstanceProfile"]["Arn"].matches("(.*)(?=Enterprise-Reserved-CloudCustodian.*)") && (resource["Tags"].filter(x, x["Key"] == "ASSET")[0]["Value"] != "CLOUDCUSTODIAN" && resource["Tags"].filter(x, x["Key"] == "ASSET")[0]["Value"] != "CLOUDCORESERVICES" && present(resource["Tags"].filter(x, x["Key"] == "ASSET")[0]["Value"]))
Resources : 1,000
Total Time: 19,423.9 ms
Range : 13.0 ms - 172.3 ms
Mean  : 19.19 ms
Median: 15.52 ms

After Fix:

❯ python3.8 benches/large_resource_set.py TagAssetBenchmark
Filter    : resource["IamInstanceProfile"]["Arn"].matches("(.*)(?=Enterprise-Reserved-CloudCustodian.*)") && (resource["Tags"].filter(x, x["Key"] == "ASSET")[0]["Value"] != "CLOUDCUSTODIAN" && resource["Tags"].filter(x, x["Key"] == "ASSET")[0]["Value"] != "CLOUDCORESERVICES" && present(resource["Tags"].filter(x, x["Key"] == "ASSET")[0]["Value"]))
Resources : 1,000
Total Time: 2,690.9 ms
Range : 2.0 ms - 8.9 ms
Mean  : 2.60 ms
Median: 2.45 ms
eandersson commented 1 year ago

Had to close and re-open PR to fix issues with SLA not being recognized.

kapilt commented 1 year ago

looks like one test failure on test_trace_decorator to address.

eandersson commented 1 year ago

looks like one test failure on test_trace_decorator to address.

Ops. Missed that one. Fixed.

codecov[bot] commented 1 year ago

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage :thumbsup:

Coverage data is based on head (6622fe6) compared to base (50f9e8b). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #36 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 8 8 Lines 2602 2601 -1 ========================================= - Hits 2602 2601 -1 ``` | [Impacted Files](https://codecov.io/gh/cloud-custodian/cel-python/pull/36?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloud-custodian) | Coverage Δ | | |---|---|---| | [src/celpy/\_\_init\_\_.py](https://codecov.io/gh/cloud-custodian/cel-python/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloud-custodian#diff-c3JjL2NlbHB5L19faW5pdF9fLnB5) | `100.00% <100.00%> (ø)` | | | [src/celpy/\_\_main\_\_.py](https://codecov.io/gh/cloud-custodian/cel-python/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloud-custodian#diff-c3JjL2NlbHB5L19fbWFpbl9fLnB5) | `100.00% <100.00%> (ø)` | | | [src/celpy/celtypes.py](https://codecov.io/gh/cloud-custodian/cel-python/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloud-custodian#diff-c3JjL2NlbHB5L2NlbHR5cGVzLnB5) | `100.00% <100.00%> (ø)` | | | [src/celpy/evaluation.py](https://codecov.io/gh/cloud-custodian/cel-python/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloud-custodian#diff-c3JjL2NlbHB5L2V2YWx1YXRpb24ucHk=) | `100.00% <100.00%> (ø)` | | | [src/xlate/c7n\_to\_cel.py](https://codecov.io/gh/cloud-custodian/cel-python/pull/36/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloud-custodian#diff-c3JjL3hsYXRlL2M3bl90b19jZWwucHk=) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloud-custodian). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cloud-custodian)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.