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

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

Open
danielnorberg opened this issue May 31, 2024 · 5 comments
Open

Comments

@danielnorberg
Copy link

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
Copy link
Owner

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
Copy link

SeanGrady commented Oct 28, 2024

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:

  • Write my own macros
  • Have every cadence macro invocation in my entire codebase wrapped in an if cadence_macros.is_global_default_set() { statsd_gauge!(<etc>)) } check
  • Initialize a dummy global client in every single test

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
Copy link

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
Copy link

arthuratplayableworlds commented Oct 29, 2024

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
Copy link
Owner

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.

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

No branches or pull requests

4 participants