fix(django): trace cache.add operations#6403
Conversation
5512f0e to
01bfc73
Compare
ericapisani
left a comment
There was a problem hiding this comment.
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"): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, just adapt the test to follow the existing pattern in the same file @immanuwell
|
Please merge master into your branch as well to resolve unrelated CI failures. |
Refs #6402
cache.add()was a blind spot.cache.set()emittedcache.put,cache.add()emitted nothing. kinda odd for a normal Django cache write path.Repro
Before this patch: no cache span.
After this patch: one
cache.putspan forcache.add().What changed
cache.add()cache.putTested
tox -e py3.12-django-v4.2.30 -- -k test_cache_spans_addI 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.