Add `redis` instrumentation adapter #176

grantbdev · 2020-01-24T22:43:58Z

Overview

Add redis instrumentation adapter. This is the first database client adapter, so I made it conform to those conventions as best I could understand. I was particularly uncertain on what the span names should be, but based on the spec guidelines I decided to go with the command name for single command requests and "pipeline" for a pipeline request. Let me know if there is anything that should be changed or added.

A lot of the formatting code is copied from dd-trace-rb. It was unclear to me whether in OpenTelemetry there are length limits on span attributes, so I kept the truncation behavior from dd-trace-rb let me know if the truncation limit should be changed or removed entirely.

I don't see how we have per-connection span reporting. Either this module is prepended to the redis client, or the no-op version is. We either get all spans, or no spans. We already have the ability to do this by installing the instrumentation (with c.use), or not installing the instrumentation. Correct me if I'm mistaken on this point.

@mwear "per-connection" may be misleading, but the arguments provided to this method would allow someone to create their own patch that would could disable spans for certain methods, keys, values, etc. That may not be a use case that needs to be supported and could be simplified/removed, but I kept it for parity with the functionality of the HTTP client adapters so far.

I would err on the side of YAGNI at this stage in the project unless compelling use cases become apparent. My preference would be to remove it for now. That would also apply to the http client instrumentation

@mwear Should the ability to provide a custom patch/middleware for adapters still exist? It was only used in tests/examples for overriding this method, but I'm not sure if there is another use case or if it is desired for dependency injection.

adapters/redis/lib/opentelemetry/adapters/redis/utils.rb

adapters/redis/test/opentelemetry/adapters/redis/adapter_test.rb

grantbdev · 2020-01-31T03:00:40Z

@mwear PR updated. I ported the tests of the ported utils. I would like more input on what more of the adapter should be tested.

grantbdev · 2020-02-06T19:01:22Z

PR updated to add test assertions on the span names so it's easier for reviewers to see what it looks like.

mwear · 2020-02-11T02:03:55Z

adapters/redis/lib/opentelemetry/adapters/redis/patches/client.rb

+          def client_attributes
+            host = options[:host]
+            port = options[:port]
+
+            {
+              'component' => 'redis',
+              'db.type' => 'redis',
+              'db.instance' => options[:db].to_s,
+              'db.url' => "redis://#{host}:#{port}",
+              'net.peer.name' => host,
+              'net.peer.port' => port
+            }
+          end


It looks like db is changeable at runtime, but is everything else going to be static data once the client is initialized? If so, can we memoize and freeze this data instead of making it each time? The db.name can be merged in with db.statement in call.

is everything else going to be static data once the client is initialized?

@mwear I don't think so. https://github.com/redis/redis-rb#sentinel-support and https://github.com/redis/redis-rb#cluster-support seem to indicate to me that the port and even host can be different between requests. Using the example code I was able to see different hosts/ports for the spans using Sentinel. Unfortunately it does not appear that fakeredis which we are using for the tests supports Sentinel or clusters.

The db.name can be merged in with db.statement in call.

Can you explain this more?

I was just saying that if the config was static aside from db.name we could have a frozen hash and merge('db.name' => ..., 'db.statement') with it. It's a moot point given that config will change at runtime. Thanks for your research!

adapters/redis/test/opentelemetry/adapters/redis/adapter_test.rb

grantbdev · 2020-02-11T17:33:33Z

@mwear Feedback addressed.

Something to consider: ddtrace's #trace automatically catches errors and sets the span status. I've started in-lining that logic, but it's something that may be good to add to #in_span.

mwear · 2020-02-12T00:39:27Z

We might want to change in_span automatically rescue, record the error, and reraise. I'll open an issue about this and we can discuss it at the SIG.

mwear · 2020-02-12T01:07:00Z

I'm not totally sure about the inlining, but everything else looks good to me. I created an issue with some questions / proposals around error handling: #181. I think we should discuss this at the SIG and determine what to do about the inlining there. Right now, to replicate what is happening in dd-trace would have to inline exception handling for every Tracer#in_span call, which is definitely not ideal.

mwear

I'm tentatively approving this pending the outcome of our discussion about recording errors to see if the inlining should stay or go. It's possible there will be some minor changes depending on what we decide, but otherwise, everything looks great!

fbogsany · 2020-02-20T02:59:38Z

adapters/redis/lib/opentelemetry/adapters/redis/patches/client.rb

+      module Patches
+        # Module to prepend to Redis::Client for instrumentation
+        module Client
+          def call(*args, &block)


The gem has a couple of other call_* methods: call_pipelined (note the difference from call_pipeline) and call_loop. Have you considered patching process instead? The upside is that it is a bottleneck method that is called on all call* paths. The downside is that errors are sometimes raised outside its scope, so they're harder to intercept.

@fbogsany I did not consider those because dd-trace-rb does not patch those methods.

Relevant bits of Redis client: https://github.com/redis/redis-rb/blob/41395e9a1923fe72fad8403e60cb98627d515a85/lib/redis/client.rb#L124-L215

I made an issue to track improvements: #197.

grantbdev · 2020-02-20T18:03:32Z

@mwear This PR (and my other adapter PRs) are rebased and the exception handling has been simplified.

mwear · 2020-02-27T01:22:09Z

Last call for comments on this. It'd be nice to merge this tomorrow after the SIG meeting.

Add `redis` instrumentation adapter based on the adapter for `rest-client` and the `dd-trace-rb` code for `redis`, including its utilites. The `fakeredis` gem is used in the tests to have a fake server. The Docker container for the example includes a real Redis server. Related to #67

grantbdev requested review from bai, dazuma, elskwid, fbogsany, luvtechno and mwear as code owners January 24, 2020 22:43

mwear requested changes Jan 31, 2020

View reviewed changes

grantbdev mentioned this pull request Feb 4, 2020

Add rest-client instrumentation adapter #172

Merged

mwear reviewed Feb 11, 2020

View reviewed changes

mwear approved these changes Feb 12, 2020

View reviewed changes

mwear mentioned this pull request Feb 14, 2020

Preliminary error recording #184

Merged

fbogsany reviewed Feb 20, 2020

View reviewed changes

mwear mentioned this pull request Feb 27, 2020

Improve Redis instrumentation #197

Closed

fbogsany approved these changes Feb 27, 2020

View reviewed changes

mwear merged commit ba8b7b6 into open-telemetry:master Feb 28, 2020

grantbdev deleted the gb/feature--redis-instrumentation--67 branch February 28, 2020 19:08

+                        # Override implementation (subclass) to determine per-connection
+                        # span reporting rules.
+                        def disable_span_reporting?(*_args)
+                          false
+                        end

Add redis instrumentation adapter #176

Add redis instrumentation adapter #176

Uh oh!

Conversation

grantbdev commented Jan 24, 2020

Overview

Related

Uh oh!

mwear left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

grantbdev commented Jan 31, 2020

Uh oh!

grantbdev commented Feb 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

grantbdev commented Feb 11, 2020

Uh oh!

mwear commented Feb 12, 2020

Uh oh!

mwear commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mwear left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grantbdev commented Feb 20, 2020

Uh oh!

mwear commented Feb 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add `redis` instrumentation adapter #176

Add `redis` instrumentation adapter #176

mwear commented Feb 12, 2020 •

edited

Loading