Add (optional) statsd metrics reporting

[faern]

To better monitor the health of the tcp2udp (WireGuard over TCP server side component) service, we should add simple error metrics to it. After discussion with @Rizzly we agreed on adding a counter that shows number of connected clients, and an error event when we fail to accept incoming TCP connections.

I added it in such a way that both the library and the binary can be compiled without statsd support. This makes the entire dependency optional. Just like we have done with clap and env_logger. This forced me to expose a dummy-layer of the metrics so the code still builds when it's not enabled, and so the metrics callsites did not become too complex (I did not want the metrics reporting to interfere a lot with the actual forwarding logic code)

---

This change is 

[dlon]

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @faern)

---

src/statsd.rs line 1 at r1 (raw file):

#[cfg(feature = "statsd")]

Might it be cleaner to move the conditionally compiled code into a module instead of having these conditions everywhere?

#[cfg(feature = "statsd")]
mod real {
    const QUEUE_SIZE: usize = 8 * 1024;

    struct StatsdMetricsImpl {
        client: StatsdClient,
        num_connections: AtomicU64,
    }

    impl StatsdMetricsImpl {
    }

}

---

src/statsd.rs line 50 at r1 (raw file):

pub struct StatsdMetrics(Option<StatsdMetricsImpl>);
#[cfg(not(feature = "statsd"))]
pub struct StatsdMetrics(Option<()>);

Would it be more readable to define an enum?

pub struct StatsdMetrics(MetricsImplementation);

enum MetricsImplementation {
    Dummy,
    #[cfg(feature = "statsd")]
    Real(real::StatsdMetrics),
}

impl StatsdMetrics {
    pub fn dummy() -> Self {
        Self(MetricsImplementation::Dummy)
    }

    pub fn accept_error(&self) {
        #[cfg(feature = "statsd")]
        if let MetricsImplementation::Real(statsd) = &self.0 {
            statsd.accept_error()
        }
}

---

src/bin/tcp2udp.rs line 27 at r1 (raw file):

    /// Host to send statsd metrics to.
    #[clap(long)]
    statsd_host: Option<std::net::SocketAddr>,

This could be moved to tcp2udp::Options.

[faern]

Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @dlon)

---

src/statsd.rs line 1 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Might it be cleaner to move the conditionally compiled code into a module instead of having these conditions everywhere?

#[cfg(feature = "statsd")]
mod real {
    const QUEUE_SIZE: usize = 8 * 1024;

    struct StatsdMetricsImpl {
        client: StatsdClient,
        num_connections: AtomicU64,
    }

    impl StatsdMetricsImpl {
    }

}

Yeah, let's do this. The number of conditionally compiled stuff slowly increased without me noticing 😂

---

src/statsd.rs line 50 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Would it be more readable to define an enum?

pub struct StatsdMetrics(MetricsImplementation);

enum MetricsImplementation {
    Dummy,
    #[cfg(feature = "statsd")]
    Real(real::StatsdMetrics),
}

impl StatsdMetrics {
    pub fn dummy() -> Self {
        Self(MetricsImplementation::Dummy)
    }

    pub fn accept_error(&self) {
        #[cfg(feature = "statsd")]
        if let MetricsImplementation::Real(statsd) = &self.0 {
            statsd.accept_error()
        }
}

That's a good idea. I thought about it and rejected it because it would require three types instead of two. But if you count the conditional compilation it's the equal amounts of types. I like your idea.

---

src/bin/tcp2udp.rs line 27 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

This could be moved to tcp2udp::Options.

Done!

[dlon]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @faern)

[faern]

Reviewable status: 6 of 8 files reviewed, 1 unresolved discussion (waiting on @dlon)

---

src/statsd.rs line 139 at r3 (raw file):

        pub fn decr_connections(&self) {
            let num_connections = self.num_connections.fetch_sub(1, Ordering::Relaxed) - 1;

I initially set the memory orderings to SeqCst to not have to think to much about it. But I now relaxed them since they really did not need sequential consistency. No other data depends on there being a memory barrier on this read+write, so it can be relaxed without a problem.

Given how cold this code path is, it probably makes no performance difference at all, but no reason to enforce sequential consistency on all threads for no reason.

[dlon]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @faern)