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

enhancement(metrics): add support for scoped (local) recorders #414

Merged
merged 17 commits into from
Dec 23, 2023

Conversation

tobz
Copy link
Member

@tobz tobz commented Nov 23, 2023

Context

In #389, it's pointed out that the current situation of needing to use the undocumented metrics::clear_recorder() for testing recorders is very suboptimal. Installation of the global recorder is meant to happen once during the life of a process. Clearing the global recorder to install another is suboptimal for more than a few reasons, too:

  • the old global recorder is leaked (set_boxed_recorder leaks the box if the recorder is already set #409 touches on this due to another related behavior, but it also applies here) which can lead to ASAN warnings
  • it's subject to race conditions since the default Cargo test runner will run unit tests in parallel
  • it depends on an undocumented function, which means we're trading the ergonomics of testing recorder implementations for hiding a rightfully dangerous function from normal users

Solution

In this PR, we've introduced the concept of scoped recorders, or local recorders as we call them. This is modeled after tracing, where a free function takes a reference to the dispatcher to temporarily set as the default, and a closure to run where only that overridden default dispatcher applies. It works on a thread-local basis so one instance of using a local recorder never conflicts with another.

A new function, metrics::with_local_recorder, takes a &dyn Recorder and a closure to run where the recorder used by the convenience macros -- counter!, etc -- will be temporarily overridden to use the given recorder.

While this feature primarily supports the ability to properly test recorder implementations, it has a side benefit that it also allows writing a Recorder implementation that does not need to be safe for concurrent use. As the local recorder override only applies to the current thread, the recorder can use something like RefCell internally, and entirely avoid locks, concurrent data structures, atomics, and so on. Between only needing to pass a temporary reference to the recorder, this means that single threaded use cases that wish to avoid the additional overhead of all of that now have the ability to do so.

Additionally, we've also changed the function for installing a global recorder, removing metrics::set_recorder and renaming metrics::set_boxed_recorder to metrics::set_global_recorder. The functions have been changed to be slightly more ergonomic (handle boxing the value for callers) but to also remove the illusion that metrics is designed to work well in no_std environments, which is generally the only reason a user would ever install a recorder via a &'static dyn Recorder reference.

Fixes #389.
Fixes #409.

@tobz tobz changed the title fix(metrics): ensure boxed recorders are dropped when installation fails enhancement(metrics): add support for scoped (local) recorders Nov 25, 2023
@tobz tobz force-pushed the tobz/better-drop-handling-for-recorder branch from ce929a4 to 34ca1c9 Compare December 23, 2023 15:09
@tobz tobz merged commit 0e5874d into main Dec 23, 2023
12 checks passed
@tobz tobz deleted the tobz/better-drop-handling-for-recorder branch December 23, 2023 23:26
@tobz tobz added C-core Component: core functionality such as traits, etc. E-complex Effort: complex. T-enhancement Type: enhancement. S-awaiting-release Status: awaiting a release to be considered fixed/implemented. T-ergonomics Type: ergonomics. and removed S-awaiting-release Status: awaiting a release to be considered fixed/implemented. labels Dec 24, 2023
ijc added a commit to ijc/metrics-exporter-statsd that referenced this pull request Jan 8, 2024
metrics-rs/metrics@metrics-v0.21.1...metrics-v0.22.0

There are some breaking changes:

- metrics-rs/metrics#414 Added a generic recorder to
  `SetRecorderError` and removed `set_boxed_recorder`
- metrics-rs/metrics#380 Added metadata to the metrics
- metrics-rs/metrics#394 Reworked the macros
bltavares added a commit to bltavares/axum-prometheus that referenced this pull request Jan 22, 2024
The current version of metrics (0.21.0) uses an older version of macros that
point to different global allocators than the current latest version (0.22.0)
causing metrics to not be reported by other tools integrating with the
`metrics` facade.

Since metrics-rs/metrics#414, the `metrics` project now
can have different registries, and such, the macros (eg: `counter!`) have been refactored.

As this is a pre-1.0 project, including axum-prometheus into a project with
`metrics:0.22.0` register 2 different versions of the metrics crate. So metrics
reported by `axum-prometheus` will be registered on the `metrics:0.21.0` global
collector, while the project's metrics will use the `metrics:0.22.0` global
collector.

While the current suggested method of exporting the `let (_, handle)` as a
route will work, as it's pointing to the `metrics:0.21.0` collectors, other
integrations such as the push-based background task will miss out the layer
metrics.

By upgrading this project to `metrics:0.22.0` we can ensure it's reporting to
the same global collector, and thus integrated with other `metrics`-based
tools.

This commit bumps the metrics version to 0.22.0 as well as fix the macro
invocations introduced on the new version.
mnpw pushed a commit to mnpw/metrics that referenced this pull request Mar 8, 2024
ijc added a commit to ijc/metrics-exporter-dogstatsd that referenced this pull request Nov 13, 2024
metrics-rs/metrics@metrics-v0.21.1...metrics-v0.22.0

There are some breaking changes:

- metrics-rs/metrics#414 Added a generic recorder to
`SetRecorderError` and removed `set_boxed_recorder`
- metrics-rs/metrics#380 Added metadata to the metrics
kp-mariappan-ramasamy pushed a commit to kp-mariappan-ramasamy/metrics-exporter-dogstatsd that referenced this pull request Nov 19, 2024
metrics-rs/metrics@metrics-v0.21.1...metrics-v0.22.0

There are some breaking changes:

- metrics-rs/metrics#414 Added a generic recorder to
`SetRecorderError` and removed `set_boxed_recorder`
- metrics-rs/metrics#380 Added metadata to the metrics
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. E-complex Effort: complex. T-enhancement Type: enhancement. T-ergonomics Type: ergonomics.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

set_boxed_recorder leaks the box if the recorder is already set Scoped recorders
1 participant