Django caching instrumentation update #3009
Conversation
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Have some questions and suggestions
|
|
||
| original_method = getattr(cache, method_name) | ||
|
|
||
| @ensure_integration_enabled(DjangoIntegration, original_method) |
There was a problem hiding this comment.
| @ensure_integration_enabled(DjangoIntegration, original_method) |
I think it makes more sense to place this on the sentry_method function. It is inaccurate to say that this method maps original_method because it has a different signature. Although, perhaps it would be more appropriate to make this change in a separate PR.
There was a problem hiding this comment.
Yes, probably better a new PR. Can you please create a ticket @szokeasaurusrex ?
|
|
||
| return value | ||
|
|
||
| @functools.wraps(original_method) |
There was a problem hiding this comment.
| @functools.wraps(original_method) | |
| @ensure_integration_enabled(DjangoIntegration, original_method) |
To be applied alongside my comment on (original version) line 41; also, likely makes sense to do in separate PR.
Co-authored-by: Daniel Szoke <[email protected]>
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
I found some logic that appears to be broken – this should be fixed, and we should also add tests for it.
Besides that, I have also added some more minor suggestions and questions.
Co-authored-by: Daniel Szoke <[email protected]>
…try/sentry-python into antonpirker/django-caching-module
antonpirker
dismissed
szokeasaurusrex’s stale review
All the changes where address from Daniels review, but he is out of office for some time, so I will dismiss this review so I can merge this.
This adds more data to the cache spans and makes adding the cache item size optional. This implements parts of following spec https://develop.sentry.dev/sdk/performance/modules/cache/ --------- Co-authored-by: Daniel Szoke <[email protected]>
This adds more data to the cache spans and makes adding the cache item size optional.
This implements parts of following spec https://develop.sentry.dev/sdk/performance/modules/cache/
Documentation for manual instrumentation for the Cache module can be found here:
getsentry/sentry-docs#9926
Load testing of the feature was done here: #3058
Fixes #2964