discourse / prometheus_exporter

A framework for collecting and aggregating prometheus metrics
MIT License
525 stars 153 forks source link

Sidekiq middleware not Ruby >=3.0 compatible #229

Closed benzimmer closed 1 year ago

benzimmer commented 2 years ago

The Sidekiq middleware is initialized with a keyword argument (https://github.com/discourse/prometheus_exporter/blob/main/lib/prometheus_exporter/instrumentation/sidekiq.rb#L36)

def initialize(client: nil)
  @client = client || PrometheusExporter::Client.default
end

which goes against Sidekiq's base implementation of a middleware which suggests an options hash being passed in: https://github.com/mperham/sidekiq/wiki/Middleware#server-middleware

class MyMiddleware::Server::ErrorLogger
  def initialize(options=nil)
    # options == { :foo => 1, :bar => 2 }
  end
  def call(worker, msg, queue)
    begin
      yield
    rescue => ex
      puts ex.message
    end
  end
end

While this is not a problem before Ruby 3.0, using the prometheus_exporter Sidekiq middleware with Ruby 3.0 or 3.1 will result in an ArgumentError because Sidekiq's middleware implementation only passes on *args (https://github.com/mperham/sidekiq/blob/main/lib/sidekiq/middleware/chain.rb#L158) and with the changes to positional/keyword arguments in Ruby 3.0 the method signature doesn't match any more.

I'm currently trying to put together a test case and PR to fix this in a backwards compatible way.

NickLarsenNZ commented 1 year ago

Does #230 close this issue?