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 named pipe support for windows #169

Merged
merged 20 commits into from
Dec 16, 2021
Merged

Conversation

randomanderson
Copy link
Contributor

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:

  • Abstracting DatagramChannel into the ClientChannel interface
  • Adding a ClientChannel implementation for named pipes
  • Adding configuration support in the builder
  • Subclassing DummyStatsD server for testing of the 3 transport types

Unfortunately, only existing named pipes can be written to in pure Java. In order to create named pipes for testing, JNA is used.

Copy link
Contributor

@vickenty vickenty left a 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.

@randomanderson
Copy link
Contributor Author

@vickenty Did you want to rereview or should I just merge this PR with the fixes?

@vickenty
Copy link
Contributor

vickenty commented Dec 6, 2021

@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
@randomanderson
Copy link
Contributor Author

@vickenty Windows pipes are only implemented in .NET and the dogstatsd server. They both use DD_DOGSTATSD_PIPE_NAME which is what I used here

@vickenty
Copy link
Contributor

vickenty commented Dec 7, 2021

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?

@randomanderson
Copy link
Contributor Author

@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.

Copy link
Contributor

@vickenty vickenty left a 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this work?

Suggested change
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).

@randomanderson
Copy link
Contributor Author

@vickenty I made that small change. I left the 2-arg constructor because UnixDatagramClientChannel uses it.

@randomanderson randomanderson merged commit 455f4a5 into master Dec 16, 2021
@randomanderson randomanderson deleted the landerson/named-pipes branch December 16, 2021 18:14
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.

2 participants