fix: telemetry recording on corrupt entries and exception log level - #28
emiliano-go wants to merge 5 commits into
Conversation
tishun
added
needs-triage
tishun
left a comment
tishun
left a comment
There was a problem hiding this comment.
Hey, thanks for the contribution!
Can you also make sure you run uv run nox to make sure lint/format/mypy cleaned up
|
Willco! |
Address tishun's review on PR redis#28: - Route all telemetry metric failures through a log-once pattern: the first failure is surfaced as a WARNING, repeats fall back to DEBUG to avoid log spam. - Apply the same handling to rate-limit telemetry. - Replace weak tests with real regression tests: corrupt entry records exactly one miss (never hit) and the cache.get span never sets cache.hit True; metric failures log one WARNING then DEBUG.
382135e to
9ea0e8d
Compare
|
Log-once pattern: all telemetry recording failures are now routed through a single mechanism (_warn_once), the first failure surfaces as a WARNING, and repeats fall back to DEBUG so operators notice the problem without log spam. This applies to the cache metrics and the rate-limit metrics (record_rate_limit_request/record_rate_limit_latency), which previously only logged at DEBUG. Tests rewritten as real regression tests: instead of tests that passed without the fix, the suite now has genuine regression coverage:
Also fixed the pre-existing lint failures (unused/unsorted imports) and the Python 3.10 module-shadowing issue in the tests by patching via importlib. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
==========================================
+ Coverage 96.78% 97.16% +0.38%
==========================================
Files 12 12
Lines 1214 1271 +57
==========================================
+ Hits 1175 1235 +60
+ Misses 39 36 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Parametrize the telemetry log-once regression test across all six metric helpers (cache request/eviction/write/latency and rate-limit request/latency), asserting exactly one WARNING on the first failure and DEBUG on repeats. This also covers record_rate_limit_latency, which Codecov flagged as the single uncovered line in telemetry.py.
- Change except clause from (json.JSONDecodeError, KeyError) to Exception in cache-hit path so TypeError, AttributeError, and other exceptions from corrupt entries are caught gracefully - Prevents 500 errors when binary garbage or partial JSON is stored in Redis - Add tests for corrupt entry recovery with binary data and non-JSON strings
3 participants
Changes
Bug: Corrupt cache entry records double telemetry (hit + miss)
record_cache_request(result="hit")and the spancache.hitattribute wereemitted before deserializing the cached response. When the entry had
corrupt JSON, the exception was caught, execution fell through to the MISS
path, and a second metric was recorded.
Fix: Deserialize first, then emit telemetry only on success. If
deserialization fails only a single "miss" metric is recorded.
Bug: Swallowed telemetry exceptions logged at DEBUG level
OpenTelemetry metric helpers (
record_cache_request,record_cache_eviction,record_cache_write,record_cache_latency) swallowed all exceptions atlogger.debug(...), making OTel errors invisible in production withoutdebug logging enabled.
Fix: Changed all four handlers to
logger.warning(...).