sentry-javascript icon indicating copy to clipboard operation
sentry-javascript copied to clipboard

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

Open rahul-kumar-saini opened this issue 1 year ago • 9 comments

Is there an existing issue for this?

  • [X] I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
  • [X] I have reviewed the documentation https://docs.sentry.io/
  • [X] I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

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.

rahul-kumar-saini avatar May 16 '24 02:05 rahul-kumar-saini

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.[..].

s1gr1d avatar May 16 '24 06:05 s1gr1d

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?

rahul-kumar-saini avatar May 16 '24 21:05 rahul-kumar-saini

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 avatar May 17 '24 09:05 s1gr1d

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.

s1gr1d avatar May 17 '24 13:05 s1gr1d

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.'] }?

rahul-kumar-saini avatar May 17 '24 14:05 rahul-kumar-saini

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?

mydea avatar May 21 '24 15:05 mydea

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 avatar May 22 '24 20:05 rahul-kumar-saini

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

rahul-kumar-saini avatar May 22 '24 20:05 rahul-kumar-saini

@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

lforst avatar May 24 '24 07:05 lforst

We have two variants, that will be supported with the upcoming release. Each one instruments a set of redis commands.

  • cache.get: get, mget
  • cache.put: set, setex

All other commands will be handled as db spans. In regards to your question about other commands like evalsha: Do you use any other commands to interact with your cache (which ones)?

Currently, it is not possible to evaluate other commands than listed above as cache spans.

s1gr1d avatar Jun 07 '24 07:06 s1gr1d

Closing because I think this is solved and to clean up our issues stream. Feel free to reach out if you have any more questions!

lforst avatar Jun 24 '24 13:06 lforst