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

feat(metrics): support Gauge<u32, AtomicU32> type #191

Merged
merged 3 commits into from Mar 7, 2024

Conversation

koushiro
Copy link
Contributor

@koushiro koushiro commented Mar 6, 2024

support Gauge<u32, AtomicU32> metric

Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@koushiro
Copy link
Contributor Author

koushiro commented Mar 6, 2024

@mxinden PTAL

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Cool. Thanks. Looks good to me.

Can you bump the crate version in the root's Cargo.toml to v0.22.2 and add a CHANGELOG.md entry? Once this pull request merged, I can then cut a new release right away.

@koushiro koushiro requested a review from mxinden March 6, 2024 18:42
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@mxinden
Copy link
Member

mxinden commented Mar 6, 2024

The following test:

    #[test]
    fn encode_gauge() {
        let mut registry = Registry::default();
        let gauge = Gauge::<u32, AtomicU32>::default();
        registry.register("my_gauge", "My gauge", gauge);

        let mut encoded = String::new();

        encode(&mut encoded, &registry).unwrap();

        parse_with_python_client(encoded);
    }

Would fail with:

   --> src/encoding/text.rs:635:51
    |
635 |         registry.register("my_gauge", "My gauge", gauge);
    |                  --------                         ^^^^^ the trait `EncodeGaugeValue` is not implemented for `u32`
    |                  |
    |                  required by a bound introduced by this call

Mind adding an implementation of EncodeGaugeValue for u32? You can cast into an i64 and then use the existing implementation for i64.

Please also extend the encode_gauge test in text.rs to register both a default Gauge and a Gauge<u32, AtomicU32 to make sure we don't break this in the future.

Sorry for the additional work. Thank you for the help.

Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@koushiro
Copy link
Contributor Author

koushiro commented Mar 7, 2024

Thanks for your review, I have changed encoding related stuff and added a simple test case to encode_gauge.
@mxinden PTAL again

@koushiro koushiro changed the title feat(metrics): impl Atomic<u32> for AtomicU32 feat(metrics): support Gauge<u32, AtomicU32> type Mar 7, 2024
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Well done!

@mxinden mxinden merged commit de27234 into prometheus:master Mar 7, 2024
10 checks passed
@mxinden
Copy link
Member

mxinden commented Mar 7, 2024

Published and tagged.

@koushiro koushiro deleted the impl-gague-atomic-u32 branch March 7, 2024 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants