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

fix(metrics): remove Redis spans from Queries insights module #3795

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mjq
Copy link
Member

@mjq mjq commented Jul 5, 2024

Redis spans were intended to be filtered out of the Queries insights module, since it currently only supports SQL. This used to work via a check for *redis* in the span op. With the JS SDK's v8 release the span op for redis spans became simply db and Redis spans starting showing up in the Queries module.

JS v8 follows the OTel standard of setting the db.system attribute to "redis", so we can use that as a secondary check to bring back the expected filtering.

Redis spans were intended to be filtered out of the Queries insights module,
since it currently only supports SQL. This used to work via a check for
`*redis*` in the span op. With the JS SDK's v8 release the span op for redis
spans became simply `db` and Redis spans starting showing up in the Queries module.

JS v8 follows the OTel standard of setting the `db.system` attribute to
`"redis"`, so we can use that as a secondary check to bring back the expected
filtering.
@mjq mjq requested a review from gggritso July 5, 2024 18:27
@mjq mjq requested a review from a team as a code owner July 5, 2024 18:27
@gggritso
Copy link
Member

gggritso commented Jul 5, 2024

The strange thing is, I don't think db.redis was ever omitted from tag extraction, the only check for *redis* is when extracting the span.description tag! I think what's been happening is that we're extracting tags for db.redis but none of them have a span.description tag, so they don't show up in the module. In the V8 SDK the op is "db" which means the spans get metrics buckets and a span.description tag, so they show up in the UI all of a sudden.

The Cardinality Analyzer actually confirms this, there are metric buckets with the "db.redis" op. If true, we should ... probably stop extracting those, by adding "*redis*" to DISABLED_DATABASES?

Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

The PR makes sense to me, see also #3642.

+1 on what @gggritso said, db.redis should be in disabled databases.

@mjq could you add a test case to https://github.com/getsentry/relay/blob/master/relay-server/src/metrics_extraction/event.rs to confirm the new behavior?

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.

3 participants