feat(redis): Support mget command in caching functionality
#12348
Conversation
size-limit report 📦
|
| 'db.statement': 'get ioredis-cache:unavailable-data', | ||
| 'cache.hit': false, | ||
| 'cache.key': 'ioredis-cache:unavailable-data', | ||
| 'network.peer.address': 'localhost', | ||
| 'network.peer.port': 6379, | ||
| }), | ||
| }), | ||
| // MGET | ||
| expect.objectContaining({ | ||
| description: 'test-key,ioredis-cache:test-key,ioredis-cache:unavailable-data', |
There was a problem hiding this comment.
If I understood correctly these should have a space too, so:
| description: 'test-key,ioredis-cache:test-key,ioredis-cache:unavailable-data', | |
| description: 'test-key, ioredis-cache:test-key, ioredis-cache:unavailable-data', |
is the expected outcome?
| span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, cacheItemSize > 0); | ||
|
|
||
| span.setAttributes({ | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_OP]: cacheOperation, |
There was a problem hiding this comment.
I guess we can combine this into on setAttributes() call here :)
| [SEMANTIC_ATTRIBUTE_CACHE_KEY]: safeKey, | ||
| }); | ||
|
|
||
| span.updateName(`${safeKey.substring(0, 1024)}...`); |
There was a problem hiding this comment.
I think this is not correct, this would always add ... even if this is not too long, right...? 😅
| keys.push(subArg.toString()); | ||
| } else if (Buffer.isBuffer(subArg)) { | ||
| keys.push(subArg.toString()); | ||
| } |
There was a problem hiding this comment.
We should probably add an else block here adding something like < unknown > or so, that you at least see how many things there are? Otherwise you may see e.g.
cache:x, cache:y when in reality it may be cache:x, cache:y, cache:BINARY_THINK?
| /** Determines whether a redis operation should be considered as "cache operation" by checking if a key is prefixed. | ||
| * We only support certain commands (such as 'set', 'get', 'mget'). */ | ||
| export function shouldConsiderForCache(redisCommand: string, key: string, prefixes: string[]): boolean { | ||
| const lowercaseCommand = redisCommand.toLowerCase(); |
There was a problem hiding this comment.
Could we do:
if (!getCacheOperation(redisCommand)) {
return false;
}Instead of the first part of the check here? Keeping this in one place :)
| return false; | ||
| } | ||
|
|
||
| return key.reduce((prev, key) => { |
There was a problem hiding this comment.
This seems a bit excessive use of reduce :D I'd avoid it where not absolutely necessary, here an early return is easier to follow IMHO:
for (const k of key) {
if (keyHasPrefix(key, prefixes) {
return true;
}
}
return false;or something along this line?
| const dbSystem = attributes[SEMATTRS_DB_SYSTEM]; | ||
| if (dbSystem) { | ||
| const opIsCache = | ||
| typeof attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] === 'string' && |
There was a problem hiding this comment.
did we end up reverting to this on purpose, vs. doing opIsCache = typeof attributes['cache.hit'] === 'boolean'? :) If we can avoid string operations here I think it would be preferable!
There was a problem hiding this comment.
Yes I reverted it as cache.hit is only added for a get operation.
|
|
||
| // If db.type exists then this is a database call span | ||
| // If the Redis DB is used as a cache, the span description should not be changed | ||
| if (dbSystem && (!attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] || !opIsCache)) { |
There was a problem hiding this comment.
| if (dbSystem && (!attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] || !opIsCache)) { | |
| if (dbSystem && !opIsCache) { |
feat(redis): Support `mget` command in caching functionality
Added support for Redis statement
mget. Refactored the code to make it work with array-based keys. Moved cache-related functions toutilsand added unit tests for those functions.