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

Initial metrics support #8646

Closed
wants to merge 1 commit into from
Closed

Initial metrics support #8646

wants to merge 1 commit into from

Conversation

emberian
Copy link
Member

No description provided.

@graydon
Copy link
Contributor

graydon commented Aug 23, 2013

I keep writing up then losing the comments I wanted to make here.

I think I'd prefer the following changes:

  • MetricGroup contains only a static string -- the prefix -- and can thus be declared statically with a macro that expands to include module_path!()
  • MetricGroup manages a hashtable in TLS privately.
  • The enum is, if not private to the module, at least something hidden from most casual users of it. IOW, that the different types of metric are exposed to clients through a set of named convenience-methods on MetricGroup such as MetricGroup::get_counter(name: &str, f: &fn(&Counter)->T)->T. The method populates the TLS hashtable with a counter named by prepending the group prefix to the provided name, if it does not yet exist, and raises a condition (or fails) if someone tries to use the same name for different metrics types. I think there is a small enough number of metrics types that get_counter, get_timer, get_gauge, get_histogram, get_meter, etc. will not be too much clutter. If there are sub-options or special details to provide when getting a metric, they can either be wired into the convenience API (eg. get_expdecay_histogram), passed as enum flags, or can use a more-general "raw" method such as you've provided in the basic version here.
  • An even shorter set of convenience APIs on MetricGroup that provide shortcuts to the default recording-action of contributing a sample to a metric, such as inc_counter(&str) or time_event(&str,fn()->T)->T or such.

Mostly I just want this interface to be as simple as possible so that people use it casually. Best usage I can picture is like:

mod foo {
    metric_group!(foometrics);

    fn bar() {
        foometrics.inc_counter("bar");
        do foometrics.time_event("bar.first_part") {
            // ...
        }
    }
}

Most of this can be done as followon commits (which I'm happy to help with), but I think MetricsGroup itself needs to be organized a little differently to get started.

@emberian
Copy link
Member Author

@graydon I'm going to close this to clear up the queue as I don't foresee getting a chance to work on it in the next week, but if you don't get to it (I understand you're on vacation) I'll pick it up as soon as I can.

@emberian emberian closed this Aug 28, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 7, 2022
…fate

Fix `as_deref_mut` false positives in `needless_option_as_deref`

Also moves it into `methods/`

Fixes rust-lang#7846
Fixes rust-lang#8047

changelog: [`needless_option_as_deref`]: No longer lints for `as_deref_mut` on Options that cannot be moved

supersedes rust-lang#8064
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 21, 2022
…rednet

Check for loops/closures in `local_used_after_expr`

Follow up to rust-lang#8646, catches when a local is used multiple times because it's in a loop or a closure

changelog: none
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 20, 2025
fixes rust-lang#14148

Another case of rust-lang#13077 and rust-lang#8646

changelog: [`needless_option_as_deref`]: fix FP in trait
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.

2 participants