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

Add cache.hit and cache.item_size to Django #2057

Merged
merged 28 commits into from
May 5, 2023

Conversation

antonpirker
Copy link
Member

@antonpirker antonpirker commented Apr 28, 2023

In Django we want to add information to spans if a configured cache was hit or missed and if hit what the item_size of the object in the cache was.

Fixes #2043

@antonpirker antonpirker marked this pull request as ready for review May 4, 2023 06:45
Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

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

lgtm!

sentry_sdk/integrations/django/caching.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/django/caching.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/django/caching.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/django/caching.py Show resolved Hide resolved
sentry_sdk/integrations/django/caching.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/django/caching.py Outdated Show resolved Hide resolved
Copy link
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

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

wrong return value

sentry_sdk/integrations/django/caching.py Outdated Show resolved Hide resolved
Co-authored-by: Neel Shah <neel.shah@sentry.io>
@antonpirker antonpirker enabled auto-merge (squash) May 5, 2023 11:40
@antonpirker antonpirker merged commit efa55d3 into master May 5, 2023
233 checks passed
@antonpirker antonpirker deleted the antonpirker/2403-cache-hit-django branch May 5, 2023 11:46
with hub.start_span(op=OP.CACHE, description=description) as span:
value = original_method(*args, **kwargs)

if value:
Copy link

@nicootto nicootto May 9, 2023

Choose a reason for hiding this comment

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

@antonpirker you can't call __bool__ of the returned value, it might not be implemented. You should check if a default value was returned, in which case it is a cache miss, or if there is not default value configured you should check value is not None.

I recently reported the same issue on ddtrace, you can check of examples on how to reproduce there DataDog/dd-trace-py#5460 and here is how they handle it DataDog/dd-trace-py#5477

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.

Add cache.hit data to Django spans
4 participants