New hash syntax #34

Merged
merged 6 commits into from Feb 7, 2014

Conversation

Projects
None yet
3 participants
Owner

wvanbergen commented Feb 7, 2014

Changes the sample rate and tags argument to be an option hash:

  • Allows for easier extension with new metric options later on
  • The old syntax is still supported.
  • Metaprogramming methods now also support tags :)
# basic metrics
StatsD.increment('counter', tags: ['a', 'b'])
StatsD.measure('timer', sample_rate: 0.1) { ... }

# metaprogramming
Gateway.statsd_count :sync, 'ActiveMerchant.Gateway.sync', :tags => ['t'], :sample_rate => 1.0

# old syntax still works
StatsD.increment('counter', 1, StatsD.default_sample_rate, ['a', 'b'])

@Soleone @jstorimer

Owner

wvanbergen commented Feb 7, 2014

This should be 100% backwards compatible ❤️

lib/statsd/instrument.rb
@@ -9,6 +9,8 @@ class << self
:prefix, :implementation
end
+ VALID_OPTIONAL_ARGUMENTS = [:sample_rate, :tags, :delta]
@Soleone

Soleone Feb 7, 2014

Member

i guess this array is not used, not sure if you wanna keep it around

@@ -130,39 +130,62 @@ def remove_from_method(method, name, action)
end
# glork:320|ms
- def self.measure(key, milli = nil, sample_rate = default_sample_rate, tags = nil)
+ def self.measure(key, value = nil, *metric_options)
+ if value.is_a?(Hash) && metric_options.empty?
@Soleone

Soleone Feb 7, 2014

Member

not sure if you want/can extract these 4 lines in to a method. you have pretty much the same code again in increment. might not be worth it, but maybe consider it if more public methods are gonna be added and it might potentially be copy/pasted there again.

@wvanbergen

wvanbergen Feb 7, 2014

Owner

I tried to implement it, but it actually gets more messy, because the value not being set means something different depending on the case. The code actually ends up being way more complicated.

Member

Soleone commented Feb 7, 2014

minor comments, looks great. thanks!

Contributor

jstorimer commented Feb 7, 2014

LGTM. This is a great API.

wvanbergen added a commit that referenced this pull request Feb 7, 2014

@wvanbergen wvanbergen merged commit ff47432 into master Feb 7, 2014

1 check passed

default The Travis CI build passed
Details

@wvanbergen wvanbergen deleted the new-hash-syntax branch Feb 7, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment