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

Allow family to use constructors that do not coerce to function pointers #21

Merged
merged 6 commits into from
Nov 9, 2021

Conversation

divagant-martian
Copy link
Contributor

@divagant-martian divagant-martian commented Oct 22, 2021

Right now a Family can only use as constructors types that coerce to function pointers, but it can be useful to provide more general constructors.

An example is in libp2p/rust-libp2p#2235 where creating an histogram for the score has no clear bucket bounds since those can be affected by the application score, or simply put.. someone wanting to get score metrics might be interested in some custom bounds/buckets. In order to make this parameterizable we would do something like:

let my_metric = Family::new_with_constructor(move || Histogram::new(exponential_buckets(custom_start, 1.0, 10)));
This does not coerce to a function pointer and seems like a reasonable thing to expect to be able to do.

This makes it possible in a way slightly more convoluted but still convenient (I think) without creating a breaking change (See tests for an example).

If you think there is a better way to do it let me know

@divagant-martian
Copy link
Contributor Author

I did a first attempt to simplify this (see a732bbc) and at first glace it seems to work. However this makes it extremely hard (not sure if even possible) to type the resulting Family.

That way it would be possible to have

let captured_var = vec![3.4, 5.5];
let family_with_constructor = Family::<(), Histogram, _>::new_with_constructor(|| {
    Histogram::new(exponential_buckets(captured_var[0], captured_var[1], 10))
});

but the common case includes wanting to store the resulting Family metric in a struct

struct MyMetrics {
    family_with_constructor: Family<(), Histogram, WhatHere???>,
}

so I still think it's best the way the current pr proposes.

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.

Sorry for the delay here @divagant-martian!

I think this is a valid approach and not too intrusive.

constructor: C,
}

pub trait MetricConstructor<M> {
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind adding some documentation pointing to Family::new_with_constructor and a doc example to this type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added docs and an example to the trait, and also to the Fn() -> M implementor for completeness. Thanks for the suggestion

src/metrics/family.rs Outdated Show resolved Hide resolved
src/metrics/family.rs Outdated Show resolved Hide resolved
src/metrics/family.rs Outdated Show resolved Hide resolved
src/metrics/family.rs Outdated Show resolved Hide resolved
/// let metric = Family::<(), Histogram, _>::new_with_constructor(|| {
/// Histogram::new(custom_buckets.clone().into_iter())
/// });
/// # metric.get_or_create(&());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commenting here because I think you'll wonder about this line. This helps test that the constructor above is a Fn() -> M and not a FnOnce and thus, that this is a valid example for the implementor

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.

Thank you for the elaborate work!

@mxinden mxinden merged commit 18f072d into prometheus:master Nov 9, 2021
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