Refactor #35

Merged
merged 8 commits into from Feb 10, 2014

Conversation

Projects
None yet
4 participants
Owner

wvanbergen commented Feb 7, 2014

  • Add explicit key_value support for statsite. Warning: this means a gauge call no longer is a key_value call on statsite; you need to call StatsD.key_value instead.
  • Simplified the packet generation.
  • Moved some code around, and actually made private methods private :)

I also added some aliases because people were confused by some of the names:

  • Add a StatsD.counter alias for increment.
  • Add a StatsD.timer alias for measure.
Owner

wvanbergen commented Feb 7, 2014

@jstorimer @Soleone @disaacs for review please.

Given that this is slightly backwards compatible for statsite, and because of the syntax change for optional arguments to a hash (even though that's not backwards incompatible), I will release this as version 2.0 afterwards.

Owner

wvanbergen commented Feb 7, 2014

So we could potentially do more backwards incompatible changes if we want to. :)

Contributor

disaacs commented Feb 7, 2014

👍

@Soleone Soleone commented on an outdated diff Feb 7, 2014

lib/statsd/instrument.rb
- # uniques:765|s
- def self.set(key, value, *metric_options)
- collect(:s, key, value, hash_argument(metric_options))
- end
+ alias_method :timer, :measure
@Soleone

Soleone Feb 7, 2014

Member

this is just my humble opinion, but i'm on the fence about aliases in the api.

like i don't see the big advantage of having collect and map or inject and reduce or length, count and size do the same thing in Ruby (well it's not always 100% the same for the latter).

for me two or more choices for the same api call just adds to confusion and is more to learn overall. but i'll leave it to you.

@Soleone Soleone commented on an outdated diff Feb 7, 2014

lib/statsd/instrument.rb
- def self.write_packet(command)
- if mode.to_s == 'production'
- begin
- socket.send(command, 0)
- rescue SocketError, IOError, SystemCallError => e
- logger.error e
- end
- else
- logger.info "[StatsD] #{command}"
+ def hash_argument(args)
+ return {} if args.length == 0
+ return args.first if args.length == 1 && args.first.is_a?(Hash)
+
@Soleone

Soleone Feb 7, 2014

Member

now that i see some NotImplementedErrors in the code I wonder if you want to also raise that or ArgumentError here if tags are being provided and the StatsD.implementation is not :datadog. just a thought

@Soleone

Soleone Feb 7, 2014

Member

nevermind, just seeing the other ArgumentError for tags now in generate_packet

@Soleone Soleone commented on an outdated diff Feb 7, 2014

lib/statsd/instrument.rb
- def self.generate_packet(type, k, v, sample_rate = default_sample_rate, tags = nil)
- command = "#{self.prefix + '.' if self.prefix}#{k}:#{v}"
- case type
- when :incr
- command << '|c'
- when :ms
- command << '|ms'
- when :g
- command << (self.implementation == :statsite ? '|kv' : '|g')
- when :h
- raise NotImplementedError, "Histograms only supported on DataDog implementation." unless self.implementation == :datadog
- command << '|h'
- when :s
- command << '|s'
+ def write_packet(command)
+ if mode.to_s == 'production'
@Soleone

Soleone Feb 7, 2014

Member

doesn't rally matter, but you could do:

def write_packet(command)
  if mode.to_s == 'production'
    send_packet(command)
  else
    logger.info "[StatsD] #{command}"
  end
end

def send_packet(command)
  socket.send(command, 0)
rescue SocketError, IOError, SystemCallError => e
  logger.error e
end

only thinking that because i try to avoid begin whenever i can and instead utilize the implicit begin block of a method definition, less indentation.

@Soleone Soleone commented on an outdated diff Feb 7, 2014

lib/statsd/instrument.rb
end
- command << "\n" if self.implementation == :statsite
- return command
+ def generate_packet(type, k, v, sample_rate = default_sample_rate, tags = nil)
+ command = self.prefix ? self.prefix + '.' : ''
+ command << "#{k}:#{v}|#{type}"
+ command << "|@#{sample_rate}" if sample_rate < 1 || (self.implementation == :statsite && sample_rate > 1)
+ if tags
+ raise ArgumentError, "Tags are only supported on :datadog implementation" unless self.implementation == :datadog
+ command << "|##{clean_tags(tags).join(',')}"
+ end
+
+ command << "\n" if self.implementation == :statsite
+ return command
@Soleone

Soleone Feb 7, 2014

Member

doesn't matter much, but i try to avoid return keyword at the end of a method where we have the implicit return of the last expression: http://monosnap.com/image/DBCKFa1mSpQ2w7JLs4UluwUppktBIV.png

Member

Soleone commented Feb 7, 2014

only random nitpicks, feel free to ignore

Member

Soleone commented Feb 7, 2014

👍

@jstorimer jstorimer commented on the diff Feb 7, 2014

lib/statsd/instrument.rb
end
- command << "\n" if self.implementation == :statsite
- return command
+ def generate_packet(type, k, v, sample_rate = default_sample_rate, tags = nil)
@jstorimer

jstorimer Feb 7, 2014

Contributor

This method is getting a bit hairy with multiple conditionals for the different implementations. A future refactor might add a separate class for each implementation that doesn't have to care about the details of the other implementations.

Contributor

jstorimer commented Feb 7, 2014

Great work maintaining this library Willem. 🚢

Member

Soleone commented Feb 7, 2014

Indeed, thanks a lot @wvanbergen, take all my unicoins: https://unicorn.shopify.com/posts/10068

@wvanbergen wvanbergen merged commit 3cb9e2f into master Feb 10, 2014

1 check passed

default The Travis CI build passed
Details

wvanbergen deleted the consolidate_implementations branch Feb 10, 2014

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