56quarters / cadence

An extensible Statsd client for Rust
https://docs.rs/cadence/
Apache License 2.0
87 stars 27 forks source link

macro variants that do not panic if global default client is not set #210

Open danielnorberg opened 5 months ago

danielnorberg commented 5 months ago

Thank you a lot for this library!

Would it make sense to add variants of the statsd macros that do not panic if the global default client is not set?

When testing code makes use of the statsd macros it is sometimes awkward to have to initialize the global default client in unit tests to avoid the macros panicking.

Would be happy to contribute a PR to add such macros if you think it would make sense.

Wdyt?

56quarters commented 5 months ago

Thanks for the issue! I can understand that using the macros makes tests hard to write. I'm not sure about adding non-panicking versions since that seems like it would make it even harder to detect incorrect configurations (and it's already UDP in a lot of cases).

I'm curious about your use case and how the macros and statsd are involved in the tests. Does the code under test make metrics calls that you'd like to verify as part of the tests? Or does it just happen to make metrics calls that you don't care about while testing? If it's the latter, there's no harm in calling cadence_macros::set_global_client() multiple times in tests - any calls after the first time are ignored.

SeanGrady commented 3 weeks ago

I'm running into the same thing -- for me, at least, it's that there are metrics in code that needs to be tested, but I don't care about the metrics during the tests. As far as I can tell, my options are:

None of these are great, and I also think it might make more sense to have the macros just not panic if there's no global default set, which is what the log crate does for its logging macros.

arthuratplayableworlds commented 3 weeks ago

It might also be reasonable to have something like a feature flag that sets the default to a dummy macro. That way you could set your test modules to include that feature flag and not have to worry about it, but still get the panic behavior in production builds.

arthuratplayableworlds commented 3 weeks ago

Do you take PR's? I just tried out this change in macros.rs and it works for my needs.

#[doc(hidden)]
#[cfg(not(feature="disable_metrics"))]
macro_rules! _generate_impl {
    ($method:ident, $key:expr, $val:expr, $($tag_key:expr => $tag_val:expr),*) => {
        use cadence::prelude::*;
        let client = $crate::get_global_default().unwrap();
        let builder = client.$method($key, $val);
        $(let builder = builder.with_tag($tag_key, $tag_val);)*
        builder.send()
    }
}
#[macro_export]
#[doc(hidden)]
#[cfg(feature="disable_metrics")]
macro_rules! _generate_impl {
    ($method:ident, $key:expr, $val:expr, $($tag_key:expr => $tag_val:expr),*) => {
    }
}

This causes the macros to expand to nothing if the flag is set, then in my dev-dependencies I specify the disable_metrics feature and things just work.

56quarters commented 3 weeks ago

I'd rather not introduce a configuration feature for this. If the log! macro silently drops calls when there's no backend configured, that's good enough for me. However, because this is a breaking change it'll be a little while before I make it.