Add `rest-client` instrumentation adapter #172

grantbdev · 2020-01-22T04:47:51Z

Overview

Add rest-client instrumentation adapter similar to the existing adapter for Faraday.

The rest-client gem uses restclient internally for file/folder naming, so I used that style for the adapter code. The gem's README also titleizes the name as simply "REST Client", so that is used in some spots but if that is too confusing I could try to clarify or consolidate on a single name.

This is a great start @grantbdev! A number of things have changed over the last week that helps to streamline setup and install instrumentation. A good portion of my suggestions are based on those improvements. As I went through this PR, I realized that there were some existing issues with the Faraday and Sinatra adapters and ultimately made #177 to address them. I reference that PR in a number of comments and it corrects a few issues in our current instrumentation "template".

adapters/restclient/example/Gemfile

adapters/restclient/example/restclient.rb

adapters/restclient/lib/opentelemetry/adapters/restclient/patches/request.rb

adapters/restclient/test/test_helper.rb

adapters/restclient/lib/opentelemetry/adapters/restclient/adapter.rb

mwear · 2020-01-25T20:46:51Z

adapters/restclient/lib/opentelemetry/adapters/restclient/adapter.rb

+    module RestClient
+      class Adapter < OpenTelemetry::Instrumentation::Adapter
+        install do |config|
+          require_dependencies


I'm taking a mental note of how. many times I see this in adapters. It might make sense to make this an optional part of the DSL at some point.

grantbdev · 2020-01-27T19:22:11Z

@mwear Thank you for the feedback! I've pushed up my changes.

mwear

As I go through these adapters with a fine tooth comb, I find a few more holes in our instrumentation template. I noticed that we are not using Rubocop on the instrumentation adapters, which lead me to create #179. We should add it for all instumentation adapters (including this one). You can look at #173, but the basic approach will be to:

add .rubocop.yml to the root directory, see https://github.com/open-telemetry/opentelemetry-ruby/blob/22ea74a5a435de9b63e6522ab8c6066223f0f172/adapters/faraday/.rubocop.yml
add .rubocop.yml to the test directory, see https://github.com/open-telemetry/opentelemetry-ruby/blob/22ea74a5a435de9b63e6522ab8c6066223f0f172/adapters/faraday/test/.rubocop.yml
update the Rakefile, see: https://github.com/open-telemetry/opentelemetry-ruby/blob/c07e5378de2c377103ee0749632bce4613c9e40a/adapters/faraday/Rakefile
update the gemspec, see: https://github.com/open-telemetry/opentelemetry-ruby/blob/22ea74a5a435de9b63e6522ab8c6066223f0f172/adapters/faraday/opentelemetry-adapters-faraday.gemspec
Add to the top level rake file, see: https://github.com/open-telemetry/opentelemetry-ruby/blob/c07e5378de2c377103ee0749632bce4613c9e40a/Rakefile
Fix violations

adapters/faraday/lib/opentelemetry/adapters/faraday/middlewares/tracer_middleware.rb

adapters/restclient/lib/opentelemetry/adapters/restclient/patches/request.rb

grantbdev · 2020-01-28T19:41:57Z

@mwear Feedback addressed.

mwear

This looks great @grantbdev. Thanks for working through my feedback as we hone our instrumentation template.

grantbdev · 2020-01-29T20:10:23Z

adapters/faraday/example/faraday_no_op.rb

+end
+
+OpenTelemetry::SDK.configure do |c|
+  c.use 'OpenTelemetry::Adapters::Faraday', tracer_middleware: NoOp


@mwear @elskwid Based on our discussion, I've updated the example files to use the proper installation and to have the no-op examples in separate files.

mwear · 2020-01-30T23:22:27Z

adapters/restclient/lib/opentelemetry/adapters/restclient/patches/request.rb

+
+          def trace_request # rubocop:disable Metrics/AbcSize, Metrics/MethodLength
+            span = tracer.start_span(
+              url,


This will lead to high-cardinality span names. See: #175 (comment).

@mwear We could use "HTTP #{METHOD_NAME}". If that sounds good I'll update both PRs.

Only other thing I can think of that would add specificity without introducing a lot more would be to include URL host/domain.

grantbdev · 2020-01-31T23:41:43Z

@mwear Updated this PR with lower-cardinality span names and a few other tweaks I've found from working on more adapters.

Do you think we should add setting of span.status to each adapter?
https://github.com/mutations/opentelemetry-ruby/commit/35204f07cb543ec3e533365f6cb148fb0f4ca866#diff-d5fef5fa085cedfa6cea4c783a6bd8c2R47

mwear

Good call on setting span.status. We should be doing that here and on the other http client spans. I had a couple of other comments.

adapters/restclient/lib/opentelemetry/adapters/restclient/adapter.rb

adapters/restclient/lib/opentelemetry/adapters/restclient/patches/request.rb

grantbdev · 2020-02-04T17:45:22Z

@mwear Feedback addressed.

grantbdev · 2020-02-06T19:00:29Z

PR updated to use "HTTP #{method_name}" span names as discussed in the SIG meeting.

mwear

There's one last change now that the context prop PR has merged. You'll probably want to rebase, make the suggested change, and ensure things work.

adapters/faraday/lib/opentelemetry/adapters/faraday/middlewares/tracer_middleware.rb

grantbdev · 2020-02-07T02:18:04Z

@mwear PR updated and I made sure to use the block that provides the span context for restclient like you suggested for excon.

I'm not sure that we have anything explicitly testing propagation, let me know if you have ideas on how to accomplish that.

mwear · 2020-02-07T19:20:15Z

@grantbdev Thanks for bringing up propagation tests. It is something we should test and in general, we should port over applicable tests when converting Datadog instrumentation. Here is an example from dd-trace-rb: https://github.com/DataDog/dd-trace-rb/blob/master/spec/ddtrace/contrib/rest_client/request_patch_spec.rb#L196-L209

By default OpenTelemetry.propagation.http_injectors is empty, making inject effectively a no-op. You could setup a custom injector for your test, or you could just use the trace context injector. Currently, there is a debate as to whether or not trace context should be a default injector. You can use the Datadog test as template and setup an injector like so:

before do
  # these are currently empty, but this will future proof the test
  @orig_injectors = OpenTelemetry.propagation.http_injectors
  OpenTelemetry.propagation.http_injectors = [OpenTelemetry::Trace::Propagation.http_trace_context_injector]
end

after do
  OpenTelemetry.propagation.http_injectors = @orig_injectors
end

grantbdev · 2020-02-07T22:32:10Z

@mwear Tests added, let me know what you think. I wasn't sure if we should use the TraceParent class directly to generate the header value in the test and I didn't know how to get the context for that approach either.

mwear · 2020-02-08T00:14:52Z

👀 ing good @grantbdev. Thanks for addressing all my feedback. I am slightly weirded out that the assertions check for headers['Traceparent'] when the injector uses traceparent as the key. Out of curiosity, do you know if that is the libraries doing that normalization?

Anyhow, I am happy with where this is. Would you like me to leave this open for you to take a look @fbogsany, or should I go ahead and merge?

grantbdev · 2020-02-08T22:55:18Z

@mwear I think the capitalization is coming from the libraries. So far, all of them except ethon (which I haven't opened a PR for yet) have capitalized the traceparent header.

mwear · 2020-02-18T04:58:35Z

This has been open for a bit and hasn't received any new comments. I'd like to merge it, but will need a rebase first.

grantbdev · 2020-02-18T16:50:16Z

@mwear Rebased

* Add datadog-porting-guide * Reflection on experiences implementing instrumentation adapters so far (faraday, sinatra, rack) * Update based on recent PR comments * e.g., #172 * Update with suggestions from PR * Make a note about runtime performance considerations * usually show up as requests in submitted PRs

Add `rest-client` instrumentation adapter based on the adapter for Faraday and `ddtrace` code for `rest-client`. The `rest-client` gem uses `restclient` internally for file and folder naming, so I used that style for most of the internal adapter code. `rest-client` is used for names referring to the gem name itself. Related to #67

Update `faraday` adapter to match changes in the `rest-client` adapter. In the example code, the install call is replaced to work with the structure changes from #175. Support for disabling span reporting via a custom override is no longer provided since there is not a need for it at this time. This also removes the need to support injection of custom middleware. In the tracer middleware, the HTTP method attribute value is set to an uppercase string to be compliant with the specification. The name of the span is set to "HTTP #{verb}" to reduce the cardinality of the span names as described in the spec. The span status is now set when the response is received. The span propagation is now done directly. Propagated headers are now tested.

mwear · 2020-02-18T17:22:39Z

Thanks @grantbdev!

grantbdev marked this pull request as ready for review January 22, 2020 04:51

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

mwear requested changes Jan 25, 2020

View reviewed changes

grantbdev mentioned this pull request Jan 27, 2020

Add excon instrumentation adapter #175

Merged

mwear requested changes Jan 28, 2020

View reviewed changes

adapters/faraday/lib/opentelemetry/adapters/faraday/middlewares/tracer_middleware.rb Outdated Show resolved Hide resolved

adapters/restclient/lib/opentelemetry/adapters/restclient/patches/request.rb Outdated Show resolved Hide resolved

mwear approved these changes Jan 29, 2020

View reviewed changes

grantbdev commented Jan 29, 2020

View reviewed changes

mwear reviewed Jan 30, 2020

View reviewed changes

mwear reviewed Feb 4, 2020

View reviewed changes

adapters/restclient/lib/opentelemetry/adapters/restclient/adapter.rb Outdated Show resolved Hide resolved

adapters/restclient/lib/opentelemetry/adapters/restclient/patches/request.rb Show resolved Hide resolved

mwear reviewed Feb 7, 2020

View reviewed changes

adapters/faraday/lib/opentelemetry/adapters/faraday/middlewares/tracer_middleware.rb Outdated Show resolved Hide resolved

adapters/faraday/lib/opentelemetry/adapters/faraday/middlewares/tracer_middleware.rb Outdated Show resolved Hide resolved

grantbdev added 2 commits February 18, 2020 11:11

mwear merged commit 8745e14 into open-telemetry:master Feb 18, 2020

grantbdev deleted the gb/feature--rest-client-instrumentation--67 branch February 18, 2020 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `rest-client` instrumentation adapter #172

Add `rest-client` instrumentation adapter #172

grantbdev commented Jan 22, 2020

mwear left a comment

mwear Jan 25, 2020

grantbdev commented Jan 27, 2020

mwear left a comment

grantbdev commented Jan 28, 2020

mwear left a comment

grantbdev Jan 29, 2020

mwear Jan 30, 2020

grantbdev Jan 30, 2020

grantbdev commented Jan 31, 2020

mwear left a comment

grantbdev commented Feb 4, 2020

grantbdev commented Feb 6, 2020

mwear left a comment

grantbdev commented Feb 7, 2020

mwear commented Feb 7, 2020

grantbdev commented Feb 7, 2020

mwear commented Feb 8, 2020

grantbdev commented Feb 8, 2020

mwear commented Feb 18, 2020

grantbdev commented Feb 18, 2020

mwear commented Feb 18, 2020

Add rest-client instrumentation adapter #172

Add rest-client instrumentation adapter #172

Conversation

grantbdev commented Jan 22, 2020

Overview

Related

mwear left a comment

Choose a reason for hiding this comment

mwear Jan 25, 2020

Choose a reason for hiding this comment

grantbdev commented Jan 27, 2020

mwear left a comment

Choose a reason for hiding this comment

grantbdev commented Jan 28, 2020

mwear left a comment

Choose a reason for hiding this comment

grantbdev Jan 29, 2020

Choose a reason for hiding this comment

mwear Jan 30, 2020

Choose a reason for hiding this comment

grantbdev Jan 30, 2020

Choose a reason for hiding this comment

grantbdev commented Jan 31, 2020

mwear left a comment

Choose a reason for hiding this comment

grantbdev commented Feb 4, 2020

grantbdev commented Feb 6, 2020

mwear left a comment

Choose a reason for hiding this comment

grantbdev commented Feb 7, 2020

mwear commented Feb 7, 2020

grantbdev commented Feb 7, 2020

mwear commented Feb 8, 2020

grantbdev commented Feb 8, 2020

mwear commented Feb 18, 2020

grantbdev commented Feb 18, 2020

mwear commented Feb 18, 2020

Add `rest-client` instrumentation adapter #172

Add `rest-client` instrumentation adapter #172