-
Notifications
You must be signed in to change notification settings - Fork 101
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 named pipe support for windows #169
Conversation
9b2869a
to
33f76a4
Compare
33f76a4
to
aaca3ce
Compare
f052c3f
to
b2f8caa
Compare
75010ff
to
dcfe5aa
Compare
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.
Looks mostly good. Left a few comments inline, please let me know what you think.
src/main/java/com/timgroup/statsd/NonBlockingStatsDClientBuilder.java
Outdated
Show resolved
Hide resolved
29b4638
to
5f7ddbe
Compare
@vickenty Did you want to rereview or should I just merge this PR with the fixes? |
@randomanderson Sorry for the delay, I will take another look. Meanwhile, we would like to make sure configuration options are the same between libraries for different languages. Windows pipes are not universally supported by all libraries we support, but where it is available, we'd like the configuration to be the same (same env vars / methods). Can you please check if we're aligned with https://github.com/DataDog/datadog-go for example? |
# Conflicts: # src/test/java/com/timgroup/statsd/TelemetryTest.java
@vickenty Windows pipes are only implemented in .NET and the dogstatsd server. They both use |
There are some warnings in the CI, would you mind taking a look and fixing if any are new? Do you think it might be useful to throw an error if named pipe is requested on a non-windows OS? |
@vickenty There were a few javadoc warnings that I fixed. The rest of the warnings were preexisting (one about the deploy plugin missing a version and one about the EnvironmentVariables test fixture) There's no error for Unix Domain Sockets on Windows so I don't think there needs to be one for named pipes. If the named pipe does not exist, an IOException is thrown. |
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.
LGTM, except one small nit.
} else if (address instanceof UnixSocketAddress) { | ||
return new UnixDatagramClientChannel(address, timeout, bufferSize); | ||
} else { | ||
return new DatagramClientChannel(DatagramChannel.open(), address); |
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.
Would this work?
return new DatagramClientChannel(DatagramChannel.open(), address); | |
return new DatagramClientChannel(address); |
And then DatagramChannel import can be removed, and maybe 2-argument constructor too (other client channel impls don't have one, so I guess we don't need one there too).
@vickenty I made that small change. I left the 2-arg constructor because |
This PR adds named pipe support for windows machines. Named pipes are similar to unix domain sockets. This PR supports submitting metrics to Datadog in Azure App Services (https://github.com/DataDog/datadog-aas-extension).
The main changes are:
DatagramChannel
into theClientChannel
interfaceClientChannel
implementation for named pipesDummyStatsD
server for testing of the 3 transport typesUnfortunately, only existing named pipes can be written to in pure Java. In order to create named pipes for testing, JNA is used.