discourse / prometheus_exporter

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

FIX: correct instrumentation for Redis 5 #250

Closed SamSaffron closed 1 year ago

SamSaffron commented 1 year ago

Redis 5 gem introduces redis_client as the low level transport method

We no longer patch Redis gem if we detect it is version 5 and above and instead use the supported middleware interface to patch it.

SamSaffron commented 1 year ago

@tgxworld / @byroot I think I have all the feedback addressed, let me know what you think

SamSaffron commented 1 year ago

Oh tricky one @byroot ... what happens if a pipeline takes say N seconds to commit and get results back from Redis, how is the instrumenter going to pick it up?

byroot commented 1 year ago

what happens if a pipeline takes say N seconds to commit and get results back from Redis, how is the instrumenter going to pick it up?

I'm not sure I understand what is tricky here.

module MyInstru
  def call_pipelined(commands, config)
    start = Time.now
    result = super
    duration = Time.now - start # N seconds
    results
  end
end
SamSaffron commented 1 year ago

Oh so individual commands are not instrumented, makes sense

On Fri, 28 Oct 2022 at 7:10 pm, Jean Boussier @.***> wrote:

what happens if a pipeline takes say N seconds to commit and get results back from Redis, how is the instrumenter going to pick it up?

I'm not sure I understand what is tricky here.

module MyInstru def call_pipelined(commands, config) start = Time.now result = super duration = Time.now - start # N seconds results endend

— Reply to this email directly, view it on GitHub https://github.com/discourse/prometheus_exporter/pull/250#issuecomment-1294658430, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABIXKUVLAX2MDSYEDUL43WFOC77ANCNFSM6AAAAAARPU2QY4 . You are receiving this because you authored the thread.Message ID: @.***>

SamSaffron commented 1 year ago

Awesome thanks about the tip !

On Fri, 28 Oct 2022 at 8:02 pm, Sam Saffron @.***> wrote:

Oh so individual commands are not instrumented, makes sense

On Fri, 28 Oct 2022 at 7:10 pm, Jean Boussier @.***> wrote:

what happens if a pipeline takes say N seconds to commit and get results back from Redis, how is the instrumenter going to pick it up?

I'm not sure I understand what is tricky here.

module MyInstru def call_pipelined(commands, config) start = Time.now result = super duration = Time.now - start # N seconds results endend

— Reply to this email directly, view it on GitHub https://github.com/discourse/prometheus_exporter/pull/250#issuecomment-1294658430, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABIXKUVLAX2MDSYEDUL43WFOC77ANCNFSM6AAAAAARPU2QY4 . You are receiving this because you authored the thread.Message ID: @.***>

SamSaffron commented 1 year ago

@tgxworld we are all done now, your comment is addressed