search

discourse / prometheus_exporter

A framework for collecting and aggregating prometheus metrics
MIT License
539 stars 156 forks source link

Pass msg hash to Sidekiq jobs custom_labels function #227

Closed TLischkaRemerge closed 2 years ago

TLischkaRemerge commented 2 years ago

A previous PR added the option to define a class method on Sidekiq worker classes. One thing that was accidentally left out is the ability for this function to use the arguments to an individual job run to generate the custom labels. I changed the code to pass the jobs hash to the custom_labels class function if it is defined.

The hash only gets passed if the custom_labels function expects > 0 arguments. This avoids a breaking change with the previous version where a custom_labels function on a worker class didn't take any arguments.

SamSaffron commented 2 years ago

the arity check does make me a bit nervous, can we put a TODO in the code base to remove it at some point?

TLischkaRemerge commented 2 years ago

I know what you mean, I also felt quite apprehensive about adding it but I wasn't sure how to avoid a breaking change otherwise. Another option could be to leave it until the next major version is being prepared. Then it could just be a breaking change rather than having the workaround in place. What do you think?

SamSaffron commented 2 years ago

I am ok just to keep the awkward for now as long as you add a #TODO remove when version X is released

TLischkaRemerge commented 2 years ago

I put a TODO comment to remove the arity check in the next major version: 3.0.0 Sorry it took a long time to get back to you on this one, caught covid in the meantime 😷

SamSaffron commented 2 years ago

Thanks @TLischkaRemerge