Update Redis instrumentation to support v5 #1611
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
fallwith
reviewed
Nov 10, 2022
tannalynn
reviewed
Nov 10, 2022
This reverts commit d49ef0f.
tannalynn
approved these changes
Nov 11, 2022
SimpleCov Report
|
kaylareopelle
commented
Nov 11, 2022
| @@ -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.
The stubbed method was changed to connect because establish_connection does not exist in Redis 5.0
fallwith
approved these changes
Nov 11, 2022
|
@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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_vcall_vand 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 definesmultiandpipelinedmethods, 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).pipelinedandmulticalls 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.