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

Make assert_no_statsd_calls more flexible #235

Closed sambostock closed 4 years ago

sambostock commented 4 years ago

Currently, assert_no_statsd_calls has two modes:

This adds flexibility in the following ways:

assert_no_statsd_calls(/\.metric/)

This is useful in cases where the number of metrics to watch out for is impractical, but detecting all metrics would be too restrictive e.g. any metric with a certain prefix

Any combination thereof ```ruby assert_no_statsd_calls('my.metric', /your/) ```
wvanbergen commented 4 years ago

I am not a big fan of the regular expression option. When in practice do you need that?

sambostock commented 4 years ago

@wvanbergen I have a job which forwards some statistics we get from Twilio about our queues. I would like to use this change to do something like this:

datagrams = capture_stats_d_calls do
  forward_statistics_from_twilio_to_datadog
end

assert_statsd_distribution('twilio.statistics.realtime.a_metric', datagrams: datagrams)
assert_statsd_distribution('twilio.statistics.realtime.another_metric', datagrams: datagrams)
# ...
assert_statsd_distribution('twilio.statistics.realtime.yet_another_metric', datagrams: datagrams)

# Make sure this test covers all the relevant emitted metrics
assert_no_statsd_calls(/^twilio\.statistics\.realtime\./, datagrams: datagrams)

I'm currently doing assert_no_statsd_calls(datagrams: datagrams), but that seems too restrictive, and subject to unrelated failures. For example, if we were to start emitting metrics around the API calls we make to Twilio.

I could use assert_no_statsd_calls('...', datagrams: datagrams), but that would effectively mean listing every metric with that prefix, and need to be updated any time another metric is added, to ensure this test covers all relevant metrics.

wvanbergen commented 4 years ago

This can be done fairly cleanly after capturing the datagrams assert datagrams.none? { |datagram| datagram.name.match?(/^twilio\.statistics\.realtime\./) }

sambostock commented 4 years ago

Another alternative would be to start supporting a wildcard, so you could do something like

assert_no_statsd_calls('twilio.statistics.realtime.*')
sambostock commented 4 years ago

This can be done fairly cleanly after capturing the datagrams assert datagrams.none? { |datagram| datagram.name.match?(/^twilio\.statistics\.realtime\./) }

Ahh, good point. I feel like matching by prefix (not the STATSD_PREFIX, but the first parts of the metric) wouldn't be a bad middle ground, but that's just me.

Do you think the variadic arguments make sense, or should I close this all together?

wvanbergen commented 4 years ago

I just ended up doing a big pass of removing features that got added over time to serve minor use cases that made maintenance of the library harder in the long run. So I rather not start re-adding features like this unless we have some evidence that this is a very common use case.

Moreover, I specifically want to push back against features that make it easier to deal with large amount of different metrics or tags, or merely by their existence "legitimizes" using a high number of metrics or tags. We have a problem of having way too many metrics. That costs us a lot of money, makes metrics less discoverable, and makes the UI slower. I would like this library to nudge users in the right direction of using as little metrics/tags as possible by having to be more precies & specific in their metric usage.

If you want, you can still do this with the existing helper methods with the oneliner I gave you, or write your own helper method in your app. But do you really need to emit all of those metrics?

wvanbergen commented 4 years ago

I am OK with the variadic arguments, because it still requires you to be precise

sambostock commented 4 years ago

I am OK with the variadic arguments, because it still requires you to be precise

Cool. I'll shrink this PR down to just that. 👌

removing features that got added over time to serve minor use cases that made maintenance of the library harder in the long run

Fair 👍

really need to emit all of those metrics?

We're trying to replicate a dashboard that exists inside one of Twilio's products, which requires a license to access. Moreover, having the data in Datadog allows us to build dashboard which surface the queues across the various tools, since not everything is in Twilio.

sambostock commented 4 years ago

@wvanbergen I've updated the code and PR description to remove the RegExp support, and leave variadic arguments. If approved, I will squash the commits together before merging.

wvanbergen commented 4 years ago

Only thing missing is a CHANGELOG entry.