-
Notifications
You must be signed in to change notification settings - Fork 599
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
Update Redis instrumentation to support v5 #1611
Conversation
Co-authored-by: hramadan <hramadan@newrelic.com> Redis gem 5.x comprises a major structural refactor for the library. * Calls to methods like get and set are no longer routed through `call`, but through a method named `call_v` * The methods we instrument have been moved into a new library, redis-client, a dependency of the redis gem. * Prepending onto the new library's class (::RedisClient) for the `call_v` and connect methods maintains consistent reporting behavior with earlier versions of Redis. * The location, behavior, and arguments of pipelined and multi methods changed. * In previous versions, pipeline and multi calls were routed through a method named `call_pipeline`. There is a similar method in Redis 5.x named `call_pipelined`. However, this method is not defined in ::RedisClient, preventing our prepend instrumentation from working. ::RedisClient defines `multi` and `pipelined` methods, but they do not have access to the NoSQL statement as an argument or other instance variable/method on the caller. * For `call_pipelined`, we now leverage Redis 5.x's new instrumentation API, `::RedisClient.register(module_name)`. * This process introduces instrumentation too late to capture errors at the segment level. Despite this shortcoming, this strategy is used for `pipelined` and `multi` calls because it has access to the NoSQL statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
This reverts commit d49ef0f.
SimpleCov Report
|
@@ -336,8 +336,8 @@ def simulated_error_class | |||
end | |||
|
|||
def simulate_read_error | |||
redis = Redis.new(:host => redis_host) | |||
redis.send(client).stubs("establish_connection").raises(simulated_error_class, "Error connecting to Redis") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stubbed method was changed to connect
because establish_connection
does not exist in Redis 5.0
@kaylareopelle hi, I stumbled upon this PR because I am using I am unclear on whether Wondering if you can provide any guidance and/or whether this is something that is on your radar to address? Using |
Hi @stevenou, thanks for bringing this to our attention. We're taking a look at the problem and hope to have more info for you soon. |
Co-authored-by: hramadan hramadan@newrelic.com
Redis gem 5.x comprises a major structural refactor for the library.
call
, but through a method namedcall_v
call_v
and connect methods maintains consistent reporting behavior with earlier versions of Redis.call_pipeline
. There is a similar method in Redis 5.x namedcall_pipelined
. However, this method is not defined in ::RedisClient, preventing our prepend instrumentation from working. ::RedisClient definesmulti
andpipelined
methods, but they do not have access to the NoSQL statement as an argument or other instance variable/method on the caller.call_pipelined
, we now leverage Redis 5.x's new instrumentation API,::RedisClient.register(module_name)
.pipelined
andmulti
calls because it has access to the NoSQL statementCloses: #1361
P.S. Test coverage is very high for Redis instrumentation, so we could rely on the existing test suite to inform us of the gaps with this new major version.