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

Replace lazy static initializers with const fn #221

Open
brson opened this issue Jan 29, 2019 · 7 comments
Open

Replace lazy static initializers with const fn #221

brson opened this issue Jan 29, 2019 · 7 comments

Comments

@brson
Copy link

brson commented Jan 29, 2019

Should be possible after upgrading to 2018 re #217 (comment)

lazy_static! {
    static ref TIKV_REQUEST_DURATION_HISTOGRAM_VEC: HistogramVec = register_histogram_vec!(
        "tikv_request_duration_seconds",
        "Bucketed histogram of TiKV requests duration",
        &["type"]
    )
}
@brson
Copy link
Author

brson commented Feb 11, 2019

I peeked at the details of this task briefly and it does not look so simple; certainly not as simple as converting (in the above example) register_histogram_vec! to a const fn. That macro invokes plenty of dynamic behavior, like creating Boxes.

@Hoverbear @overvenus do you have an idea of the transformation that needs to be done here?

@Hoverbear
Copy link
Contributor

In this case @breeswish would be a good resource

@brson brson removed the help wanted label Feb 12, 2019
@breezewish
Copy link
Member

@brson I tried with const fn in 1.0 experiment and looks to be not a good choice currently, due to a lot of std functions necessary in building the metric are not marked as const fn.

@Hoverbear
Copy link
Contributor

This is problematic. :(

@kennytm
Copy link

kennytm commented Feb 14, 2019

A const fn is not going to support the "register" part of register_histogram_vec even after the entire of rust-lang/rust#57563 is implemented, since a const fn cannot mutate something the function doesn't own (the DEFAULT_REGISTRY).

Not using the macro means user would write instead

static TIKV_REQUEST_DURATION_HISTOGRAM_VEC: HistogramVec = HistogramVec::new(
    "tikv_request_duration_seconds",
    "Bucketed histogram of TiKV requests duration",
    &["type"],
);

static SOME_MORE_COLLECTORS: ... = ...;

fn main() -> Result<()> {
    register(Box::new(TIKV_REQUEST_DURATION_HISTOGRAM_VEC.clone())?;
    register(Box::new(SOME_MORE_COLLECTORS.clone())?;  // <-- need to call these manually :(
    ...
}

@breezewish
Copy link
Member

Thanks @kennytm ! I think we can wait rust-lang/rust#57563 and then mark as more function as possible to be const fn, but might not be very useful in all cases (as the case kenny illustrated). We can relax the goal of this issue.

Additionally, I would like to mention that the process of registering and creating a metric can be simplified, for example, in 1.0-pre, we can just write:

let counter = CounterBuilder::new("name", "help")
    .build_vec(["a", "b"])
    .unwrap();

@teohhanhui
Copy link

A cleaner alternative is https://docs.rs/once_cell/1.4.0/once_cell/#lazy-initialized-global-data

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

No branches or pull requests

5 participants