feat(redis): Set db.query.text and cache.key span attributes#6639
feat(redis): Set db.query.text and cache.key span attributes#6639ericapisani wants to merge 1 commit into
Conversation
When using span streaming, set SPANDATA.DB_QUERY_TEXT on db.redis spans and SPANDATA.CACHE_KEY on cache spans. These were already set as the span description/name but were missing from the attributes dict in the streaming path. Refs PY-2549 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| @@ -116,6 +116,7 @@ def sentry_patched_execute_command( | |||
| attributes={ | |||
| "sentry.op": cache_properties["op"], | |||
| "sentry.origin": SPAN_ORIGIN, | |||
There was a problem hiding this comment.
Bug: The cache span is initialized with SPANDATA.CACHE_KEY set to the description string, not the key tuple, causing an incorrect attribute type in exception paths.
Severity: LOW
Suggested Fix
Modify the span creation to use the correct property from the start. Instead of setting SPANDATA.CACHE_KEY to cache_properties["description"], set it to cache_properties["key"]. This ensures the attribute has the correct type (a tuple of strings) from the moment of initialization, even in exception paths.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: sentry_sdk/integrations/redis/_sync_common.py#L118
Potential issue: The code in `_sync_common.py` and `_async_common.py` incorrectly
initializes the `SPANDATA.CACHE_KEY` attribute on the cache span with
`cache_properties["description"]`, which is a string. The correct value should be
`cache_properties["key"]`, a tuple of strings. While a subsequent call to
`_set_cache_data` overwrites this with the correct value in the normal execution flow,
this correction does not happen if an exception is raised during the Redis command
execution. In such an exception scenario, the span would be captured with the wrong
attribute type (a string instead of a list of strings), leading to inconsistent
telemetry data.
Also affects:
sentry_sdk/integrations/redis/_async_common.py:119~119
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov Results 📊✅ 91401 passed | ⏭️ 6136 skipped | Total: 97537 | Pass Rate: 93.71% | Execution Time: 332m 56s 📊 Comparison with Base Branch
✨ No test changes detected All tests are passing successfully. ✅ Patch coverage is 100.00%. Project has 2403 uncovered lines. Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
- Coverage 89.90% 89.89% -0.01%
==========================================
Files 192 192 —
Lines 23763 23763 —
Branches 8206 8206 —
==========================================
+ Hits 21362 21360 -2
- Misses 2401 2403 +2
- Partials 1344 1346 +2Generated by Codecov Action |
When using the span streaming path,
db.query.textwas not being set ondb.redisspans andcache.keywas not being set on cache spans. Both values were already computed and used as the span description/name, but were missing from the attributes dict passed to the span constructor.This adds
SPANDATA.DB_QUERY_TEXTto db spans andSPANDATA.CACHE_KEYto cache spans in both the sync and async Redis common modules, and extends existing tests to assert these attributes are present.Fixes PY-2549
Fixes #6638