Shopify / statsd-instrument

A StatsD client for Ruby apps. Provides metaprogramming methods to inject StatsD instrumentation into your code.
http://shopify.github.io/statsd-instrument
MIT License
570 stars 94 forks source link

Add builder pattern for instrumenting via meta-programming #271

Closed bravehager closed 3 years ago

bravehager commented 4 years ago

Why?

At @justworkshr, we use statsd-instrument for a lot of our instrumentation. We're fairly split between inlining our StatsD calls and using the provided meta-programming methods. Even though many of us despise it, inlining is often a more convenient and readable solution than meta-programming. We'd love to use the meta-programming methods, but we end up with code that looks like this:

GoogleBase.extend StatsD::Instrument
GoogleBase.singleton_class.extend StatsD::Instrument
GoogleBase.singleton_class.statsd_measure :insert ...
GoogleBase.singleton_class.statsd_count_success :insert ...
GoogleBase.statsd_count_success, :save ...
GoogleBase.singleton_class.statsd_measure :insert ...
GoogleBase.statsd_measure :insert ...
...

A lot of the verbosity comes down to how the gem doesn't really support instrumenting class methods (meaning, it's possible, but it isn't pretty). More than anything, we want to be able to logically divide our instrumentation by class, without littering our code with GoogleBase and GoogleBase.singleton_class. Issue #104 proposed a module builder pattern for meta-programming, a definite improvement over the existing meta-programming methods. However, it still forces us to think in terms of regular classes and singleton classes. In my opinion, it's much more natural to think in terms of one class and many class methods and instance methods. This PR attempts to provide a better pattern for instrumenting via meta-programming without introducing any breaking changes to the existing API.

Description

The PR is almost all syntactic sugar. We introduce a new StatsD::InstrumentBuilder class which wraps over the existing meta-programming methods, as well as a new method StatsD.instrument, for interacting with InstrumentBuilder.

Here's what we can do with it:

StatsD.instrument GoogleBase do |klass|
  klass.measure :insert, "GoogleBase.insert"
  klass.measure :insert, "GoogleBase.insert", class_method: true
end

And if we know our class is mostly class methods, we'd like to establish that outside of the block via an optional singleton argument:

StatsD.instrument GoogleBase, singleton: true do |klass|
  klass.measure :insert, "GoogleBase.insert",  class_method: false
  klass.measure :insert, "GoogleBase.insert"
end

Concerns

Performance

All changes in this PR wrap over existing functionality in statsd-instrument. It doesn't change any existing functionality. The only performance concern I have is how InstrumentBuilder initializes:

class InstrumentBuilder
  def initialize(klass, singleton: false)
    klass.extend(Instrument)
    klass.singleton_class.extend(Instrument)
    ...

Ideally, we wouldn't extend a class unless we knew it was going to be instrumented. This issue can be prevented if we StatsD::InstrumentBuilder to build up a definition of the instrument and apply it after StatsD.instrument yields its block argument.

def instrument(klass, singleton: false)
  builder = StatsD::InstrumentBuilder.new(klass, singleton: singleton)
  yield builder
  builder.build!
end