feat: Faraday instrumentation installs OTel middleware first in handler list

[nicksieger]

This allows propagation headers to be added before other middleware that may depend on them being installed, such as
faraday_middleware-aws-sigv4, which does a signature calculation that hashes all headers. If the headers are installed last, the signature calculation will not match the servers's hash value.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Nick Sieger (84e1123, 0e0d7c7)

[nicksieger]

Upon further inspection, while I think this might still be worth considering, instead the AWS signer should be configured to ignore tracing headers, like it already does for authorization. Let me know if you think the contribution is worthwhile, otherwise feel free to close.

[fbogsany]

instead the AWS signer should be configured to ignore tracing headers, like it already does for authorization

I think this is the right approach.

It's also worth considering how to allow users to adjust the position of the middleware in the stack. IMO the right default is to place it as "close to the wire" as possible. I think the "escape hatch" should be an option to not install the middleware, in which case users would install it explicitly in their Faraday middleware stacks.

I think we have part of this already: if the user explicitly inserts the middleware, the RackBuilder module will skip adding it explicitly:

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

Lines 15 to 17 in ec213f0

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

[nicksieger]

Thanks for the feedback. I agree with your analysis. Closing this.

Would appropriately instrumenting the AWS signing library to ignore trace headers be worth a new library? If so I can throw up a PR with a stab at that.

[fbogsany]

Would appropriately instrumenting the AWS signing library to ignore trace headers be worth a new library? If so I can throw up a PR with a stab at that

I don't know what's involved in that. Is that a configuration option for the AWS signing library, or does it require monkey-patching? It doesn't quite feel like "instrumentation".

[nicksieger]

Yeah, it's not really instrumentation, just configuration. It gets configured via an initializer, so I have to monkey-patch it like so. Maybe just having this info in the PR is enough documentation for people to fix this for themselves.

  # Patch Aws::Sigv4::Signer to always ignore tracing headers
  begin
    require "aws-sigv4"
    module IgnoreTracingHeaders
      def initialize(options = {})
        (options[:unsigned_headers] ||= []).concat %w(traceparent tracestate baggage)
        super(options)
      end
    end

    class Aws::Sigv4::Signer
      prepend IgnoreTracingHeaders
    end
  rescue LoadError
    # Skip since aws_sigv4 isn't present
  end

[leemeichin]

As an alternative to monkeypatching, you can pass the unsigned headers in directly when setting up the middleware:

f.request(:aws_sigv4, service: 'es', region: 'eu-west-1', 
  unsigned_headers: %w[traceparent tracestate baggage])

The middleware just passes all of your config to the Signer object: https://github.com/winebarrel/faraday_middleware-aws-sigv4/blob/master/lib/faraday_middleware/request/aws_sigv4.rb#L12