Default position of Faraday tracer middleware changes when upgrading Faraday about opentelemetry-ruby-contrib HOT 6 CLOSED

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?

from opentelemetry-ruby-contrib.

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.

from opentelemetry-ruby-contrib.

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.

from opentelemetry-ruby-contrib.

@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?

from opentelemetry-ruby-contrib.

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)

from opentelemetry-ruby-contrib.

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?

from opentelemetry-ruby-contrib.