Skip to content

fix(django): trace cache.add operations#6403

Draft
immanuwell wants to merge 1 commit into
getsentry:masterfrom
immanuwell:fix/django-cache-add-span
Draft

fix(django): trace cache.add operations#6403
immanuwell wants to merge 1 commit into
getsentry:masterfrom
immanuwell:fix/django-cache-add-span

Conversation

@immanuwell
Copy link
Copy Markdown

Refs #6402

cache.add() was a blind spot. cache.set() emitted cache.put, cache.add() emitted nothing. kinda odd for a normal Django cache write path.

Repro

from django.core.cache import cache
import sentry_sdk
from sentry_sdk.integrations.django import DjangoIntegration

sentry_sdk.init(
    integrations=[DjangoIntegration(cache_spans=True)],
    traces_sample_rate=1.0,
)

with sentry_sdk.start_transaction(name="t", op="test"):
    cache.add("k", "value")

Before this patch: no cache span.
After this patch: one cache.put span for cache.add().

What changed

  • instrument cache.add()
  • treat it as cache.put
  • add a regression test for static + stream mode

Tested

  • tox -e py3.12-django-v4.2.30 -- -k test_cache_spans_add

I also checked a wider Django cache test slice, but this env doesnt have the repo's local Postgres test service, so the db-marked Django tests bail early.

@immanuwell immanuwell force-pushed the fix/django-cache-add-span branch from 5512f0e to 01bfc73 Compare May 26, 2026 04:42
Copy link
Copy Markdown
Member

@ericapisani ericapisani left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR! These changes are overall looking good, just need to update something in the span streaming test before we look to get it across the finish line.

if span_streaming:
items = capture_items("span")

with sentry_sdk.start_transaction(name="cache-add", op="test"):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the context of span streaming, we no longer have the concept of a transaction, we instead have a segment.

Because of that, we shouldn't be invoking this method, but if we need to create a span to attach the generated cache span to, use with sentry_sdk.traces.start_span(...) instead.

The assertions from lines 575 to 579 will likely need to be updated as a result, as there will be another span caught by the capture_items line above, so the index value used will change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, just adapt the test to follow the existing pattern in the same file @immanuwell

@alexander-alderman-webb
Copy link
Copy Markdown
Contributor

Please merge master into your branch as well to resolve unrelated CI failures.

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.

3 participants