metrics-rs / metrics

A metrics ecosystem for Rust.
MIT License
1.1k stars 151 forks source link

Derive Debug trait for all structs and enums #504

Closed joshka closed 1 week ago

joshka commented 3 weeks ago

This allows all the structs and enums to easily be placed in a struct which derives Debug itself.

The metrics::recorder::Recorder and metrics_tracing_context::LabelFilter traits now also extend Debug.

tobz commented 3 weeks ago

This PR is unreasonably wide reaching. What is it exactly that you're trying to accomplish?

joshka commented 3 weeks ago

The core motivation was to within a library (Ratatui) to be able to centrally define, describe and store Counter/Gauge/Histogram away from the line of code where changes are recorded, to store them for usage from that code. However these don't implement Debug, which means that I'd have to replace all the derived Debug implementations in places that store these with manual implementations. So these made sense to implement Derive for.

After implementing those, I decided that it probably made sense to implement it for everything (per https://rust-lang.github.io/api-guidelines/interoperability.html#c-common-traits) (I'm assuming this was a mistake now and I should have checked). Several of the items where deriving Debug were problematic, and I went with what seemed to be the right approach of manual implementations / adding a Debug constraint to the traits.

Are there any particular parts of this PR that fall into the unreasonable bucket more than others, or is your perspective more that you're unconvinced of the usefulness of generally implement Debug in this crate? My perspective is that implementing Debug is generally a useful thing to do on any item that would potentially ever need to be stored in another item. Is that something you might have differences of opinion on?

tobz commented 2 weeks ago

Overall, I think just blanket deriving Debug has the potential to rapidly clutter debug output, especially with some of the larger structs that are being updated here... but that's not a blocking opinion.

My two no-nos here:

For the first one, this mostly just strikes me as clutter. If there was a strong need to have debug output, they'd have Debug derived or implemented already.

For the second one, it doesn't make sense to make traits a supertrait of common traits unless they actually require that trait's functionality for their own behavior (i.e. the supertrait's effective behavior is the sum of the subtraits) or they would otherwise solve a large ergonomics issue, like having Send and Sync as subtraits on a trait with async functions, where it makes it cleaner downstream by not forcing callers to have to add + Send + Sync to all of their where clauses.

In this case, neither of those scenarios is at play, and more tactically, this would only matter if you're holding a &dyn Recorder (or Box<dyn Recorder, etc), which... I don't see why anybody's code would be doing that. :P

So, all of that said: if we remove the derives from the binary crates, and from traits, I'd be OK with merging the rest.

joshka commented 2 weeks ago

Makes sense. I've removed the derives from the metrics-observer and benches, and removed the trait changes. Rebased on main to pick up the other merged changes.

Clippy lints are unrelated to this change (will submit another PR to clean these up).

joshka commented 1 week ago

Rebased on main. Fixed the unnecessary import. Added _len debug fields on Fanout, FanoutBuilder, Router, and RouterBuilder.

tobz commented 1 week ago

Thanks again for going through all the small nits and revisions on this one. 👍🏻

joshka commented 1 week ago

No probs. For more on how this is useful to me, see https://github.com/ratatui/ratatui/pull/1351, in particular the code where I centralize the initialization of metrics for each struct that uses them (e.g. https://github.com/ratatui/ratatui/pull/1351/files#diff-2c068b77d24467c86dd276124445d84fa48752b9c38e5f444a54f7a27ea2e0e7). I think this pattern makes it easy to use consistent IDs and language in the descriptions.

static METRICS: Lazy<Metrics> = Lazy::new(Metrics::new);

#[derive(Debug)]
struct Metrics {
    pub clear_region_count: Counter,
    pub clear_region_duration: Histogram,
    pub draw_count: Counter,
    pub draw_duration: Histogram,
    pub append_lines_count: Counter,
    pub append_lines_duration: Histogram,
    pub hide_cursor_duration: Histogram,
    pub show_cursor_duration: Histogram,
    pub get_cursor_position_duration: Histogram,
    pub set_cursor_position_duration: Histogram,
    pub size_duration: Histogram,
    pub window_size_duration: Histogram,
    pub flush_count: Counter,
    pub flush_duration: Histogram,
}