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 event & service checks #109

Merged
merged 3 commits into from
May 17, 2018
Merged

Add statsd event & service checks #109

merged 3 commits into from
May 17, 2018

Conversation

maximebedard
Copy link

@maximebedard maximebedard commented Oct 20, 2017

Fixes #89

This exposes 2 datadog specific metric types: event and service checks.

Few questions before I proceed with this:

Do we mind keeping UdpBackend.new("localhost:1234", :datadog)? Because I would change this public api to expose directly the protocol supported. E.g.

# before
StatsD.backend = UdpBackend.new("localhost:1234", :datadog)

# after
StatsD.backend = UdpBackend.new("localhost:1234", UdpBackend::DatadogStatsDProtocol.new)

This way, it's easier to define what a specific implementation can and can't do.

@wvanbergen WDYT?

@wvanbergen
Copy link
Contributor

Do we mind keeping UdpBackend.new("localhost:1234", :datadog)? Because I would change this public api to expose directly the protocol supported.

Yes; we have too many users of this gem and I don't want to spend time updating all of them. I also don't find this a good enough reason to break backwards compatibility.

Also, can you run some benchmarks to see if this refactor has any impact on the performance of the hot code paths?

@@ -3,21 +3,108 @@
module StatsD::Instrument::Backends
class UDPBackend < StatsD::Instrument::Backend

class DatadogStatsDProtocol
Copy link
Contributor

Choose a reason for hiding this comment

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

You can call it DogStatsDProtocol, which matches Datadog's naming.


def initialize(server = nil, implementation = nil)
super()
self.server = server || "localhost:8125"
self.implementation = (implementation || DEFAULT_IMPLEMENTATION).to_sym
end

def implementation=(value)
@packet_factory =
case value
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I prefer the case to be after the = sign.

Copy link
Contributor

@wvanbergen wvanbergen left a comment

Choose a reason for hiding this comment

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

If you can show that this doesn't impact hot path performance, and is backwards compatible, 🚢

@fmejia97
Copy link
Contributor

@wvanbergen Also, can you run some benchmarks to see if this refactor has any impact on the performance of the hot code paths? I'm not too familiar with how to go about doing this. Could you give me some pointers? What do you mean by hot code paths?

@wvanbergen
Copy link
Contributor

Its primarily sending a metric. You can measure it using benchmark-ips, and compare before and after.

@fmejia97
Copy link
Contributor

BENCHMARKING RESULTS

TLDR: The number of iterations /s on hot code path blocks increased by 16% on average.

Below I'm showing the results of benchmarking I carried out for the new changes.

Benchmarking the new changes (rebased with current master)

Warming up --------------------------------------
increment metric benchmark
20.679k i/100ms
measure metric benchmark
16.442k i/100ms
gauge metric benchmark
20.935k i/100ms
set metric benchmark 15.829k i/100ms
Calculating -------------------------------------
increment metric benchmark
227.577k (± 5.5%) i/s - 1.137M in 5.013417s
measure metric benchmark
178.828k (± 4.0%) i/s - 904.310k in 5.065022s
gauge metric benchmark
230.724k (± 4.9%) i/s - 1.151M in 5.003040s
set metric benchmark 167.083k (± 5.0%) i/s - 838.937k in 5.034690s

Benchmarking the current master

Warming up --------------------------------------
increment metric benchmark
24.810k i/100ms
measure metric benchmark
18.099k i/100ms
gauge metric benchmark
25.025k i/100ms
set metric benchmark 18.508k i/100ms
Calculating -------------------------------------
increment metric benchmark
275.077k (± 6.5%) i/s - 1.389M in 5.071140s
measure metric benchmark
195.861k (± 9.2%) i/s - 977.346k in 5.026521s
gauge metric benchmark
274.615k (± 5.8%) i/s - 1.376M in 5.028793s
set metric benchmark 190.630k (± 4.3%) i/s - 962.416k in 5.057944s

@fmejia97
Copy link
Contributor

fmejia97 commented May 17, 2018

@wvanbergen I carried out some benchmarking tests and posted the results above. Performance on hot code paths was affected by 16%. I don't think this is too much of a concern. What are your thoughts?

@wvanbergen
Copy link
Contributor

Is the refactor necessary to add the new features? I assume it's the additional indirection that is costing us some performance.

@maximebedard
Copy link
Author

I'm pretty curious about this 16% increase. I assume the method dispatch is more costly than I assumed it would :(

@fmejia97
Copy link
Contributor

Created a new PR that adds the same functionality without the refactoring, and benchmarked the changes (https://github.com/Shopify/statsd-instrument/pull/122/files). The performance hit increased to a value of 42%. We asked on #core-cloud-migration and they didn’t veto the 16% performance hit. I'll merge the changes (approval by Willem).

@fmejia97 fmejia97 merged commit bf6c1a7 into master May 17, 2018
@fmejia97 fmejia97 deployed to rubygems May 18, 2018 15:24 Active
ElvinEfendi added a commit that referenced this pull request May 22, 2018
ElvinEfendi added a commit that referenced this pull request May 22, 2018
ElvinEfendi added a commit that referenced this pull request May 23, 2018
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