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

distributor: more efficient labels error messages #6785

Merged
merged 4 commits into from
Dec 1, 2023
Merged

Conversation

bboreham
Copy link
Contributor

What this PR does

This is split into three commits:

  • Changing getMetricAndEllipsis - instead of converting to Labels then to a map then to a string,
    we use nearby function formatLabelSet which avoids the first two conversions.
    Some tests need to be fixed up, because prometheus/common is sorting labels incorrectly.
  • Re-code formatLabelSet to remove intermediate string slice and two uses of Sprintf.
  • Tweak one of the benchmarks, to use a more reasonable test case. 2KB is so long it distorts benchmark timings.
goos: darwin
goarch: arm64
pkg: github.com/grafana/mimir/pkg/distributor
                                                            │    before    │               after                │
                                                            │    sec/op    │   sec/op     vs base               │
Distributor_Push/too_many_labels_limit_reached-8              11.067m ± 2%   3.450m ± 0%  -68.83% (p=0.002 n=6)
Distributor_Push/max_label_name_length_limit_reached-8         3.096m ± 1%   1.937m ± 1%  -37.42% (p=0.002 n=6)
Distributor_Push/max_label_value_length_limit_reached-8        4.244m ± 0%   3.051m ± 2%  -28.11% (p=0.002 n=6)
Distributor_Push/timestamp_too_new-8                           623.5µ ± 1%   623.7µ ± 2%        ~ (p=0.589 n=6)
Distributor_Push/all_samples_go_to_metric_relabel_configs-8    1.388m ± 2%   1.376m ± 1%        ~ (p=0.394 n=6)
Distributor_Push/all_samples_successfully_pushed-8             652.9µ ± 1%   644.7µ ± 1%   -1.26% (p=0.002 n=6)
Distributor_Push/ingestion_rate_limit_reached-8                302.7µ ± 1%   297.4µ ± 2%   -1.75% (p=0.041 n=6)
geomean                                                        1.583m        1.189m       -24.89%

                                                            │    before    │                after                 │
                                                            │     B/op     │     B/op       vs base               │
Distributor_Push/too_many_labels_limit_reached-8              7.723Mi ± 0%   1.177Mi ±  0%  -84.77% (p=0.002 n=6)
Distributor_Push/max_label_name_length_limit_reached-8        2.436Mi ± 0%   1.117Mi ±  0%  -54.13% (p=0.002 n=6)
Distributor_Push/max_label_value_length_limit_reached-8       2.697Mi ± 0%   1.178Mi ±  0%  -56.31% (p=0.002 n=6)
Distributor_Push/timestamp_too_new-8                          348.9Ki ± 0%   349.2Ki ±  0%   +0.08% (p=0.009 n=6)
Distributor_Push/all_samples_go_to_metric_relabel_configs-8   233.7Ki ± 0%   233.4Ki ±  1%        ~ (p=0.394 n=6)
Distributor_Push/all_samples_successfully_pushed-8            152.0Ki ± 0%   152.2Ki ±  0%        ~ (p=0.240 n=6)
Distributor_Push/ingestion_rate_limit_reached-8               2.140Ki ± 1%   2.126Ki ± 11%        ~ (p=0.833 n=6)
geomean                                                       392.8Ki        238.5Ki        -39.30%

                                                            │    before     │               after                │
                                                            │   allocs/op   │  allocs/op   vs base               │
Distributor_Push/too_many_labels_limit_reached-8              108.181k ± 0%   6.046k ± 0%  -94.41% (p=0.002 n=6)
Distributor_Push/max_label_name_length_limit_reached-8         43.052k ± 0%   6.047k ± 0%  -85.95% (p=0.002 n=6)
Distributor_Push/max_label_value_length_limit_reached-8        44.053k ± 0%   6.047k ± 0%  -86.27% (p=0.002 n=6)
Distributor_Push/timestamp_too_new-8                            5.043k ± 0%   5.043k ± 0%        ~ (p=1.000 n=6)
Distributor_Push/all_samples_go_to_metric_relabel_configs-8     5.057k ± 0%   5.057k ± 0%        ~ (p=1.000 n=6)
Distributor_Push/all_samples_successfully_pushed-8               54.00 ± 2%    54.00 ± 2%        ~ (p=1.000 n=6)
Distributor_Push/ingestion_rate_limit_reached-8                  34.00 ± 0%    34.00 ± 3%        ~ (p=1.000 n=6)
geomean                                                         3.706k        1.396k       -62.32%

Checklist

  • Tests updated.
  • NA Documentation added.
  • CHANGELOG.md updated

2KB is so long it distorts benchmark timings.
Instead of converting to Labels then to a map then to a string,
we use `formatLabelSet` which avoids the first two conversions.

Some tests need to be fixed up, because `prometheus/common`
is sorting labels incorrectly.
Remove intermediate string slice and two uses of Sprintf.
@bboreham bboreham requested a review from a team as a code owner November 30, 2023 16:17
Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

Nicely done!

Copy link
Member

@jhalterman jhalterman left a comment

Choose a reason for hiding this comment

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

LGTM

Somewhat tangential, but I wonder if formatLabelSet should be moved to the mimirpb package since it seems generally useful for []LabelAdapter.

@bboreham
Copy link
Contributor Author

Somewhat tangential, but I wonder if formatLabelSet should be moved to the mimirpb package since it seems generally useful for []LabelAdapter.

Definitely seems reasonable.

@bboreham bboreham enabled auto-merge (squash) December 1, 2023 11:09
@bboreham bboreham merged commit c4f6c37 into main Dec 1, 2023
28 checks passed
@bboreham bboreham deleted the faster-maxlabels branch December 1, 2023 11:21
@bboreham
Copy link
Contributor Author

bboreham commented Dec 4, 2023

Move to mimirpb and call it from more places in #6822

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

4 participants