-
Notifications
You must be signed in to change notification settings - Fork 1
feat(dogstatsd): add configurable SO_RCVBUF for UDP socket #73
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
Changes from all commits
e0ca5d1
8817f53
92133b3
377a20e
35d3972
e28ae7f
8a215bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,7 @@ use std::str::Split; | |||||||||||||||
| use crate::aggregator_service::AggregatorHandle; | ||||||||||||||||
| use crate::errors::ParseError::UnsupportedType; | ||||||||||||||||
| use crate::metric::{id, parse, Metric}; | ||||||||||||||||
| use socket2::{Domain, Protocol, Socket, Type}; | ||||||||||||||||
| use tracing::{debug, error, trace}; | ||||||||||||||||
|
|
||||||||||||||||
| // Windows-specific imports | ||||||||||||||||
|
|
@@ -36,6 +37,9 @@ pub struct DogStatsDConfig { | |||||||||||||||
| /// Optional Windows named pipe name. (e.g., "\\\\.\\pipe\\my_pipe"). | ||||||||||||||||
| #[cfg(all(windows, feature = "windows-pipes"))] | ||||||||||||||||
| pub windows_pipe_name: Option<String>, | ||||||||||||||||
| /// Optional socket receive buffer size (SO_RCVBUF) in bytes. | ||||||||||||||||
| /// If None, uses the OS default. Increase this to reduce packet loss under high load. | ||||||||||||||||
| pub so_rcvbuf: Option<usize>, | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /// Represents the source of a DogStatsD message. Varies by transport method. | ||||||||||||||||
|
|
@@ -174,9 +178,9 @@ impl DogStatsD { | |||||||||||||||
| let addr = format!("{}:{}", config.host, config.port); | ||||||||||||||||
| // TODO (UDS socket) | ||||||||||||||||
| #[allow(clippy::expect_used)] | ||||||||||||||||
| let socket = tokio::net::UdpSocket::bind(addr) | ||||||||||||||||
| let socket = create_udp_socket(&addr, config.so_rcvbuf) | ||||||||||||||||
| .await | ||||||||||||||||
| .expect("couldn't bind to address"); | ||||||||||||||||
| .expect("couldn't create UDP socket"); | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possibility of not-very-specific errors here, since the raw io::Error from bind() and set_nonblocking() Claude Comment:
Non-blocking comment, low change of things breaking, but the socket2 docs do specifically mention they don't do error handling.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Too verbose so far, might improve this in another PR with a cleaner approach |
||||||||||||||||
| BufferReader::UdpSocket(socket) | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -267,6 +271,53 @@ impl DogStatsD { | |||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| async fn create_udp_socket( | ||||||||||||||||
|
duncanista marked this conversation as resolved.
|
||||||||||||||||
| addr: &str, | ||||||||||||||||
| so_rcvbuf: Option<usize>, | ||||||||||||||||
| ) -> std::io::Result<tokio::net::UdpSocket> { | ||||||||||||||||
|
duncanista marked this conversation as resolved.
|
||||||||||||||||
| // Resolve via lookup_host to support hostnames (e.g. "localhost:8125"), | ||||||||||||||||
| // matching the previous behavior of tokio::net::UdpSocket::bind(). | ||||||||||||||||
| let socket_addr = tokio::net::lookup_host(addr).await?.next().ok_or_else(|| { | ||||||||||||||||
| std::io::Error::new( | ||||||||||||||||
| std::io::ErrorKind::InvalidInput, | ||||||||||||||||
| format!("Could not resolve address '{}'", addr), | ||||||||||||||||
| ) | ||||||||||||||||
| })?; | ||||||||||||||||
|
|
||||||||||||||||
| let socket = Socket::new(Domain::IPV4, Type::DGRAM, Some(Protocol::UDP))?; | ||||||||||||||||
|
Lewis-E marked this conversation as resolved.
|
||||||||||||||||
| let socket = Socket::new(Domain::IPV4, Type::DGRAM, Some(Protocol::UDP))?; | |
| let domain = if socket_addr.is_ipv4() { | |
| Domain::IPV4 | |
| } else { | |
| Domain::IPV6 | |
| }; | |
| let socket = Socket::new(domain, Type::DGRAM, Some(Protocol::UDP))?; |
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.
For now, we are always sending localhost addresses, might add in another PR
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.
The PR description mentions configuring this via
DD_DOGSTATSD_SO_RCVBUF, butso_rcvbufis still hard-coded toNonehere. If this binary is expected to expose the knob, wireso_rcvbuffrom env/config (with parsing/validation) so the option is actually usable.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.
Not adding an option a project I don't manage – ensuring that it works as before