feat(node,server-utils): Set cache.key on dataloader spans and capture redis delete operations as cache.remove#22389
Conversation
size-limit report 📦
|
msonnb
changed the title
cache.key on dataloader spans and capture redis delete operations as cache.removecache.key on dataloader spans and capture redis delete operations as cache.remove
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0af75dc. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Small style/nit to use an undefined attribute in favor of a spread, but LGTM.
…der.ts Co-authored-by: isaacs <i@izs.me>
| } | ||
|
|
||
| return keyArg == null ? undefined : [String(keyArg)]; | ||
| } |
There was a problem hiding this comment.
Array keys mishandled on load
Medium Severity
getCacheKey uses Array.isArray to decide between a single key and a key list, but load can take an array as one composite key. In that case cache.key is expanded into multiple entries instead of one, so the span attribute no longer matches the actual dataloader key.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ece7d46. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d4fe083. Configure here.
| 'network.peer.address': 'localhost', | ||
| 'network.peer.port': 6383, | ||
| }), | ||
| }), |
There was a problem hiding this comment.
Missing remove size assertion
Low Severity
This violates the testing rule about expect.objectContaining when a payload must omit a field: the new DEL expectations never assert that cache.item_size is absent, even though remove responses are intentionally excluded from size calculation. A regression that sets size again would still pass. Flagged because it was mentioned in this rules file.
Additional Locations (2)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit d4fe083. Configure here.
3 participants
cache.keyattribute in dataloader instrumentation (otel and tracing channels)optocache.removefor redisDELandUNLINKcommandsCloses #22365