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 UDS support #42

Merged
merged 8 commits into from
Jun 26, 2018
Merged

Add UDS support #42

merged 8 commits into from
Jun 26, 2018

Conversation

xvello
Copy link
Contributor

@xvello xvello commented Jan 8, 2018

This PR adds support for submission of custom metrics through a Unix Socket on Linux.
See https://docs-staging.datadoghq.com/xvello/dsd_uds/developers/dogstatsd/unix_socket/ and https://github.com/DataDog/datadog-agent/wiki/Unix-Domain-Sockets-support for context and benefits.

Implementation details:

  • no change in the public API, UDS mode is triggered by setting 0 as port, the socket path as hostname
  • the send timeout is set at 100ms. It only triggers if dogstatsd is overloaded/frozen and the socket buffer is full. The equivalent in UDP is a full reception buffer, triggering a silent packet drop
  • the default behaviour of the library is to ignore all exceptions, but a custom handler can be passed. Documented the expected exceptions and their meaning

Testing

  • Reduced the parallelism of NonBlockingStatsDClientPerfTest (from 20 to 10) to reduce flakes on Travis
  • Added the UnixSocketTest test class to test good transmission + error handling and recovery

Windows

The feature is Linux only, and requires an external jnr.unixsocket JNI library. This library provides NOOP stubs for unsupported OSes, so the library still compiles and operates in UDP mode with no dark magic.

Tests pass on windows, all of the UnixSocketTest class is ignored of course. As with Linux, the NonBlockingStatsDClientPerfTest test is still a bit flaky.

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running com.timgroup.statsd.EventTest
Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.047 sec
Running com.timgroup.statsd.NonBlockingStatsDClientPerfTest
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.172 sec
Running com.timgroup.statsd.NonBlockingStatsDClientTest
Tests run: 44, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.703 sec
Running com.timgroup.statsd.UnixSocketTest
Tests run: 1, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0 sec

Results :

Tests run: 51, Failures: 0, Errors: 0, Skipped: 1

@masci masci added this to the 2.6 milestone Jan 23, 2018
@erks
Copy link

erks commented Apr 17, 2018

Hi @xvello @masci, any ETA on when this can be merged?

@xvello
Copy link
Contributor Author

xvello commented Apr 18, 2018

Hello @erks,

The code is manually tested and ready for code review, I need to work on the unit testing before merging it. As we are currently focusing on the 6.2.0 release, I hope I'll be able to return to it in 2 weeks.

In the meantime, feedback on the PR and testing with your use case would be greatly appreciated.

@xvello
Copy link
Contributor Author

xvello commented May 16, 2018

@nmuesch @masci Tests are green and documentation is updated, could you please review?

@erks Did you have time to test the branch yet?

@erks
Copy link

erks commented May 16, 2018

@xvello No, I did not, unfortunately.

@sean-madden
Copy link

Any update on this PR? Exciting feature!

@hkaj hkaj requested a review from nmuesch June 25, 2018 13:16
@nmuesch
Copy link
Contributor

nmuesch commented Jun 25, 2018

Hey, apologies for the delay. Having a look at this today! Thanks for the improvements!!

@xvello xvello changed the title [WIP] add UDS support Add UDS support Jun 26, 2018
Copy link
Contributor

@nmuesch nmuesch left a comment

Choose a reason for hiding this comment

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

This looks 💯 Thanks! I was able to test this manually and submit metrics through the unix socket. Also, thanks for the tests!

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.

5 participants