Skip to content
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

Add statsd sample rate compat #67

Merged
merged 1 commit into from
Dec 20, 2017
Merged

Conversation

sj26
Copy link
Contributor

@sj26 sj26 commented Dec 20, 2017

statsd-ruby takes a simple numeric argument for sample rate:

  # Sends an increment (count = 1) for the given stat to the statsd server.
  #
  # @param [String] stat stat name
  # @param [Numeric] sample_rate sample rate, 1 for always
  # @see #count
  def increment(stat, sample_rate=1)
    count stat, 1, sample_rate
  end

The datadog statsd version only takes sample rate as a keyword option:

    # Sends an increment (count = 1) for the given stat to the statsd server.
    #
    # @param [String] stat stat name
    # @param [Hash] opts the options to create the metric with
    # @option opts [Numeric] :sample_rate sample rate, 1 for always
    # @option opts [Array<String>] :tags An array of tags
    # @option opts [Numeric] :by increment value, default 1
    # @see #count
    def increment(stat, opts={})
      incr_value = opts.fetch(:by, 1)
      count stat, incr_value, opts
    end

The datadog statsd instance doesn't quite provide drop-in compatibility given this argument. We're using a Datadog Statsd instance as a Statsd instance to pass into a third party gem and it uses this argument. Adding compatibility here seems reasonable?

Thanks!

Copy link
Contributor

@masci masci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, thanks for your PR!

@masci masci merged commit ebd7392 into DataDog:master Dec 20, 2017
@sj26
Copy link
Contributor Author

sj26 commented Dec 20, 2017

Thanks @masci! What’s your release schedule like?

@masci
Copy link
Contributor

masci commented Dec 20, 2017

We don't have an actual release schedule, assuming this is blocking the issue on Sidekiq I guess this can go out asap as 3.2.0.
Will make the release before the end of the week, even today if I manage to get some bandwidth.

@mperham
Copy link

mperham commented Dec 20, 2017

I'm going to be rolling out Sidekiq Pro 3.6.1 soon which fixes this issue on my end.

@masci
Copy link
Contributor

masci commented Dec 21, 2017

3.2.0 is out 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants