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(family): add ability to lookup metric by query type #158

Closed
wants to merge 1 commit into from

Conversation

cheshirskycode
Copy link
Contributor

Added ability to lookup metric by query type in Family::get_or_create

Example:

#[derive(Clone, Eq, Hash, PartialEq, EncodeLabelSet)]
struct Labels {
    method: String,
    url_path: String,
    status_code: String,
}

let family = Family::<Labels, Counter>::default();

// Will create or get the metric with label `method="GET",url_path="/metrics",status_code="200"`
family.get_or_create(&Labels {
    method: "GET".to_string(),
    url_path: "/metrics".to_string(),
    status_code: "200".to_string(),
}).inc();

// Will return a reference to the metric without unnecessary cloning and allocation.
family.get_or_create(&LabelsQ {
    method: "GET",
    url_path: "/metrics",
    status_code: "200",
}).inc();

#[derive(Debug, Eq, Hash, PartialEq)]
struct LabelsQ<'a> {
    method: &'a str,
    url_path: &'a str,
    status_code: &'a str,
}

impl CreateFromEquivalent<Labels> for LabelsQ<'_> {
    fn create(&self) -> Labels {
        Labels {
            method: self.method.to_string(),
            url_path: self.url_path.to_string(),
            status_code: self.status_code.to_string(),
        }
    }
}

impl Equivalent<Labels> for LabelsQ<'_> {
    fn equivalent(&self, key: &Labels) -> bool {
        self.method == key.method &&
            self.url_path == key.url_path &&
            self.status_code == key.status_code
    }
}

Performance difference:

  • 61.247 ns - lookup by &Labels {method: "GET".to_string(), url_path: "/metrics".to_string(), status_code: "200".to_string()}
  • 13.324 ns - lookup by &LabelsQ {method: "GET", url_path: "/metrics", status_code: "200"}
counter family with custom type label set and direct lookup
                        time:   [61.076 ns 61.247 ns 61.422 ns]
Found 9 outliers among 100 measurements (9.00%)
  8 (8.00%) high mild
  1 (1.00%) high severe

counter family with custom type label set and equivalent lookup
                        time:   [13.291 ns 13.324 ns 13.358 ns]
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

Fix: #157

…get_or_create`

Signed-off-by: Michael Zhalevich <cheshirsky.code@gmail.com>
@mxinden
Copy link
Member

mxinden commented Sep 1, 2023

Thank you for the work here.

I am hesitant to move forward.

  • This pull request increases the maintenance surface of this crate.
  • This patch ties us to the public interface of hashbrown, i.e. a breaking release in hashbrown requires a breaking release in prometheus-client.
  • While the benchmark shows a significant impact on the micro level, I wonder how much impact this has on the macro level. In other words, do you see this impacting your application?

I wonder whether keeping a Labels struct around across get_or_create calls would be a valid option for you. Or whether wrapping your strings in a Arc on the higher level.

@cheshirskycode
Copy link
Contributor Author

Thank you for the answer. Concerns are reasonable.

While the benchmark shows a significant impact on the micro level, I wonder how much impact this has on the macro level. In other words, do you see this impacting your application?
I don't see the critical impact of cloning a dozen strings on each processed message. But looking into code like this is enough to decide to do something.

self.family.get_or_create(&Labels {
    one: one.to_owned(),
    two: two.to_owned(),
    three: three.to_owned(),
    four: four.to_owned(),
    five: five.to_owned(),
}).inc();

All my alternative solutions lead to having Arc<RwLock<hashbrown::HashMap<RealLabels, ...>>> which is very similar to Family :)

Anyway, you provide a good API. Thank you. Let me know if you need some help.

@mxinden
Copy link
Member

mxinden commented Sep 7, 2023

All my alternative solutions lead to having Arc<RwLock<hashbrown::HashMap<RealLabels, ...>>> which is very similar to Family :)

As you likely know, you can implement your own metrics, e.g. in this particular case it sounds like you should provide your own custom Family type that implements EncodeMetric.

Thank you for the answer. Concerns are reasonable.

Appreciate the understanding!

Anyway, you provide a good API. Thank you. Let me know if you need some help.

Always! Thank you for offering your help on #162 (comment) @cheshirskycode!

@mxinden
Copy link
Member

mxinden commented Jul 17, 2024

Closing here given the concerns above. Please open a new pull request in case you want to continue the discussion.

@mxinden mxinden closed this Jul 17, 2024
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.

Abstract key lookup into a trait for Family::get_or_create
2 participants