Update metrics dependency from 0.21.0 to 0.22.0
#34
Conversation
WalkthroughThe project has undergone a cleanup and refactoring phase. A step in the CI pipeline for diff checking has been removed, and the formatting step has been made more stringent. Documentation has been polished across several markdown files, with improved readability and style. The core library code has seen a refactor, with a structural change in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- Cargo.toml
- examples/basic/Cargo.toml
- examples/with_axum/Cargo.toml
Files selected for processing (5)
- .github/workflows/test.yml (1 hunks)
- README.md (4 hunks)
- examples/basic/README.md (1 hunks)
- examples/with_axum/README.md (1 hunks)
- src/lib.rs (5 hunks)
Files skipped from review due to trivial changes (4)
- .github/workflows/test.yml
- README.md
- examples/basic/README.md
- examples/with_axum/README.md
Additional comments: 6
src/lib.rs (6)
1-1: Including the README.md content directly in the library documentation using
include_str!.10-17: Added individual fields for metrics to the
Collectorstruct.30-39: The
defaultmethod initializes the new fields with static metric names.51-62: The
newmethod usesformat!andleak()to create 'static strings for metric names with a prefix. This approach permanently allocates memory for these strings, which may not be reclaimed. Ensure this is the intended behavior and acceptable in the context of this library's usage patterns.83-125: The
describemethod now uses the new static string fields to describe gauges.147-175: The
collectmethod now uses the new static string fields to collect metrics. This change should improve performance by reducing memory allocations.
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
Thank you for your contribution. I appreciate it. I have a few requests for adjustments:
-
Each PR should address a single topic. Please split the PR into multiple ones, each focusing on a specific topic.
-
Generally, we discourage introducing breaking changes. If there's a significant benefit to the new style, please provide a clear explanation. Otherwise, consider maintaining the current API.
-
Regarding the
README.mdfile, I prefer not to include it in thelib.rsfile since the content serves a slightly different purpose.
Additionally, please craft a more informative commit message.
src/lib.rs
Outdated
| pub fn new(prefix: impl AsRef<str>) -> Self { | ||
| let prefix = prefix.as_ref(); | ||
| Self { | ||
| cpu_seconds_total: format!("{prefix}process_cpu_seconds_total").leak(), |
There was a problem hiding this comment.
I don't think it's good idea to use leak() here.
There was a problem hiding this comment.
This way we are allocating the String only once, and then reuse it in gauge!() macros for metrics names. The &'static str also helps -- it unties the lifetime from the &self reference.
The only downside of leaking is that the data of Strings allocated won't be freed until the program exits. In a contrived scenario, where one creates a ton of Collectors with prefix provided and then drops them this would cause problems. But from my understanding, the Collector is meant to be created once at the start and used for the entire runtime of the program.
Still, we can counter this by Box::leaking the strings allocated and then giving the Collector the Drop impl that will unsafely Box::from_raw them to drop properly. If you're fine with that, I'll implement this in differend PR.
There was a problem hiding this comment.
How about utilizing String to store each label instead of &'static str in Collectors? I'm asking because the advantages of using &'static str in Collectors are not clear to me. Is there a significant difference in memory usage or any other factors to consider?
src/lib.rs
Outdated
| pub fn prefix(mut self, prefix: impl Into<String>) -> Self { | ||
| self.prefix = prefix.into(); | ||
| self | ||
| pub fn new(prefix: impl AsRef<str>) -> Self { |
There was a problem hiding this comment.
Breaking changes without significant benefit is not acceptable.
There was a problem hiding this comment.
I'll bring back prefix so that it will work as before.
There was a problem hiding this comment.
Sad to use impl Into<String> in it though, as it is not required to build a String.
There was a problem hiding this comment.
Changing from impl Into<String> to impl AsRef<str> is OK because it's easy to notice and fix.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- Cargo.toml
- examples/basic/Cargo.toml
- examples/with_axum/Cargo.toml
Files selected for processing (3)
- README.md (4 hunks)
- examples/with_axum/src/main.rs (3 hunks)
- src/lib.rs (6 hunks)
Files skipped from review due to trivial changes (1)
- README.md
Files skipped from review as they are similar to previous changes (1)
- src/lib.rs
Additional comments: 1
examples/with_axum/src/main.rs (1)
- 1-4: The introduction of
axumandtokioimports aligns with the changes to the server setup. Ensure that these new dependencies are reflected inCargo.tomland that their versions are compatible with the rest of the project.
zohnannor
changed the title
metrics dependencies versionsmetrics dependency from 0.21.0 to 0.22.0
|
I've removed the changes regarding unnecessary allocations elimination and will provide a new PR after this one is merged. I decided to leave the documentation, markdown links, examples changes along with the version bump in this PR. Should I move them also? |
Originally, I expressed a preference not to include `README.md` in the crate documentation. However, I have since reconsidered this decision as I couldn't recall the process of generating `README.md` from `README.tpl`. (I do remember using the `cargo-readme` crate for this purpose, though). #34 (review)
Updated:
metricsto 0.22metrics-exporter-prometheusto 0.13describe&collect. Collector now takes aprefixupon it's instantiation, builds the strings for metrics names and reuses them onward. No need to allocate new string each time.This is a breaking change due to removal of
Collector::prefixmethod. Though it can be brought back, but I don't think there is much use cases for that.Additionally:
lib.rswithinclude_str!("../README.md")so that it stays updated. But I've notices theREADME.tplfile, it seems you are generating theREADMEfrom thelib.rsdocs? If you'll accept my proposal (withinclude_str), thetplfile could be deleted.[]in the markdown links, not sure why they were there.Diff checkstep from thefmtjob, there is a--checkflag incargo-fmtthat does that?Supersedes #32, #33.
Summary by CodeRabbit
Refactor
Collectorstruct for enhanced metrics handling.Documentation
Chores