Not compatible with Redis 5

[KludgeKML]

The Redis 5 gem introduces changes that cause a rails abort when the prometheus middleware is included.

These lines: https://github.com/discourse/prometheus_exporter/blob/main/lib/prometheus_exporter/middleware.rb#L14-L18

...try to patch Redis::Client#call and Redis::Client#call_pipeline , but these methods are no longer available in Redis::Client.

I'm not 100% sure if this is a breaking change in Redis that hasn't been communicated very well, or if it's prometheus_exporter relying on methods that aren't part of Redis::Client's public interface.

Steps to replicate:

• make a new rails app
• add redis and prometheus_exporter to the gemfile:

  gem "redis", "~> 5"
  gem "prometheus_exporter", "~> 2"

• create an initialiser: /config/initializers/test.rb:

  
  require "redis/client"
  require "prometheus_exporter/middleware"

PrometheusExporter::Middleware.new(nil)

Rails crashes on initialisation with:

gems/prometheus_exporter-2.0.3/lib/prometheus_exporter/instrumentation/method_profiler.rb:75:in alias_method': undefined methodcall' for class `Redis::Client' (NameError)

alias_method :call__mp_unpatched, :call ^^^^^^^^^^^^

[SamSaffron]

Yikes, care to make a PR to add cross compatibility here?

I guess we should fish for the right method and then path the correct one.

@byroot what is the right way of getting this instrumentation in Redis 5?

[byroot]

Redis 5 use redis-client under the hood, abd redis-client have a public middleware API for instrumentation purposes.

Just look at redis-client’s README.

Le 25 oct. 2022 à 01:07, Sam @.***> a écrit :

 Yikes, care to make a PR to add cross compatibility here?

I guess we should fish for the right method and then path the correct one.

@byroot what is the right way of getting this instrumentation in Redis 5?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[SamSaffron]

I see:

Instrumentation and Middlewares

redis-client offers a public middleware API to aid in monitoring and library extension. Middleware can be registered either globally or on a given configuration instance.

module MyGlobalRedisInstrumentation def connect(redis_config) MyMonitoringService.instrument("redis.connect") { super } end

def call(command, redis_config) MyMonitoringService.instrument("redis.query") { super } end

def call_pipelined(commands, redis_config) MyMonitoringService.instrument("redis.pipeline") { super } end end RedisClient.register(MyGlobalRedisInstrumentation)

MethodProfiler is technically cheaper (no extra yielding etc...) but this can certainly work.

[byroot]

(no extra yielding etc...)

The yielding is just for the example. The only overhead of the instrumentation API is calling super which is about as cheap as you can do.

[SamSaffron]

This should fix it... https://github.com/discourse/prometheus_exporter/pull/250

@byroot is there a cleaner way of testing without needing the full dance here. Ideally I don't want to force people to install redis just so for tests, but I guess if it is better we can.

[byroot]

Depends how much certainty you want.

You can call RedisClient::Middlewares.call, call_pipelined and connect directly if you wish to just unit test your integration without having to connect to redis.

[SamSaffron]

Fixed per #250

Thanks heaps for reporting and thanks for all the help @byroot :hugs:

[booleanbetrayal]

Thanks all! Possible to get a RubyGem version bump to support this change?

[SamSaffron]

sure, will push a version today

On Wed, Nov 9, 2022 at 6:00 AM Brent Dearth @.***> wrote:

Thanks all! Possible to get a RubyGem version bump to support this change?

— Reply to this email directly, view it on GitHub https://github.com/discourse/prometheus_exporter/issues/249#issuecomment-1307689516, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABIXLW3O4EBULQFOAFGXDWHKPNXANCNFSM6AAAAAARMY3654 . You are receiving this because you modified the open/close state.Message ID: @.***>

[booleanbetrayal]

Thanks @SamSaffron !