Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

promql: avoid unnecessaryMetric.Get() calls infunctions.go#17676

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
vpranckaitis wants to merge2 commits intoprometheus:main
base:main
Choose a base branch
Loading
fromvpranckaitis:avoid_unnecesary_extraction_of_metric_name

Conversation

@vpranckaitis
Copy link
Contributor

Moved someMetric.Get() calls in PromQL functions to avoid unnecessary label extraction. Withstringlabels that is a non-trivial amount of work, as shown by CPU profiles. In many cases, this work was done to extract metric name, and was only used if annotations were emitted.

In the same go I also replacedlabels.MetricName withmodel.MetricNameLabel, since the former was deprecated.

image

Which issue(s) does the PR fix:

Does this PR introduce a user-facing change?

[PERF] PromQL: Avoid unnecessary label extraction in PromQL functions

Signed-off-by: Vilius Pranckaitis <vpranckaitis@gmail.com>
// In case of a counter reset, we leave resultSample at
// its current value, which is already ss[1].
casess[1].H!=nil&&ss[0].H!=nil:
metricName:=samples.Metric.Get(model.MetricNameLabel)
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I considered pushing this further down into the 3 branches branches below, though wasn't sure if this wouldn't be too excessive.

@bboreham
Copy link
Member

Please show benchmark results, as requested in the PR template.

Copy link
Member

@bborehambboreham left a comment

Choose a reason for hiding this comment

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

Thanks for this; I think it's a good idea.

I would rather defer evaluation as far as possible, e.g. intohistogramRate, and add a small helper function to shorten the code where used. Then don't create a new variablemetricName anywhere it would only be used once.

@vpranckaitis
Copy link
ContributorAuthor

Please show benchmark results, as requested in the PR template.

Did not write a separate benchmark, but used the exisitngBenchmarkRangeQuery() to show the improvement. To cut out the noise, I removed all the test cases except these two:

cases := []benchCase{    // Simple rate.    {    expr: "rate(a_X[1m])",    },    {    expr:  "rate(a_X[1m])",    steps: 10000,    },}

The benchmark results show 1–6% reduction in runtime, and practically no change in allocations.

goos: darwingoarch: arm64pkg: github.com/prometheus/prometheus/promqlcpu: Apple M1 Pro                                                   │   old.txt   │              new.txt               │                                                   │   sec/op    │   sec/op     vs base               │RangeQuery/expr=rate(a_one[1m]),steps=1-10           9.016µ ± 1%   8.819µ ± 1%  -2.19% (p=0.000 n=10)RangeQuery/expr=rate(a_one[1m]),steps=1000-10        84.93µ ± 0%   79.18µ ± 1%  -6.77% (p=0.000 n=10)RangeQuery/expr=rate(a_hundred[1m]),steps=1-10       216.7µ ± 0%   214.4µ ± 1%  -1.06% (p=0.002 n=10)RangeQuery/expr=rate(a_hundred[1m]),steps=1000-10    8.172m ± 1%   7.763m ± 2%  -5.01% (p=0.000 n=10)RangeQuery/expr=rate(a_one[1m]),steps=10000-10       824.9µ ± 0%   779.1µ ± 1%  -5.55% (p=0.000 n=10)RangeQuery/expr=rate(a_hundred[1m]),steps=10000-10   85.89m ± 1%   80.89m ± 1%  -5.82% (p=0.000 n=10)geomean                                              676.7µ        646.8µ       -4.42%                                                   │   old.txt    │               new.txt               │                                                   │     B/op     │     B/op      vs base               │RangeQuery/expr=rate(a_one[1m]),steps=1-10           7.883Ki ± 0%   7.883Ki ± 0%       ~ (p=0.752 n=10)RangeQuery/expr=rate(a_one[1m]),steps=1000-10        11.58Ki ± 0%   11.58Ki ± 0%       ~ (p=0.631 n=10)RangeQuery/expr=rate(a_hundred[1m]),steps=1-10       68.97Ki ± 0%   68.97Ki ± 0%       ~ (p=0.670 n=10)RangeQuery/expr=rate(a_hundred[1m]),steps=1000-10    309.4Ki ± 0%   309.5Ki ± 0%       ~ (p=0.781 n=10)RangeQuery/expr=rate(a_one[1m]),steps=10000-10       52.84Ki ± 1%   52.83Ki ± 1%       ~ (p=0.469 n=10)RangeQuery/expr=rate(a_hundred[1m]),steps=10000-10   2.585Mi ± 0%   2.581Mi ± 1%  -0.14% (p=0.009 n=10)geomean                                              80.52Ki        80.50Ki       -0.03%                                                   │   old.txt   │               new.txt                │                                                   │  allocs/op  │  allocs/op   vs base                 │RangeQuery/expr=rate(a_one[1m]),steps=1-10            136.0 ± 0%    136.0 ± 0%       ~ (p=1.000 n=10) ¹RangeQuery/expr=rate(a_one[1m]),steps=1000-10         165.0 ± 0%    165.0 ± 0%       ~ (p=1.000 n=10) ¹RangeQuery/expr=rate(a_hundred[1m]),steps=1-10       1.136k ± 0%   1.136k ± 0%       ~ (p=1.000 n=10) ¹RangeQuery/expr=rate(a_hundred[1m]),steps=1000-10    3.577k ± 0%   3.577k ± 0%       ~ (p=0.265 n=10)RangeQuery/expr=rate(a_one[1m]),steps=10000-10        457.0 ± 0%    457.0 ± 0%       ~ (p=1.000 n=10) ¹RangeQuery/expr=rate(a_hundred[1m]),steps=10000-10   27.51k ± 0%   27.51k ± 0%       ~ (p=0.524 n=10)geomean                                              1.023k        1.023k       -0.00%¹ all samples are equal

Signed-off-by: Vilius Pranckaitis <vpranckaitis@gmail.com>
@vpranckaitis
Copy link
ContributorAuthor

Thanks for this; I think it's a good idea.

I would rather defer evaluation as far as possible, e.g. intohistogramRate, and add a small helper function to shorten the code where used. Then don't create a new variablemetricName anywhere it would only be used once.

@bboreham I've refactored the code to usegetMetricName() helper function. I considered to name itgetName() so that in majority of places where it's used it would look likegetName(samples.Metric). The latter is shorter, but still should convey that the metric name is being retrieved. However, I decided to go with a more conservative option first.

Also, pushed the metric name extraction further down intohistogramRate() function.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@roidelapluieroidelapluieAwaiting requested review from roidelapluieroidelapluie is a code owner

@bborehambborehamAwaiting requested review from bboreham

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vpranckaitis@bboreham
[8]ページ先頭
©2009-2025 Movatter.jp