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

Optimized sending #44

Merged
merged 2 commits into from
Apr 17, 2017
Merged

Optimized sending #44

merged 2 commits into from
Apr 17, 2017

Conversation

AMekss
Copy link
Contributor

@AMekss AMekss commented Mar 10, 2017

Hey there,
First of all thank you for your effort in maintaining this gem.
We're using this gem in production and observed some weird performance issues when statsd wasn't available during the maintenance. We're still investigating what exactly happened. Anyway I played a bit with UDPSockets and did some benchmarks using this script

Here is what it looked like:
When statsd was available

                           user     system      total        real
connect_and_send:      0.070000   0.120000   0.190000 (  0.406062)
just_send:             0.050000   0.100000   0.150000 (  0.279297)

When statsd was down

                           user     system      total        real
connect_and_send:      0.050000   0.100000   0.150000 (  0.156057)
just_send:             0.030000   0.050000   0.080000 (  0.081477)

This PR implements performance improvements based on my findings (also I fixed deprecation warnings for the spec run)

@degemer degemer added this to the 2.3.0 milestone Mar 13, 2017
@degemer degemer self-assigned this Mar 13, 2017
@degemer
Copy link
Member

degemer commented Mar 13, 2017

Thank you very much for your contribution @AMekss !

It's a huge performance improvement! I wanted to compare them both more precisely, so I used this gem, and slightly modified your script there.

Without dogstatsd:

Warming up --------------------------------------
    connect_and_send     1.000  i/100ms
           just_send     8.000  i/100ms
Calculating -------------------------------------
    connect_and_send      4.276  (±23.4%) i/s -     21.000  in   5.043441s
           just_send     85.699  (±12.8%) i/s -    424.000  in   5.059091s

Comparison:
           just_send:       85.7 i/s
    connect_and_send:        4.3 i/s - 20.04x  slower

With dogstatsd:

Warming up --------------------------------------
    connect_and_send     1.000  i/100ms
           just_send     6.000  i/100ms
Calculating -------------------------------------
    connect_and_send      2.654  (± 0.0%) i/s -     14.000  in   5.321916s
           just_send     57.761  (±10.4%) i/s -    288.000  in   5.056199s

Comparison:
           just_send:       57.8 i/s
    connect_and_send:        2.7 i/s - 21.77x  slower

I still want to check a few potential edge cases before merging (results are too good not to have a drawback).

@AMekss
Copy link
Contributor Author

AMekss commented Mar 14, 2017

Sure, its always better be safe and don't rush. Let me know if I can help

@AMekss
Copy link
Contributor Author

AMekss commented Mar 14, 2017

I've noticed that I forgot to remove @host and @port from this line, so I just did that :)

Copy link
Member

@degemer degemer left a comment

Choose a reason for hiding this comment

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

Hello again @AMekss, sorry for the delay!
I have one remark about it, please let me know what you think!

socket = UDPSocket.new
socket.connect(host, port)
socket
rescue => boom
Copy link
Member

Choose a reason for hiding this comment

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

connect shouldn't fail unless the host is unresolvable, and in this case we're probably fine with raising the exception (since it will be unabled to send anything anyway).
I'm not sure what would be the status of the socket if connect fails, most likely it would require using send(message, 0, host, port), and thus would fail afterwards. So I think failing there is better than failing after.
Could you remove the error handling? (unless you think it's needed?)

Copy link
Contributor Author

@AMekss AMekss Mar 23, 2017

Choose a reason for hiding this comment

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

My rationale for that rescue was to make a transparent change which wouldn't affect any of the current behavior, so this could be shipped as a patch version update.
Currently there is no way that Datadog::Statsd.new would raise an exception. While if I would omit this rescue this won't be true any more. So I don't mind to remove this rescue since this PR is aimed for the next minor version upgrade and such change is acceptable.

@AMekss
Copy link
Contributor Author

AMekss commented Mar 24, 2017

@degemer updated!

Copy link
Member

@degemer degemer left a comment

Choose a reason for hiding this comment

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

Thanks a lot @AMekss!
Code looks good, it's ready to be merged.

You're right, this change means that an exception can be raised when initializing the object whereas before no exception could be raised. I think it's very unlikely to affect anyone, but it should still be in 3.0, not 2.3.

I'll check in the next two weeks if we have any other potentially breaking changes we'd like to make for a 3.0 version (we'd like to avoid releasing major too frequently), if not this will be the only change.

@AMekss
Copy link
Contributor Author

AMekss commented Mar 28, 2017

Ok, thanks, looking forward for the release

@AMekss
Copy link
Contributor Author

AMekss commented Mar 29, 2017

Recently I came across why it's faster:

When you pass in host and port into the send method it DNS resolves before each send, which has major overhead.

source

@degemer degemer modified the milestones: 3.0, 2.3.0 Apr 17, 2017
@degemer degemer merged commit 9b9eb9a into DataDog:master Apr 17, 2017
@bobbytables
Copy link

Hey @AMekss , Thanks for adding this PR to this project. For fun, I thought I'd show you our graph after shipping this.

pasted_image_6_14_17__4_03_pm

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

Successfully merging this pull request may close these issues.

3 participants