Skip to content

refactor!: make global core optional, add derive macro, add MetricsGroup and MetricsGroupSet traits, reoganize modules #15

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

Merged
merged 36 commits into from
Apr 14, 2025

Conversation

Frando
Copy link
Member

@Frando Frando commented Apr 8, 2025

Description

  • Makes the static superglobal Core optional behind the new static_core feature. The macros to modify metrics tracked in the static core, inc, inc_by, etc are also behind this feature flag now.
  • Makes the Metric trait dyn-compatible, and renames it to MetricsGroup. The name method now takes &self. Adds a register(&self, registry: &mut Registry) method to the Metric trait to register already-constructed metrics to a prometheus-client registry. The new method is moved to a new trait MetricsGroupExt which is auto-impl'd for all types that impl MetricsGroup.
  • Adds a new Metric trait that is implemented on Gauge and Counter to provide a common interface.
  • Adds a custom trait and derive macro Iterable that works similar to struct_iterable, but returns an iterator over &dyn Metric and does not allocate for iteration. Removes the struct_iterable dependency.
  • Adds a derive macro MetricsGroup that expands to the Iterable impl and also implements Default by calling FieldType::new(description) for each field, where description is the first line of the field's doc comment, or a custom string set via #[metrics(description = "my description")]. Also implements MetricsGroup, with a name of either the struct name converted to camel_case, or a custom name set via #[metrics_group(name = "my_name")]
  • Adds a trait MetricsGroupSet that can be implemented on structs that contain multiple structs which each implement Metric. Requires a method groups(&self) -> impl Iterator<Item = &dyn MetricsGroup> to iterate over all contained metric structs. Provides a method register(&self, registry: &mut Registry) which uses iter to register all contained metrics onto a prometheus-client registry. Provides a method iter(&self) -> impl Iterator<Item = (&'static str, &dyn MetricItem> to iterate over all metric items from all contained groups.
  • Modifies the functions to start services (start_metrics_service, start_metrics_dumper, start_metrics_exporter) to take a impl MetricsSource as additional required argument. When the static_core feature is enabled, you can pass the zerosized GlobalRegistry struct, which impls MetricsSource and exposes all metrics contained in the static Core. When not using the static core, you can either pass a Registry or an Arc<RwLock<Registry>> to run the services for a regular registry created in application code.

Breaking Changes

Most import paths changed, but apart from that it is mostly API additions. Migration should be straightforward.

  • iroh_metrics::core::{Counter, Gauge} are now iroh_metrics::{Counter, Gauge}
  • iroh_metrics::core::Metric is now iroh_metrics::MetricsGroup.
    • MetricsGroup::name now takes &self as argument
    • MetricsGroup::new is removed.
  • metrics::{start_metrics_server, start_metrics_exporter, start_metrics_dumper} is now service::{start_metrics_server, start_metrics_exporter, start_metrics_dumper}. They all take a registry argument, which takes any T: impl MetricSource. For the old behavior of using the static Core, pass the zero-sized struct static_core::GlobalRegistry.
  • iroh_metrics::core::Core is now at iroh_metrics::static_core::Core and behind the static_core feature flag (not enabled by default)
  • The macros inc, inc_by, set, dec, dec_by are now behind the static_core feature flag (not enabled by default)
  • iroh_metrics::core module is removed
  • struct_iterable::Iterable is no longer exported from iroh-metrics, and is no longer a supertrait of MetricsGroup
    • instead, MetricsGroup now has iroh_metrics::Iterable and iroh_metrics::IntoIterable supertraits. They can be implemented manually, but it is recommended to use the new MetricsGroup or Iterable derive macros exported from iroh_metrics. See their docs for details.
    • There is now a iroh_metrics::MetricsGroup derive macro that includes the functionality previously provided by struct_iterable.

Notes & open questions

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

Copy link

github-actions bot commented Apr 8, 2025

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh-metrics/pr/15/docs/iroh_metrics/

Last updated: 2025-04-14T10:49:00Z

@n0bot n0bot bot added this to iroh Apr 8, 2025
@github-project-automation github-project-automation bot moved this to 🏗 In progress in iroh Apr 8, 2025
@Frando Frando force-pushed the Frando/refactor branch from 94653e9 to 416ad43 Compare April 8, 2025 13:57
@Frando Frando force-pushed the Frando/refactor branch from 416ad43 to 9ecdf6b Compare April 8, 2025 13:58
Copy link
Contributor

@Arqu Arqu left a comment

Choose a reason for hiding this comment

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

Just a small typo comment, this is great stuff!

src/base.rs Outdated
if let Some(counter) = item.downcast_ref::<Counter>() {
sub_registry.register(metric, counter.description, counter.counter.clone());
}
if let Some(gauge) = item.downcast_ref::<Gauge>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@Frando
Copy link
Member Author

Frando commented Apr 9, 2025

Thanks for the review!

One thing I'm wondering, while we're already doing breaking API changes: To me the trait Metric is a bit confusing, because it sounds like it defines a single metric, whereas it does define a group of metrics. So I'm wondering if we should rename it: Metric would become MetricsGroup, and the new MetricSet would become MetricsGroupSet. To me this is more fitting - wdyt @Arqu?

@Arqu
Copy link
Contributor

Arqu commented Apr 9, 2025

Yeah, that makes sense, at least it should have been a plural like Metrics.
I'd say go for it, given we don't change this code all that often.

@Frando
Copy link
Member Author

Frando commented Apr 9, 2025

I went ahead with the rename: Metric-> MetricsGroup, MetricSet -> MetricsGroupSet

@Frando
Copy link
Member Author

Frando commented Apr 10, 2025

@Arqu @dignifiedquire
Should we go forward with this?
Once merged, I think we'd need a release as well, so that we can update downstream crates (net-tools, iroh, iroh-*) to use the new release, merge their PRs (I'll add the remaining) and then be coherent again on the next iroh release. Or wdyt?

@Arqu
Copy link
Contributor

Arqu commented Apr 10, 2025

I don't see a reason not to, we already decided we want to move forward with it.

@dignifiedquire
Copy link
Contributor

LFG

Frando added 2 commits April 14, 2025 12:45
Make it consistent how the `metrics` feature flag is applied to metrics
methods.
@Frando Frando changed the title refactor: reorganize crate, make global static core optional refactor!: reorganize crate, make global static core optional Apr 14, 2025
@Frando Frando changed the title refactor!: reorganize crate, make global static core optional refactor!: make global core optional, add derive macro, add MetricsGroup and MetricsGroupSet traits, reoganize modules Apr 14, 2025
@Frando Frando merged commit 90f3038 into main Apr 14, 2025
30 of 31 checks passed
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in iroh Apr 14, 2025
@Frando Frando mentioned this pull request Apr 28, 2025
4 tasks
github-merge-queue bot pushed a commit to n0-computer/iroh that referenced this pull request May 5, 2025
## Description

Depends on n0-computer/net-tools#20

Until now, we were using a superglobal static `iroh_metrics::core::Core`
struct to collect metrics into. This allowed us to use macros to track
metrics from anywhere in the codebase. However, this also made it
impossible to collect metrics *per endpoint*, which is what you want
usually as soon as you have more than one endpoint in your app.

This PR builds on n0-computer/iroh-metrics#15,
n0-computer/iroh-metrics#22, and n0-computer/iroh-metrics#23. It removes
the global metrics collection from all crates in the iroh repository.
Instead, we now create and pass metrics collector structs to all places
where we need to collect metrics.

This PR disables the `static_core` feature from iroh-metrics, which
means the macros for superglobal metrics collection are not available
anymore. This is good, because otherwise we could easily miss metrics
not tracked onto the proper metrics collector.

This PR also updates iroh-dns-server and iroh-relay to use manual
metrics collection.

While this means that we have to pass our metrics structs to more
places, it also makes metrics collection more visible, and we can now
also split the metrics structs further easily if we want to separate
concerns more.

This PR should not change anything apart from metrics collection. Most
places are straightforward conversions from the macros to methods on the
metrics collectors. At a few places, logic was changed slightly to move
metrics collection a layer up to save a few clones.

## Breaking Changes

* All metrics structs (`iroh::metrics::{MagicsockMetrics,
PortmapMetrics, NetReportMetrics}`) now implement `MetricsGroup` from
the new version `0.34` of `iroh-metrics` and no longer implement traits
from `[email protected]`.
* Metrics are no longer registered onto the static superglobal `Core`.
`iroh` does not use `static_core` feature of `iroh-metrics`. Metrics are
now exposed from the subsystems that track them, see e.g.
`Endpoint::metrics`.

Several methods now take a `Metrics` argument. You can always pass
`Default::default` if you don't want to unify metrics tracking with
other sections.

#### `iroh`

* `iroh::metrics::{MagicsockMetrics, NetReportMetrics, PortmapMetrics}`
all are now marked `non_exhaustive`, and implement
`iroh_metrics::MetricsGroup` from `[email protected]` and no longer
implement `iroh_metrics::Metric` from `[email protected]`. They also no
longer implement `Clone` (put them into an `Arc` for cloning instead).

* `iroh::net_report::Client::new` now takes
`iroh::net_report::metrics::Metrics` as forth argument

#### `iroh-dns-server`
* `iroh_dns_server::server::Server::spawn` now takes `Metrics` as third
argument
* `iroh_dns_server::ZoneStore::persistent` now takes `Metrics` as third
argument
* `iroh_dns_server::ZoneStore::in_memory` now takes `Metrics` as third
argument
* `iroh_dns_server::ZoneStore::new` now takes `Metrics` as third
argument
* `iroh_dns_server::state::AppState` now has a public `metrics: Metrics`
field
* `iroh_dns_server::dns::DnsHandler::new` now takes `Metrics` as third
argument
* function `iroh_dns_server::metrics::init_metrics` is removed

#### `iroh-relay`

* `iroh_relay::metrics::{StunMetrics, Metrics}` all are now marked
`non_exhaustive`, and implement `iroh_metrics::MetricsGroup` from
`[email protected]` and no longer implement `iroh_metrics::Metric` from
`[email protected]`. They also no longer implement `Clone` (put them
into an `Arc` for cloning instead).

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->

## Change checklist
<!-- Remove any that are not relevant. -->
- [x] Self-review.
- [x] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [x] Tests if relevant.
- [x] All breaking changes documented.
- [x] List all breaking changes in the above "Breaking Changes" section.
- [x] Open an issue or PR on any number0 repos that are affected by this
breaking change. Give guidance on how the updates should be handled or
do the actual updates themselves.
    - [x] [`iroh-gossip`](https://github.com/n0-computer/iroh-gossip)
      - n0-computer/iroh-gossip#58
    - [x] [`iroh-blobs`](https://github.com/n0-computer/iroh-blobs)
      - n0-computer/iroh-blobs#85
    - [x] [`iroh-docs`](https://github.com/n0-computer/iroh-docs)
      - n0-computer/iroh-docs#41

---------

Co-authored-by: dignifiedquire <[email protected]>
Frando added a commit to n0-computer/iroh-gossip that referenced this pull request May 7, 2025
## Description

Adapts iroh-metrics for n0-computer/iroh-metrics#15 and n0-computer/iroh#3262

Depends on n0-computer/iroh#3262

## Breaking Changes


* `metrics::Metrics` now implements `MetricsGroup` from the  ì[email protected]`
* Metrics are no longer tracked into the `static_core` from `iroh-metrics`, but instead are tracked per `Gossip` and exposed via `Gossip::metrics`
* `proto::state::State::handle` now takes `Option<&Metrics>` as new 4th parameter

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR. -->

## Change checklist

- [x] Self-review.
- [x] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant.
- [x] Tests if relevant.
- [x] All breaking changes documented.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants