-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow :prefix option on metrics creation #148
Conversation
@@ -2,7 +2,7 @@ require 'bundler/gem_tasks' | |||
require 'rake/testtask' | |||
|
|||
Rake::TestTask.new('test') do |t| | |||
t.ruby_opts << '-rubygems' | |||
t.ruby_opts << '-r rubygems' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated change>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI was complaining about this (and was in fact preventing me from testing locally)
lib/statsd/instrument/metric.rb
Outdated
@@ -47,7 +49,11 @@ def initialize(options = {}) | |||
@type = options[:type] or raise ArgumentError, "Metric :type is required." | |||
@name = options[:name] or raise ArgumentError, "Metric :name is required." | |||
@name = normalize_name(@name) | |||
@name = StatsD.prefix ? "#{StatsD.prefix}.#{@name}" : @name unless options[:no_prefix] | |||
@name = if options[:prefix] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if prefix & no_prefix are set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefix
will override (which I should document).
I could be persuaded to have :no_prefix
have greater precedence but it's 6 of 1, half-dozen of the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. no_prefix
having precedence sounds less surprising to me.
lib/statsd/instrument/metric.rb
Outdated
@@ -47,7 +49,11 @@ def initialize(options = {}) | |||
@type = options[:type] or raise ArgumentError, "Metric :type is required." | |||
@name = options[:name] or raise ArgumentError, "Metric :name is required." | |||
@name = normalize_name(@name) | |||
@name = StatsD.prefix ? "#{StatsD.prefix}.#{@name}" : @name unless options[:no_prefix] | |||
@name = if options[:prefix] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. no_prefix
having precedence sounds less surprising to me.
Closes #69
Allows clients to pass in
:prefix
as an option to a metric. This allows classes that extend StatsD to avoid polluting the global namespace. The motivation for this is KubernetesDeploy #384, though it seems it's not the first app to feel the need for this.The change is backwards compatible and purely opt-in