Skip to content
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

Redis span ops marked as 'db' instead of 'cache' #12058

Open
3 tasks done
rahul-kumar-saini opened this issue May 16, 2024 · 9 comments
Open
3 tasks done

Redis span ops marked as 'db' instead of 'cache' #12058

rahul-kumar-saini opened this issue May 16, 2024 · 9 comments

Comments

@rahul-kumar-saini
Copy link

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

8.0.0

Framework Version

Node v18.18.2

Link to Sentry event

No response

SDK Setup

No response

Steps to Reproduce

Just initialize Sentry and use ioredis and run some Redis commands.

Expected Result

Redis span ops being cache or cache.[get|set|del] or something.

Actual Result

Redis spans marked as db, which in turn populates the SQL Queries view with Redis commands and makes it less useful. Would be cool to have a dedicated Cache usage view similar to the Queries view where Redis commands could be aggregated instead.

@s1gr1d
Copy link
Member

s1gr1d commented May 16, 2024

Hello, thanks for writing! We are indeed working on a feature which let's you define specific prefixes for your cache keys. The API will possibly look something like this: integrations: [Sentry.redisIntegration({ cachePrefixes: ['cache:'] })].

If the key is prefixed with one of the defined prefixes, the ops will be marked as cache.[..].

@rahul-kumar-saini
Copy link
Author

Hi @s1gr1d! Is there any way for me to also only have certain commands instrumented? Eg let's say I only care for get/set/delete and don't care for other Redis commands?

@s1gr1d
Copy link
Member

s1gr1d commented May 17, 2024

Currently, there isn't. Would you want to specify certain Redis commands to be instrumented in general or just when you're using Redis as a cache? Meaning, Redis would be instrumented for all commands but only when a key is prefixed with e.g. cache: we would drop commands which are not specified.

@s1gr1d
Copy link
Member

s1gr1d commented May 17, 2024

If you want, you can already try adding cache prefixes to the integration with the latest version of Sentry. Currently they pick up get and set.

@rahul-kumar-saini
Copy link
Author

Would you want to specify certain Redis commands to be instrumented in general or just when you're using Redis as a cache?

We use Redis for a couple different things, eg sometimes we use EVAL to run a script. Let's say I'm only interested in gets/sets/deletes in general across Redis, is that something I can currently do using the cache prefixes?

Meaning, Redis would be instrumented for all commands but only when a key is prefixed with e.g. cache: we would drop commands which are not specified.

I'm not sure what this means - if I set { cachePrefixes: ['cache.'] } I assume a get span will have op cache.get, but what would something like PUBLISH or EVALSHA have? cache.publish and cache.evalsha? Or would those be dropped?

Since cachePrefixes is a list of strings, what would having multiple strings there do? Eg what is the expected behavior of setting { cachePrefixes: ['cache.', 'redis.'] }?

@mydea
Copy link
Member

mydea commented May 21, 2024

So cachePrefixes defines a list of prefixes, if any of them is matched we will ensure these spans will show up as cache spans in the UI. So e.g. { cachePrefixes: ['cache.', 'redis.'] } means that e.g. these: cache.publish or redis.publish will be shown as cache spans, but other.publish will not. Does this make sense?

@rahul-kumar-saini
Copy link
Author

rahul-kumar-saini commented May 22, 2024

So cachePrefixes defines a list of prefixes, if any of them is matched we will ensure these spans will show up as cache spans in the UI. So e.g. { cachePrefixes: ['cache.', 'redis.'] } means that e.g. these: cache.publish or redis.publish will be shown as cache spans, but other.publish will not. Does this make sense?

I see, so what this sounds like to me is that anything I specify as cache.something will show up in the Caches performance UI if I have "cache" in my cachePrefixes. Going back to my original issue however, the Redis integration (as of SDK 8.2.1 at least) marks ioredis commands as db.<command> instead of redis.<command> or cache.<command>.

So even if I set cache prefixes, it probably won't resolve that issue?

@rahul-kumar-saini
Copy link
Author

It looks like the docs on cache prefixes are describing an entirely different usage. The docs are saying the cache prefixes array is used for filtering what the cache key itself is prefixed by, rather than the span op of the cache span?

image

@lforst
Copy link
Member

lforst commented May 24, 2024

@rahul-kumar-saini So in general the docs you are referring to are our "develop" docs which are not really intended for end-user consumption but rather for SDK maintainers, so take everything in there with a grain of salt because it is often idealistic and the reality looks different.

The docs are saying the cache prefixes array is used for filtering what the cache key itself is prefixed by, rather than the span op of the cache span?

The docs here say that if you have a cache invocation, and the cache key matches one of the prefixes, its db span should be wrapped in a cache span. In reality, what this will mean for the JS SDK is that the cache span will be wrapped by the db span due to a technical limitation. If the cache key doesn't match any of the prefixes, a cache span will not be created, the db span will stay. This is so that you aren't getting random redis spans in your caches view when you don't use redis as a cache. All redis operations will however still likely show up in the queries view.

The caches view will launch soon and we are just in the process of building redis integrations. They are still very much experimental.

cc @AbhiPrasad because he is looking at this soon

@AbhiPrasad AbhiPrasad self-assigned this May 24, 2024
@AbhiPrasad AbhiPrasad added this to the v8 Instrumentation Bugs milestone May 27, 2024
@lforst lforst assigned s1gr1d and unassigned AbhiPrasad Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

5 participants