Skip to content

refactor!: Make metrics raw atomics, remove prometheus_client dependency #22

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 4 commits into from
Apr 30, 2025

Conversation

Frando
Copy link
Member

@Frando Frando commented Apr 28, 2025

Description

  • Changes the Counter and Gauge to be a wrapper around raw Atomic values, without Arc
  • Removes the Clone requirement from Metric. Instead, the metric groups are Arcd.
  • Removes the help field from Counter and Gauge. Instead, the help is part of the (derived) field iterator, either set from the docstring or with the #[metric(help = "...")] attribute.
  • Adds a Registry to combine multiple metric sources into one
  • Adds encoding methods to encode registries into the OpenMetrics text format

Breaking Changes

  • Metric::help is removed. The help text for metric items is no longer available directly from the item. Instead it is accessible from the MetricsGroup that contains the metrics item. Use MetricsGroup::iter to iterate over the metric items in a group, and MetricItem::help to get the help text for each item.
  • The metric items types, Counter and Gauge, are both changed:
    • They no longer implement Clone. If you need to clone them, put them or an outer struct into an Arc.
    • The new method does not take any args anymore. They now also implement Default.
    • The help method is removed.
  • MetricsIter is no longer needed and removed (replaced by iterable::FieldsIter).
  • The trait MetricsGroupSet has a new required method groups_cloned, returning an iterator of Arc<dyn MetricsGroup>. This means that to implement MetricsGroupSet, a struct has to contain Arc<SomeMetricsGroup>. Because individual metric items are no longer cloneable, this is recommended anyway.
  • MetricsGroup::register and MetricsGroupSet::register are removed. Use Registry::register and Registry::register_all instead.
  • static_core::Core::encode is replaced by static_core::Core::encode_openmetrics
  • The trait iroh_metrics::service::MetricsSource is changed:
    • It is longer behind the services feature flag but always available under iroh_metrics::MetricsSource
    • The required method is now encode_openmetrics(&self, writer: impl std::fmt::Write). To encode directly into a string, use the provided encode_openmetrics_to_string method.

Notes & open questions

Change checklist

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

@Frando Frando force-pushed the Frando/direct-atomics branch from 53cf278 to ab69b10 Compare April 28, 2025 08:41
@Frando Frando mentioned this pull request Apr 28, 2025
4 tasks
}

impl MetricsGroupSet for CombinedMetrics {
fn name(&self) -> &'static str {
"combined"
}

fn groups_cloned(&self) -> impl Iterator<Item = Arc<dyn MetricsGroup>> {
Copy link
Member Author

Choose a reason for hiding this comment

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

#23 adds a derive macro so users don't have to impl this manually.

@@ -30,13 +30,12 @@ unexpected_cfgs = { level = "warn", check-cfg = ["cfg(iroh_docsrs)"] }
unused-async = "warn"

[dependencies]
iroh-metrics-derive = { path = "./iroh-metrics-derive", version = "0.1.0" }
itoa = "1"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is for faster int to string encoding (prometheus_client used it for that purpose, thought it wouldn't hurt here either)

};

/// Trait for structs containing metric items.
pub trait MetricsGroup:
Any + Iterable + IntoIterable + std::fmt::Debug + 'static + Send + Sync
{
/// Registers all metric items in this metrics group to a [`prometheus_client::registry::Registry`].
#[cfg(feature = "metrics")]
fn register(&self, registry: &mut prometheus_client::registry::Registry) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is now done the other way round: Registry::register(&dyn MetricsGroup)

Copy link

github-actions bot commented Apr 28, 2025

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

Last updated: 2025-04-28T11:26:29Z

@Frando Frando marked this pull request as ready for review April 28, 2025 09:06
@Frando Frando changed the title feat/refactor: Make metrics raw atomics, remove prometheus_client dependency refactor!: Make metrics raw atomics, remove prometheus_client dependency Apr 28, 2025
}
writer.write_str(name)?;
Ok(())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

well this was very simple to do :)

@Frando Frando merged commit 27fb9b5 into main Apr 30, 2025
23 of 24 checks passed
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in iroh Apr 30, 2025
@dignifiedquire dignifiedquire deleted the Frando/direct-atomics branch April 30, 2025 08:13
Frando added a commit that referenced this pull request Apr 30, 2025
## Description

* Adds a derive macro for `MetricsGroupSet`

Based on #22

## Breaking Changes
<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->
## 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.
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]>
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