Skip to content

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

Merged
merged 15 commits into from
Jun 6, 2024

Conversation

s1gr1d
Copy link
Member

@s1gr1d s1gr1d commented Jun 4, 2024

Added support for Redis statement mget. Refactored the code to make it work with array-based keys. Moved cache-related functions to utils and added unit tests for those functions.

@s1gr1d s1gr1d requested review from mydea, lforst, Lms24 and AbhiPrasad June 4, 2024 12:09
Copy link
Contributor

github-actions bot commented Jun 4, 2024

size-limit report 📦

Path Size
@sentry/browser 21.74 KB (0%)
@sentry/browser (incl. Tracing) 32.77 KB (0%)
@sentry/browser (incl. Tracing, Replay) 68.34 KB (0%)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 61.65 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas) 72.41 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) 84.5 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 86.36 KB (0%)
@sentry/browser (incl. metrics) 25.92 KB (0%)
@sentry/browser (incl. Feedback) 37.89 KB (0%)
@sentry/browser (incl. sendFeedback) 26.32 KB (0%)
@sentry/browser (incl. FeedbackAsync) 30.86 KB (0%)
@sentry/react 24.51 KB (0%)
@sentry/react (incl. Tracing) 35.81 KB (0%)
@sentry/vue 25.73 KB (0%)
@sentry/vue (incl. Tracing) 34.61 KB (0%)
@sentry/svelte 21.87 KB (0%)
CDN Bundle 23.11 KB (0%)
CDN Bundle (incl. Tracing) 34.5 KB (0%)
CDN Bundle (incl. Tracing, Replay) 68.43 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) 73.6 KB (0%)
CDN Bundle - uncompressed 68 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed 102.18 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed 212.07 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 224.54 KB (0%)
@sentry/nextjs (client) 35.16 KB (0%)
@sentry/sveltekit (client) 33.4 KB (0%)
@sentry/node 115.48 KB (+0.21% 🔺)
@sentry/node - without tracing 94.58 KB (+0.03% 🔺)
@sentry/aws-serverless 103.75 KB (+0.03% 🔺)

'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',
Copy link
Member

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:

Suggested change
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?

Comment on lines 59 to 62
span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, cacheItemSize > 0);

span.setAttributes({
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: cacheOperation,
Copy link
Member

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)}...`);
Copy link
Member

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());
}
Copy link
Member

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();
Copy link
Member

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 :)

@s1gr1d s1gr1d requested a review from mydea June 6, 2024 08:22
@s1gr1d s1gr1d enabled auto-merge June 6, 2024 08:43
return false;
}

return key.reduce((prev, key) => {
Copy link
Member

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' &&
Copy link
Member

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!

Copy link
Member Author

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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (dbSystem && (!attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP] || !opIsCache)) {
if (dbSystem && !opIsCache) {

Copy link
Member

@mydea mydea left a 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 🚀

@s1gr1d s1gr1d merged commit 435141e into develop Jun 6, 2024
110 checks passed
@s1gr1d s1gr1d deleted the sig/ioredis-statements branch June 6, 2024 09:15
billyvg pushed a commit that referenced this pull request Jun 10, 2024
feat(redis): Support `mget` command in caching functionality
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants