Add Excon instrumentation #211

walterking · 2017-10-02T06:56:04Z

I took the faraday implementation and merged it with my excon instrumentation to generate this.

Configuration, not sure how thats supposed to work, but I know youre changing that.

tracer.trace('test') do |span| Excon.get("https://google.com") end
--->
Name: test
Span ID: 2407071452987440042
Parent ID: 0
Trace ID: 8046461497076297129
Type:
Service: deliver-default
Resource: test
Error: 0
Start: 1506929900532989952
End: 1506929900707412992
Duration: 174423000
Tags: [
system.pid => 3337]
Metrics: [ ],

Name: excon-request
Span ID: 6715890832042833974
Parent ID: 2407071452987440042
Trace ID: 8046461497076297129
Type: http
Service: excon-request
Resource: GET
Error: 0
Start: 1506929900533435904
End: 1506929900707217920
Duration: 173782000
Tags: [
http.url => /,
http.method => GET,
out.host => google.com,
out.port => 443,
http.status_code => 301]
Metrics: [ ]]

palazzem · 2017-10-02T07:16:21Z

Thanks for that @walterking ! will review it during the week!

walterking · 2017-10-30T03:57:57Z

I think I updated it to the new configuration style correctly:

  Datadog.configure do |c|
    c.use :excon, distributed_tracing_enabled: true, split_by_domain: true
  end

palazzem · 2017-10-30T07:31:43Z

@walterking great! we're going to merge the new Configuration API so that your PR can be merged too! Thank you!

walterking · 2017-10-30T21:04:13Z

lib/ddtrace/contrib/excon/patcher.rb

+          end
+
+          def add_middleware
+            ::Excon.defaults[:middlewares].append(Middleware)


so, I tracked the weirdness i was seeing in our excon instrumentation to this middleware being appended after another middleware that made some redis calls. Hence the order was messed up so it looked async. I think maybe this should be prepend, but not sure, or what other ordering issues may come up from this

I think the best place to do this is by injecting the trace middleware as soon as possible in the stack, but behind the ResponseParser middleware.

With some code changes, you can do either of these:

# Default middleware stack # This is what the patcher now does Excon.new( 'http://example.com', middlewares: Datadog::Contrib::Excon::Middleware.with(middleware_options).around_default_stack ) # Custom middleware stack # If you want to specially configure an Excon connection Excon.new( 'http://example.com', middlewares: [ Excon::Middleware::ResponseParser, Datadog::Contrib::Excon::Middleware.with(middleware_options), Excon::Middleware::Mock ] )

delner · 2018-04-23T18:14:03Z

lib/ddtrace/contrib/excon/middleware.rb

+
+        def service_name(datum)
+          # TODO: Change this to implement more sensible multiplexing
+          Datadog.configuration[:excon][:split_by_domain] ? datum[:host] : @options[:service_name]


@palazzem Does this make sense to allow service_name to derive from the host per request? This was a feature in Faraday but I think it could be problematic.

delner · 2018-04-23T19:19:37Z

Did some overhauling on this pull request:

Updated to new patching style
Use new configuration
Implemented new middleware injection scheme (for customized Excon connections)
Added RSpec tests
Added documentation
Rebased on 0.13-dev
Tested in a sample app (see screenshot below)

Shown above: Three Excon requests using 1) default adapter, 2) using middleware with service name, 3) using middleware with `split_by_domain`

Should be ready for a review now.

palazzem · 2018-04-30T09:17:41Z

docs/GettingStarted.md

+| Key | Description | Default |
+| --- | --- | --- |
+| `service_name` | Service name for Excon instrumentation. When provided to middleware for a specific connection, it applies only to that connection object. | `'excon'` |
+| `split_by_domain` | Uses the request domain as the service name when set to `true`. | `false` |


palazzem · 2018-04-30T09:21:39Z

docs/GettingStarted.md

+| `error_handler` | A `Proc` that accepts a `response` parameter. If it evaluates to a *truthy* value, the trace span is marked as an error. By default only sets 5XX responses as errors. | `nil` |
+| `tracer` | A `Datadog::Tracer` instance used to instrument the application. Usually you don't need to set that. | `Datadog.tracer` |
+
+**Configuring connections to use different settings**


In other integrations, we may do something like:

Datadog.configure(client, service_name: 'something_else')

I guess we can't do it for Excon (and so the middleware must be added manually) because once initialized it cannot be changed? Consider it as a comment, if it introduces too much complexity we can keep it that way.

@palazzem Right. If you look at Faraday, which is almost identical to Excon, we can't do this either. Both of these integrations implement a middleware scheme, which require middleware to be injected when the connection/client object is created.

It might be possible to change the configuration to be derived from a Pin (much like the Sequel integration), which would permit the Datadog.configure strategy to work for a limited set of options (e.g. service_name.) But that would also introduce monkey-patching into a library that already provides an API for instrumentation. Not to say that might not be worth it, but it would represent an larger overhaul that does run contrary to the gem author's intentions.

I'd recommend keeping this as is for now, but scheduling a task to refactor it to support that Datadog.configure strategy in the future.

Agreed. Thanks!

palazzem · 2018-04-30T09:22:39Z

lib/ddtrace/contrib/excon/middleware.rb

