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

Changed cache monkey-patching for Django 3.2+ #1497

Merged
merged 3 commits into from
Aug 28, 2021

Conversation

tim-schilling
Copy link
Contributor

@tim-schilling tim-schilling commented Aug 27, 2021

Changed caching monkey-patching for Django3.2+ to iterate over existing
caches and patch them individually rather than attempting to patch
django.core.caches as a whole. The middleware.cache is still
being patched as a whole in order to attempt to catch any cache
usages before enable_instrumentation is called.

Fixes #1496

Big thanks to @LiorA1 for reporting the issue and creating a reproducible test case.

@tim-schilling tim-schilling changed the title Changed cache monkey-patching approach. Changed cache monkey-patching for Django 3.2+ Aug 27, 2021
@codecov
Copy link

codecov bot commented Aug 27, 2021

Codecov Report

Merging #1497 (060860a) into main (6d3eb44) will decrease coverage by 0.89%.
The diff coverage is 39.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1497      +/-   ##
==========================================
- Coverage   86.61%   85.72%   -0.90%     
==========================================
  Files          35       35              
  Lines        1868     1891      +23     
  Branches      262      272      +10     
==========================================
+ Hits         1618     1621       +3     
- Misses        178      190      +12     
- Partials       72       80       +8     
Impacted Files Coverage Δ
debug_toolbar/panels/cache.py 72.56% <39.39%> (-9.71%) ⬇️

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 6d3eb44...060860a. Read the comment docs.

@tim-schilling
Copy link
Contributor Author

Shoot. This still needs to monkey-patch django.core.cache.caches for other caches that get created after enable_instrumentation is called.

@tim-schilling
Copy link
Contributor Author

Shoot. This still needs to monkey-patch django.core.cache.caches for other caches that get created after enable_instrumentation is called.

That would require changing the settings which is a no-no. So I was right the first time.

@tim-schilling tim-schilling force-pushed the low-level-cache branch 2 times, most recently from 5323013 to f282050 Compare August 28, 2021 00:50
@tim-schilling
Copy link
Contributor Author

The reduction in code coverage is because our report doesn't handle the coverage across different versions (I think).

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

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

The reduction in code coverage is because our report doesn't handle the coverage across different versions (I think).

I can't say for sure but I think you're right.

docs/changes.rst Outdated Show resolved Hide resolved
docs/changes.rst Outdated Show resolved Hide resolved
tim-schilling and others added 3 commits August 28, 2021 10:31
Changed cache monkey-patching for Django 3.2+ to iterate over existing
caches and patch them individually rather than attempting to patch
django.core.caches as a whole. The middleware.cache is still
being patched as a whole in order to attempt to catch any cache
usages before enable_instrumentation is called.

Test cache panel is diabled for middleware before it.
Co-authored-by: Matthias Kestenholz <mk@feinheit.ch>
@tim-schilling tim-schilling merged commit 6cc6265 into jazzband:main Aug 28, 2021
@tim-schilling tim-schilling deleted the low-level-cache branch August 28, 2021 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DJDT not showing low level caching correctly in Docker
2 participants