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
574 stars 97 forks source link

Delegate StatsD singleton methods to a client object #171

Closed wvanbergen closed 5 years ago

wvanbergen commented 5 years ago

This adds a client object, which handles all the StatsD metric calls. The current StatsD singleton methods are forwarded to this client object.

This is for two reasons:

  1. I've always disliked that StatsD is a singleton. It means that in some cases, we have had to include a second StatsD client in the same codebase, because we wanted to use a different backend or different settings for certain purposes.

    • E.g., when running the test suite, we generally don't want to send StatsD packets to our StatsD server, except the metadata statistics about the test suite itself.
    • Another example would be to set a tag for all metrics emitted for a given request or job, e.g. shard_id:123. This is not possible right now except for monkey patching.
  2. I have a plan to simplify the client implementation, which would give us a performance boost by having less indirection, fewer if statements, and fewer object allocations. Without keeping the old implementation around, showing that the performance of the new implementation has better performance while maintaining compatibility will be hard.

Performance implications

We do introduce another level of indirection to do this, which affects performance.

On my local machine:

Warming up --------------------------------------
StatsD metrics to /dev/null log (branch: legacy_singleton_client, sha: 15d8d95)
                       790.000  i/100ms
Calculating -------------------------------------
StatsD metrics to /dev/null log (branch: legacy_singleton_client, sha: 15d8d95)
                          8.236k (± 3.1%) i/s -     41.870k in   5.088692s

Comparison:
StatsD metrics to /dev/null log (branch: master, sha: 2e2d6a5):     8678.8 i/s
StatsD metrics to /dev/null log (branch: legacy_singleton_client, sha: 15d8d95):     8235.9 i/s - same-ish: difference falls within error

The performance hit is small (within the range of error). On CI, the hit is a bit more pronounced (in the range of a 5-15% slowdown).

casperisfine commented 5 years ago

It's hard to review without having an idea of where you're trying to go.

wvanbergen commented 5 years ago

172 sketches how I was thinking about implementing the new client.

wvanbergen commented 5 years ago

Now is the right time to start land this I think.

The new client is ready and released in version 2.6, but the singleton methods are still being handled by the old client. In order to have an upgrade path, I'd like to do one more 2.x release that allows you to swap the legacy client with a new one. For that, we will have to move the singleton methods into a legacy client, and forward the StatsD singleton methods to it.

This might come with a bit of a performance hit, but it's pretty small and the evidence has not been clear. But we don't really have a choice if we want to make the upgrade path feasible.

wvanbergen commented 5 years ago
Comparison:
StatsD metrics to local UDP receiver (branch: legacy_singleton_client, sha: 6c88456):     8723.3 i/s
StatsD metrics to local UDP receiver (branch: master, sha: 84bdfec):     8148.9 i/s - same-ish: difference falls within error

Performance comparison after the latest changes: pretty much the same.