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

feat(spans): disable redis span metrics extraction #3600

Merged
merged 4 commits into from
May 16, 2024

Conversation

DominikB2014
Copy link
Contributor

@DominikB2014 DominikB2014 commented May 15, 2024

We originally re-enabled db.redis in order to collect metrics for the cache module. However, since then we decided that the cache module will only look at cache.* span ops.

Therefore we can stop collecting db.redis span metrics.

@DominikB2014 DominikB2014 marked this pull request as ready for review May 15, 2024 16:31
@DominikB2014 DominikB2014 requested a review from a team as a code owner May 15, 2024 16:31
Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

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

Should we also remove some of the redis spans from the test_extract_span_metrics test?

@DominikB2014
Copy link
Contributor Author

Should we also remove some of the redis spans from the test_extract_span_metrics test?

@Dav1dde I'm indifferent to this, I could see us needing the db.redis spans in the future, so I'm okay with keeping these spans in the tests, if we re-add them back we'll have something to test against. Also having them there does verify that the metrics are not indeed being extracted.

@Dav1dde
Copy link
Member

Dav1dde commented May 16, 2024

Sounds good to me, I don't have an issue with keeping them around 👍

@DominikB2014 DominikB2014 enabled auto-merge (squash) May 16, 2024 15:45
@DominikB2014 DominikB2014 merged commit fb06249 into master May 16, 2024
22 checks passed
@DominikB2014 DominikB2014 deleted the DominikB2014/dont-ingest-redis branch May 16, 2024 16:06
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