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

New Relic doesn't track metrics for redis-clustering gem #2444

Closed
praveen-ks opened this issue Feb 15, 2024 · 11 comments · Fixed by #2720
Closed

New Relic doesn't track metrics for redis-clustering gem #2444

praveen-ks opened this issue Feb 15, 2024 · 11 comments · Fixed by #2720
Assignees
Labels
3 Story Point Estimate community To tag external issues and PRs submitted by the community

Comments

@praveen-ks
Copy link

praveen-ks commented Feb 15, 2024

redis-rb moved the cluster support to redis-clustering gem companion gem which internally uses redis-cluster-client gem. I am using ElastiCache cluster and upgrading redis-rb gem to latest version.

Issue: New Relic is tracking the data for connections created using redis-client gem but not for redis-cluster-client gem. What can I do to make New Relic work with redis cluster & redis-clustering gem.

Redis::Client support is there but Redis::Cluster::Client is not supported.

@workato-integration
Copy link

@github-actions github-actions bot added the community To tag external issues and PRs submitted by the community label Feb 15, 2024
@kford-newrelic kford-newrelic added estimate Issue needing estimation and removed bug labels Feb 16, 2024
@kford-newrelic kford-newrelic added 3 Story Point Estimate and removed estimate Issue needing estimation labels Feb 16, 2024
@kaylareopelle
Copy link
Contributor

Hi @praveen-ks, thanks for bringing this gap in our instrumentation to our attention.

To start collecting the same data you see for the redis-rb gem, the agent will need to add instrumentation for the redis-clustering and redis-cluster-client gems. We'll start with trying to provide the same data you see for the redis-rb gem when these new gems are in play.

Are there any features unique to the redis-clustering gem that you'd like to see instrumented?

@praveen-ks
Copy link
Author

Hi @praveen-ks, thanks for bringing this gap in our instrumentation to our attention.

To start collecting the same data you see for the redis-rb gem, the agent will need to add instrumentation for the redis-clustering and redis-cluster-client gems. We'll start with trying to provide the same data you see for the redis-rb gem when these new gems are in play.

Are there any features unique to the redis-clustering gem that you'd like to see instrumented?

That should be enough @kaylareopelle, nothing specific to redis-clustering gem.

@praveen-ks
Copy link
Author

praveen-ks commented Apr 3, 2024

@kaylareopelle I have tried adding the instrumentation for redis-clustering gem using:

Redis::Cluster::Client.prepend(NewRelic::Agent::Instrumentation::Redis::Prepend)

It worked, I was able to get redis-cluster connection calls data.

But it caused performance decline for my application. Because of this override in my application the redis connection calls become **~9-10 times** slower.

Let me know if I am missing anything.

Thanks in advance for any leads 🙏

@kaylareopelle
Copy link
Contributor

Hi @praveen-ks, I'm excited to hear that prepending helped! It's troubling to hear about the performance decline.

redis-client, a dependency of redis-cluster-client has a structure to register middleware which is encouraged over prepending for instrumentation on Redis 6+.

Could you try swapping the prepend call with the following, and let me know how it goes?
RedisClient.register(NewRelic::Agent::Instrumentation::RedisClient::Middleware)

@praveen-ks
Copy link
Author

praveen-ks commented Apr 3, 2024

Hi @praveen-ks, I'm excited to hear that prepending helped! It's troubling to hear about the performance decline.

redis-client, a dependency of redis-cluster-client has a structure to register middleware which is encouraged over prepending for instrumentation on Redis 6+.

Could you try swapping the prepend call with the following, and let me know how it goes? RedisClient.register(NewRelic::Agent::Instrumentation::RedisClient::Middleware)

I am using latest version of both redis and redis-clustering and this code is already taking care of registering middleware for RedisClient.

That's why I don't feel this can help or am I missing something here?

@kaylareopelle
Copy link
Contributor

Ah, I see. I didn't realize you had redis gem installed in addition to the redis-clustering gem.

If it was just the redis-clustering gem, the Redis instrumentation may not have installed on agent startup, preventing the middleware from being registered.

I'll take another look at the issue and see if I have something else to recommend.

@praveen-ks
Copy link
Author

Ah, I see. I didn't realize you had redis gem installed in addition to the redis-clustering gem.

If it was just the redis-clustering gem, the Redis instrumentation may not have installed on agent startup, preventing the middleware from being registered.

I'll take another look at the issue and see if I have something else to recommend.

Thanks @kaylareopelle

It will be really helpful if I can get some workaround for time being.

@kaylareopelle
Copy link
Contributor

Hi @praveen-ks! I've had a chance to look a little more closely at this issue. I believe part of the slowdown may have been caused by errors silently being raised while trying to capture the db name.

I have a branch, redis-clustering, that fixes this issue and also adds instrumentation for users of this gem for the call and connect methods via the RedisClient middleware reigstration. This should bring the instrumentation experience much closer to what you're familiar with on earlier versions of Redis.

If you'd like to test out the functionality, you can do so by updating the way the newrelic_rpm gem is installed to use the branch:

gem 'newrelic_rpm', github: 'newrelic/newrelic-ruby-agent', branch: 'redis-clustering'

@github-project-automation github-project-automation bot moved this from In Sprint to Code Complete/Done in Ruby Engineering Board Jul 8, 2024
@kaylareopelle
Copy link
Contributor

Hi @praveen-ks, this branch has been merged and will be included in a future release. The redis-clustering branch will exist until this release. If you decide to test it and have any feedback for us, please add a comment to this issue!

@praveen-ks
Copy link
Author

Hi @praveen-ks, this branch has been merged and will be included in a future release. The redis-clustering branch will exist until this release. If you decide to test it and have any feedback for us, please add a comment to this issue!

Thanks @kaylareopelle, can't test it instantly because of other priorities. Will plan it sooner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 Story Point Estimate community To tag external issues and PRs submitted by the community
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants