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

Abstract key lookup into a trait for Family::get_or_create #157

Open
cheshirskycode opened this issue Aug 11, 2023 · 4 comments · May be fixed by #158
Open

Abstract key lookup into a trait for Family::get_or_create #157

cheshirskycode opened this issue Aug 11, 2023 · 4 comments · May be fixed by #158

Comments

@cheshirskycode
Copy link
Contributor

Family::get_or_create forces to clone labels data for the lookup of a counter. Example:

#[derive(Clone, Eq, Hash, PartialEq, EncodeLabelSet)]
struct RequestLabels {
    host: String,
    api: String,
}

fn test() {
    let family = Family::<RequestLabels, Counter>::default();

    fn call(host: &str, api: &str, family: &Family<RequestLabels, Counter>) {
        let labels = RequestLabels {
            host: host.to_string(),
            api: api.to_string(),
        };

        let counter = family.get_or_create(&labels);
        counter.inc();
    }
}

I must create RequestLabels with cloning host and api values instead of getting counter by data references. The Family struct uses the std::collections::HashMap inside, which doesn't allow any other way to look up.

It would be great to have the ability to write something like this:

#[derive(Clone, Eq, Hash, PartialEq, EncodeLabelSet)]
struct RequestLabels {
    host: String,
    api: String,
}

struct RequestLabelsQ<'a> {
    host: &'a str,
    api: &'a str,
}

impl Equivalent<RequestLabels> for RequestLabelsQ<'_> {
    fn equivalent(&self, key: &RequestLabels) -> bool {
        self.host == key.host && self.api == key.api
    }
}

fn test() {
    let family = Family::<RequestLabels, Counter>::default();

    fn call(host: &str, api: &str, family: &Family<RequestLabels, Counter>) {
        let labels = RequestLabelsQ {
            host, api,
        };

        let counter = family.get_or_create(&labels);
        counter.inc();
    }
}

This will require using hashbrown instead of std::collections::HashMap (but actually std map is wrapper on hashbrown).
Equivalent
HashMap::get
Also, it will affect the definition of Family::get_or_create but the behavior should be the same.

@cheshirskycode
Copy link
Contributor Author

@mxinden, could you take a look? I'd like to implement it If it sounds reasonable

@mxinden
Copy link
Member

mxinden commented Aug 16, 2023

Thank you for the code snippets. Generally I am not opposed to moving to hashbrown. That said, I am not yet fully sure I understand the rational.

Are you trying to optimize performance? If so, would you mind providing data on how much the above would safe you?

Are you trying to optimize ergonomics? If so, I don't think having to implement Equivalent is any better than calling to_string().

@cheshirskycode
Copy link
Contributor Author

cheshirskycode commented Aug 16, 2023

I have huge label sets (10+ items) and can't formalize them in static types, it's just Strings in general. I have &str slices during data processing and copying them to locate the counter doesn't look good.

I've added some bench in PR:
The best option is counter family with custom type label set. It doesn't contain allocations and is very cheap for hashing.
counter family with custom type label set and direct lookup search by type with 3 string fields&Labels {method: "GET".to_string(), url_path: "/metrics".to_string(), status_code: "200".to_string()} - 61.247 ns
counter family with custom type label set and equivalent lookup - the same but without copying - 13.324 ns

counter family with Vec<(String, String)> label set
                        time:   [104.90 ns 106.36 ns 107.95 ns]
Found 7 outliers among 100 measurements (7.00%)
  4 (4.00%) high mild
  3 (3.00%) high severe

counter family with custom type label set
                        time:   [6.4575 ns 6.4720 ns 6.4883 ns]
Found 13 outliers among 100 measurements (13.00%)
  8 (8.00%) high mild
  5 (5.00%) high severe

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]

@hdost
Copy link

hdost commented Oct 26, 2023

Perhaps using `Cowˋ would be useful?

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 a pull request may close this issue.

3 participants