Auto instrument Faraday default connection

[marcotc]

Our auto instrumentation did not configure itself to work with Faraday's default connection.
The default connection is used when using the short Faraday syntax: Faraday.get("http://domain.com/path").

Other use cases that create a Faraday::Connection (Faraday.new, for example) are already auto instrumented.

[marcotc]

Because patching the default connection happens only once and very early in the application lifecycle (at Datadog::Contrib::Faraday::Patcher::patch), we can't convert a Settings object into a hash here, as the settings delegation won't work from this point on. This causes issues, like using a stale version of the Tracer, for example.

Instead we only resolve it at request time, which happens in the build_request_options! method.

[sdrioux]

Hi Marco - any chance this fixes this issue?

#978

[marcotc]

Hey @sdrioux, are you using ::Faraday.get in your application (in contrast to conn = Faraday.new; conn.get)? If so, then yes, it does fix it.

[ericmustin]

Lgtm.

There's one edge case that's not really blocking this PR, but would be good to understand, as I'm not terribly familiar with Faraday. It looks like there's some places where default_connection gets reset, such as when a default_adapter is configured or default_connection_options are configured: https://github.com/lostisland/faraday/blob/5547e9131aa2629cec13a6b3b00ac9daeab9d430/lib/faraday.rb#L102

https://github.com/lostisland/faraday/blob/5547e9131aa2629cec13a6b3b00ac9daeab9d430/lib/faraday.rb#L152

Would this cause our instrumentation to get dropped?

[marcotc]

That a very good point @ericmustin! When explicitly replacing the default connection, one has to provide a connection instance and thus I think it's reasonable to expect users to configure ddtrace on that connection, just like they would for any new Faraday connection.
I can see the argument for handling that case internally ourselves, but I think we have enough instrumentation options today for Faraday to cover this case.

[tjwp]

With the v0.38.0 release we're seeing this warning with the default connection:

WARNING: Unexpected middleware set after the adapter. This won't be supported from Faraday 1.0.

The call ::Faraday.default_connection.use(:ddtrace) is appending the trace middleware after the adapter with Faraday v1.0. It would be better to prepend it to the handlers.

[marcotc]

👋 @tjwp, we've seen this warning before with Faraday < 1, I'm thinking a similar issue happened here. I'll take a look into it. Thank you for reporting!

[fledman]

@marcotc
I believe this should fix the problem:

diff --git a/lib/ddtrace/contrib/faraday/patcher.rb b/lib/ddtrace/contrib/faraday/patcher.rb
index c36e295d..9a01f2ab 100644
--- a/lib/ddtrace/contrib/faraday/patcher.rb
+++ b/lib/ddtrace/contrib/faraday/patcher.rb
@@ -40,14 +40,16 @@ module Datadog
         end
 
         def add_default_middleware!
+          # 1st instrument the default connection (e.g. +Faraday.get+)
+          # 2nd instrument any newly created connections
           if target_version >= Gem::Version.new('1.0.0')
+            ::Faraday.default_connection.use(:ddtrace)
             ::Faraday::Connection.send(:prepend, Connection)
           else
+            idx = ::Faraday.default_connection.builder.handlers.size - 1
+            ::Faraday.default_connection.builder.insert(idx, Middleware)
             ::Faraday::RackBuilder.send(:prepend, RackBuilder)
           end
-
-          # Instrument the Faraday default connection (e.g. +Faraday.get+)
-          ::Faraday.default_connection.use(:ddtrace)
         end
 
         def get_option(option)