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

Test false-positive when tags are updated after metric is aleady emitted. #305

Closed amomchilov closed 2 years ago

amomchilov commented 2 years ago

We have a BaseAction (which models a business action) which automates latency metrics (via StatsD.distribution). It has a method that subclasses can override to define additional tags they want stapled onto this auto-generated metric.

We found out that modifying the tags asynchronously (e.g. in a promise returned from a batch loader) makes the tags not show up in prod (obviously, they've already been ommitted), but they still pass the tests.

Here's a minimal example to illustrate the issue

#!/usr/bin/ruby

require 'bundler/inline'

gemfile(true) do 
  source 'https://rubygems.org'
  gem 'statsd-instrument'
  gem 'minitest'
end

require "minitest/autorun"

class DemoTest < MiniTest::Unit::TestCase
  include StatsD::Instrument::Assertions

  def test_happy_case_has_tags_set
    assert_statsd_distribution("happy_case", tags: { a: 1, b: 2 }) do
      happy_case
    end
  end

  def test_broken_case_still_has_tags_set
    assert_statsd_distribution("broken_case", tags: { a: 1, b: 2 }) do
      broken_case
    end
  end

  private

  def happy_case
    StatsD.distribution("happy_case", tags: { a: 1, b: 2 }) do
      sleep(0.1) # Simulate some work
    end
  end

  def broken_case
    tags = { a: 1 }
    StatsD.distribution("broken_case", tags: tags) do
      sleep(0.1) # Simulate some work
    end

    # At some later point, tags are merged in asynchronously, after the distribution is already done
    tags.merge!(b: 2)
  end
end
amomchilov commented 2 years ago

Hmmmm my reproduction sample works correctly 😬

Closing for now until I can better track down the source of my issue.

Update: it was an error in my usage. Our test never actually simulated this async update. It was setting tags synchronously, which obviously works fine. 🤦