-
Notifications
You must be signed in to change notification settings - Fork 158
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
force-pushed
the
tobz/better-drop-handling-for-recorder
branch
from
December 23, 2023 15:09
ce929a4
to
34ca1c9
Compare
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.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: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 warningsSolution
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 likeRefCell
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 renamingmetrics::set_boxed_recorder
tometrics::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 thatmetrics
is designed to work well inno_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.