-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(redis): Support mget
command in caching functionality
#12348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (dbSystem && (!attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] || !opIsCache)) { | |
if (dbSystem && !opIsCache) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some small comments only, but overall looking great 🚀
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 toutils
and added unit tests for those functions.