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

Derive Debug trait for all structs and enums #504

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

joshka
Copy link
Contributor

@joshka joshka commented Aug 19, 2024

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

tobz commented Aug 19, 2024

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

@tobz tobz added C-exporter Component: exporters such as Prometheus, TCP, etc. C-util Component: utility classes and helpers. C-core Component: core functionality such as traits, etc. T-ergonomics Type: ergonomics. labels Aug 19, 2024
@joshka
Copy link
Contributor Author

joshka commented Aug 23, 2024

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

tobz commented Aug 25, 2024

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:

  • deriving Debug on types in binary crates (metrics-benchmark, metrics-observer)
  • requiring Debug on traits

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

joshka commented Aug 25, 2024

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).

Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one little missed spot and one nit.

metrics-tracing-context/src/label_filter.rs Outdated Show resolved Hide resolved
metrics-util/src/layers/fanout.rs Outdated Show resolved Hide resolved
This allows all the structs and enums to easily be placed in a struct
which derives Debug itself.
@joshka
Copy link
Contributor Author

joshka commented Sep 2, 2024

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

@tobz tobz merged commit a56fc2f into metrics-rs:main Sep 5, 2024
13 checks passed
@tobz
Copy link
Member

tobz commented Sep 5, 2024

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

@tobz tobz added the S-awaiting-release Status: awaiting a release to be considered fixed/implemented. label Sep 5, 2024
@joshka
Copy link
Contributor Author

joshka commented Sep 6, 2024

No probs.
For more on how this is useful to me, see ratatui/ratatui#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,
}

@joshka joshka deleted the jm/derive-debug branch September 6, 2024 20:26
tobz pushed a commit that referenced this pull request Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-core Component: core functionality such as traits, etc. C-exporter Component: exporters such as Prometheus, TCP, etc. C-util Component: utility classes and helpers. S-awaiting-release Status: awaiting a release to be considered fixed/implemented. T-ergonomics Type: ergonomics.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants