Default position of Faraday tracer middleware changes when upgrading Faraday

[composerinteralia]

The Faraday tracer middleware works by patching the RackBuilder#adapter method to add the middleware to the stack if it isn't already there:

opentelemetry-ruby-contrib/instrumentation/faraday/lib/opentelemetry/instrumentation/faraday/patches/rack_builder.rb

Lines 14 to 20 in e2ba37d

	def adapter(*args)
	use(:open_telemetry) unless @handlers.any? do |handler|
	handler.klass == Faraday::Middlewares::TracerMiddleware
	end
	super
	end

In Faraday 0.17, #adapter is generally called last (Faraday warns otherwise), causing the tracer middleware to get installed last. That puts the tracer middleware as the innermost middleware, running directly around the adapter.

In Faraday 1.x, #adapter is typically called quite early while initializing the RackBuilder, before any user middlewares have been added. That's happening now because of this change. That ends up putting the tracer middleware as the outermost middleware (regardless of whether the user later adds the tracer middleware explicitly), running around all of the other middlewares.

I'm not entirely sure which position is most desirable, but having it shift from innermost middleware to outermost middleware when upgrading Faraday might be surprising, and is a breaking change at least for us at GitHub.

Does anybody have thoughts about what is most correct here? If we want to preserve having this default to the innermost middleware, we might be able to patch Faraday::Connection#initialize instead, adding the tracer middleware last if it isn't already there.

[arielvalentin]

Thanks for this report!

The instrumentations in this repo are mostly ports of the DD Trace Instrumentations. My guess is that this use case was not considered at the time when the instrumentation was written.

I believe that we want to update this instrumentation to ensure the tracer middleware is the first one in the list, which I believe is what you mean by outermost, like it does in the current version of DD Trace:

https://github.com/DataDog/dd-trace-rb/blob/4b1901101c0f4d1033558bd9c0ff2e1c2b963bc7/lib/datadog/tracing/contrib/faraday/patcher.rb#L33

Would you be amenable to submitting a fix for this?

[composerinteralia]

Thanks for that. https://github.com/DataDog/dd-trace-rb/blob/4b1901101c0f4d1033558bd9c0ff2e1c2b963bc7/lib/datadog/tracing/contrib/faraday/connection.rb#L12-L17 is basically what I was thinking of doing. But that would seem to put the middleware last in the list (i.e. closest to the adapter—what I have been calling innermost), not first.

[composerinteralia]

Sweet, this describes it perfectly: DataDog/dd-trace-rb#906, then they followed up with DataDog/dd-trace-rb#971 to remain warning-free with Faraday < 1.

I'm going to try basically the same changeset here.

[arielvalentin]

@composerinteralia That is a bit of nuance that I missed there. I see what you mean about the connection constructor adding the middleware, if it is not present already, at the end of the stack after it runs the patched initializer.

I think that moving it to the end of the list is undesirable because what I want is for the span to including timing of other middleware in the chain and have a separate span for the adapter.

🤔 After having looked at Faraday 1.x+, do you have any suggestions around how we may be able to do that?

Do we have limited options? Will automatic injection be difficult to pull off?

[composerinteralia]

I think that moving it to the end of the list is undesirable

The tricky bit is that it is already at the end of the list for Faraday < 1. If having it at the beginning of the list is what we ultimately want, it'd be nice to at least have a way of making that change separately from upgrading Faraday. I wonder if we could make it configurable in some way?

The current implementation is also a bit awkward on Faraday 1 because it gets added to the beginning of the list right away, so if somebody then adds it explicitly it'll end up in the list twice.

Maybe for Faraday >= 1 we could patch Connection#initialize without something like:

def initialize(...)
  super.tap do
    break if @handlers.any? do |handler| 
      handler.klass == Faraday::Middlewares::TracerMiddleware 
    end 

    if insert_middleware_before
      builder.insert(0, Faraday::Middlewares::TracerMiddleware)
    else
      use(:open_telemetry)
    end
  end
end

(With insert_middleware_before being attached to some config somewhere)

[arielvalentin]

Thanks for digging into this and explaining the problems in-depth to me. I am surprised to learn that the tracer middleware is currently being injected as the innermost middleware as opposed to the outermost.

In order to support 1.0 then I think it makes sense to preserve the behavior of being the innermost middleware and we can see about addressing moving its position to the outermost in a separate PR.

Is this something that you have the bandwidth to help us with?