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

Adds support for prefixes to `assert_no_statsd_calls` #329

Closed MiniLuke closed 1 year ago

MiniLuke commented 1 year ago

While using assert_no_statsd_calls, I found it doesn't currently support prefixes which can lead to false successes -- so this PR adds that support.

The current assertions generally expect an un-prefixed metric similar to how you'd emit the metric originally:

client = StatsD::Instrument::Client.new(prefix: "prefix")
assert_statsd_increment("counter", client: client) do
  client.increment("counter")
end
# Success because `prefix.counter` is emitted!

With assert_no_statsd_calls, this is actually succeeding currently:

client = StatsD::Instrument::Client.new(prefix: "prefix")
assert_no_statsd_calls("counter", client: client) do
  client.increment("counter")
end
# Success even though `prefix.counter` was emitted?!

This is because the metric names passed into assert_no_statsd_calls are never processed to append prefixes to them if a prefix was available, but are just used raw: https://github.com/Shopify/statsd-instrument/blob/d5bfc145a06beaba2467170f66782a77057e8bc3/lib/statsd/instrument/assertions.rb#L64

For the positive assertions, they have the prefix appended in the Expectation class: https://github.com/Shopify/statsd-instrument/blob/d5bfc145a06beaba2467170f66782a77057e8bc3/lib/statsd/instrument/expectation.rb#L42

This is especially insidious in an assert_no_... method when you're looking for the absence of something. Since the names aren't processed identically and the comparison finds no matches, it will always falsely succeed if a prefix is defined.

MiniLuke commented 1 year ago

That makes sense to me to make it more resilient to test that had already accounted for this and emit a warning instead.