+    module Excon
+      # Middleware implements an excon-middleware for ddtrace instrumentation
+      class Middleware < ::Excon::Middleware::Base
+        SPAN_NAME = 'excon.request'.freeze


palazzem · 2018-04-30T09:23:43Z

lib/ddtrace/contrib/excon/middleware.rb

+        end
+
+        def service_name(datum)
+          # TODO: Change this to implement more sensible multiplexing


What is this TODO about?

This split_by_domain? function is generating new services for each host it connects to, named after the host. That seems like it could make a mess of your APM services page double quick. This comment was more to say "should we be splitting the resource by domain instead?"

What is expected (and it's how it works for Python) is that if you enable that flag (disabled by default), you should see a new service for each host you're calling (so the name of the service is just the name of the host).

Splitting the resource by domain works too, but it doesn't show the time spent in different services in the sublayer graphs.

palazzem · 2018-04-30T09:28:11Z

lib/ddtrace/contrib/excon/middleware.rb

+        end
+
+        def annotate!(span, datum)
+          span.resource = datum[:method].to_s.upcase


The resource in that way is not great. We can keep it that way for now, but we should revisit all our HTTP clients at some point.

Agreed. This was how it was before, not sure what it should be. There are a lot of things we could do with this resource.

Add Excon instrumentation

palazzem added the integrations Involves tracing integrations label Oct 2, 2017

palazzem added this to the 0.10.0 milestone Oct 6, 2017

palazzem removed this from the 0.10.0 milestone Oct 19, 2017

walterking force-pushed the excon_instrumentation branch from 3f9d00c to 8fe047c Compare October 30, 2017 03:56

walterking force-pushed the excon_instrumentation branch from 8fe047c to c112129 Compare October 30, 2017 06:03

palazzem self-requested a review October 30, 2017 07:31

walterking commented Oct 30, 2017

View reviewed changes

palazzem added the community Was opened by a community member label Jan 10, 2018

delner changed the base branch from master to 0.13-dev April 20, 2018 18:09

delner force-pushed the excon_instrumentation branch from c112129 to 6326a3b Compare April 20, 2018 18:09

delner mentioned this pull request Apr 20, 2018

Inaccurate excon.response time excon/excon#621

Closed

delner force-pushed the excon_instrumentation branch from 4ed8c81 to 93b9ec1 Compare April 23, 2018 15:20

delner reviewed Apr 23, 2018

View reviewed changes

delner force-pushed the 0.13-dev branch from c51f3ac to 91e7e8b Compare April 23, 2018 18:53

adding excon instrumentation

a7e83df

delner force-pushed the excon_instrumentation branch 2 times, most recently from a648c5a to 71ee091 Compare April 23, 2018 19:16

delner self-assigned this Apr 23, 2018

delner added this to the 0.13.0 milestone Apr 23, 2018

delner added 3 commits April 23, 2018 17:34

Refactored: Excon instrumentation to use new style.

9064267

Added: Excon RSpec tests.

6388726

Added: Excon documentation.

f666303

delner force-pushed the excon_instrumentation branch from 71ee091 to f666303 Compare April 23, 2018 21:34

palazzem reviewed Apr 30, 2018

View reviewed changes

palazzem approved these changes May 2, 2018

View reviewed changes

delner changed the title ~~adding excon instrumentation~~ Add Excon instrumentation May 2, 2018

delner merged commit 896a906 into DataDog:0.13-dev May 2, 2018

delner added a commit that referenced this pull request May 8, 2018

Merge pull request #211 from walterking/excon_instrumentation

589f9ff

Add Excon instrumentation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Excon instrumentation #211

Add Excon instrumentation #211

walterking commented Oct 2, 2017 •

edited

Loading

palazzem commented Oct 2, 2017

walterking commented Oct 30, 2017 •

edited by palazzem

Loading

palazzem commented Oct 30, 2017

walterking Oct 30, 2017

delner Apr 23, 2018 •

edited

Loading

delner Apr 23, 2018

delner commented Apr 23, 2018 •

edited

Loading

palazzem Apr 30, 2018

palazzem Apr 30, 2018

delner Apr 30, 2018

palazzem May 2, 2018

palazzem Apr 30, 2018

palazzem Apr 30, 2018

delner Apr 30, 2018

palazzem May 2, 2018

palazzem Apr 30, 2018

delner Apr 30, 2018

palazzem May 2, 2018

Add Excon instrumentation #211

Add Excon instrumentation #211

Conversation

walterking commented Oct 2, 2017 • edited Loading

palazzem commented Oct 2, 2017

walterking commented Oct 30, 2017 • edited by palazzem Loading

palazzem commented Oct 30, 2017

Choose a reason for hiding this comment

delner Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delner commented Apr 23, 2018 • edited Loading

Shown above: Three Excon requests using 1) default adapter, 2) using middleware with service name, 3) using middleware with split_by_domain

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterking commented Oct 2, 2017 •

edited

Loading

walterking commented Oct 30, 2017 •

edited by palazzem

Loading

delner Apr 23, 2018 •

edited

Loading

delner commented Apr 23, 2018 •

edited

Loading

Shown above: Three Excon requests using 1) default adapter, 2) using middleware with service name, 3) using middleware with `split_by_domain`