search

discourse / prometheus_exporter

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

swap from hardcoded STDERR to logger #177

Closed anujbiyani closed 2 years ago

anujbiyani commented 2 years ago

logger still defaults to STDERR, so there's no change unless a user passes in their own logger

~I wasn't sure how to unit test this, doesn't seem like logger code is usually unit tested but happy to add them if you'd like!~ 75281d3 (#177)

I think there are three affected code paths:

  1. running via bin/prometheus_exporter
  2. including the gem in another project and initializing a client
  3. running a server like https://github.com/discourse/prometheus_exporter#single-process-mode

I tested 1+2 as thoroughly as I could think of. For 3 I'm not sure how to test it other than launching the server (which works fine).

fixes https://github.com/discourse/prometheus_exporter/issues/83

SamSaffron commented 2 years ago

Overall this looks great and complete.

Yeah I think it makes sense to have some minimal testing here, you can just implement a test logger and confirm at least that it is passed in and usable?

anujbiyani commented 2 years ago

Thanks for the push, I think I was just being lazy. This seems like a useful test to have: 75281d3 (#177)

anujbiyani commented 2 years ago

Looks like Appraisal has a known issue that matches the current build failure, with a possible fix+release this Friday: https://github.com/thoughtbot/appraisal/pull/184#issuecomment-884087923

SamSaffron commented 2 years ago

This is a great PR and change @anujbiyani, thank you!

If you feel like doing any triage on other open bugs we have I would be delighted!

anujbiyani commented 2 years ago

@SamSaffron thanks for all your work on this gem!

theorician commented 2 years ago

@SamSaffron Hi, would you mind releasing a point version with this change in? We've been waiting for this eagerly and would love to get this feature in to minimise some of the output spam.

@anujbiyani Thanks for this change!

SamSaffron commented 2 years ago

Sure ... will post a release today.

On Tue, Aug 3, 2021 at 9:23 PM theorician @.***> wrote:

@SamSaffron https://github.com/SamSaffron Hi, would you mind releasing a point version with this change in? We've been waiting for this eagerly and would love to get this feature in to minimise some of the output spam.

@anujbiyani https://github.com/anujbiyani Thanks for this change!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/discourse/prometheus_exporter/pull/177#issuecomment-891766167, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABIXIR7LJE5L5SJ4R7A4TT27GUXANCNFSM5AW2BE4Q .