discourse / prometheus_exporter

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

Fix kwargs patching #313

Closed simpl1g closed 2 months ago

simpl1g commented 3 months ago

Fixes https://github.com/discourse/prometheus_exporter/issues/312

simpl1g commented 3 months ago

Also it looks like in Ruby 3.2/3.3 prepend works with the same speed as alias I did some comparison here https://gist.github.com/simpl1g/e34c073a8e33128d1f309873d9164967 and even with empty work there is no difference in speed. Maybe it is time to make prepend as default instrumentation, because it gives more benefits.

Comparison:
              method:  2189776.7 i/s
   method_triple_dot:  2178433.1 i/s - same-ish: difference falls within error
  prepend_triple_dot:  2161501.4 i/s - same-ish: difference falls within error
      method_prepend:  2135422.1 i/s - same-ish: difference falls within error
SamSaffron commented 3 months ago

I think I am fine with this ... crazy that 2.7 was EOL a year ago!

CI is failing though, can you have a look?

simpl1g commented 2 months ago

Thank you for review!

Looks like CI is failing for a long time, it is connected with rubocop-discourse gem, it is installing different version for different gemfiles. And in new version there are some changes in cops which break CI. So I made changes to rubocop config

BUNDLE_GEMFILE=gemfiles/ar_60.gemfile bundle list | grep rubocop-discourse
* rubocop-discourse (3.6.1)

BUNDLE_GEMFILE=gemfiles/ar_61.gemfile bundle list | grep rubocop-discourse
* rubocop-discourse (3.7.1)

Also ar_70.gemfile actually didn't test 7.0, but 6.0. Fixed this as well

BUNDLE_GEMFILE=gemfiles/ar_70.gemfile bundle list | grep active
  * activemodel (6.0.6.1)
  * activerecord (6.0.6.1)
  * activesupport (6.0.6.1)
SamSaffron commented 2 months ago

This is certainly a radical change (deliberately breaking releases prior to Ruby 3.0) , but I am going to accept it @tgxworld / @davidtaylorhq it is more correct and probably something we should pull into method profiler on Discourse.

I guess at the end of the day people that need Ruby 2 support can just install earlier gems.

Thanks @simpl1g