Remove NewPossibleNonCounterInfo and minimise creating empty annotations#13012
Merged
bboreham merged 4 commits intoprometheus:release-2.48from Oct 24, 2023
Merged
Conversation
… and avoid creating empty annotations as much as possible Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
LeviHarrison
approved these changes
Oct 22, 2023
Contributor
LeviHarrison
left a comment
There was a problem hiding this comment.
Did another benchmark and looks to save allocs from when this annotation was available, so LGTM.
name old allocs/op new allocs/op delta
RangeQuery/expr=rate(a_one[1m]),steps=1-10 171 ± 0% 154 ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_one[1m]),steps=100-10 969 ± 0% 160 ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_one[1m]),steps=1000-10 8.21k ± 0% 0.20k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_ten[1m]),steps=1-10 410 ± 0% 249 ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_ten[1m]),steps=100-10 8.38k ± 0% 0.30k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_ten[1m]),steps=1000-10 80.8k ± 0% 0.7k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_hundred[1m]),steps=1-10 2.75k ± 0% 1.15k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_hundred[1m]),steps=100-10 82.5k ± 0% 1.7k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_hundred[1m]),steps=1000-10 806k ± 0% 5k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_one[1m]),steps=10000-10 80.7k ± 0% 0.6k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_ten[1m]),steps=10000-10 805k ± 0% 5k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_hundred[1m]),steps=10000-10 8.05M ± 0% 0.04M ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_one[1d]),steps=1-10 555 ± 0% 538 ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_one[1d]),steps=100-10 1.35k ± 0% 0.54k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_one[1d]),steps=1000-10 8.59k ± 0% 0.58k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_ten[1d]),steps=1-10 4.09k ± 0% 3.93k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_ten[1d]),steps=100-10 12.1k ± 0% 4.0k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_ten[1d]),steps=1000-10 84.4k ± 0% 4.3k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_hundred[1d]),steps=1-10 39.4k ± 0% 37.8k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_hundred[1d]),steps=100-10 119k ± 0% 38k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_hundred[1d]),steps=1000-10 843k ± 0% 42k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_one[1m])_+_rate(b_one[1m]),steps=1-10 320 ± 0% 285 ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_one[1m])_+_rate(b_one[1m]),steps=100-10 2.11k ± 0% 0.49k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_one[1m])_+_rate(b_one[1m]),steps=1000-10 18.4k ± 0% 2.4k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_ten[1m])_+_rate(b_ten[1m]),steps=1-10 857 ± 0% 534 ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_ten[1m])_+_rate(b_ten[1m]),steps=100-10 17.0k ± 0% 0.8k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_ten[1m])_+_rate(b_ten[1m]),steps=1000-10 164k ± 0% 3k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_hundred[1m])_+_rate(b_hundred[1m]),steps=1-10 6.11k ± 0% 2.90k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_hundred[1m])_+_rate(b_hundred[1m]),steps=100-10 166k ± 0% 4k ± 0% ~ (p=1.000 n=1+1)
RangeQuery/expr=rate(a_hundred[1m])_+_rate(b_hundred[1m]),steps=1000-10 1.62M ± 0% 0.01M ± 0% ~ (p=1.000 n=1+1)
bboreham
approved these changes
Oct 23, 2023
Member
bboreham
left a comment
There was a problem hiding this comment.
Should also modify the changelog entry for annotations.
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
juliusv
reviewed
Oct 23, 2023
CHANGELOG.md
Outdated
| * [ENHANCEMENT] Promtool: Add support for specifying series matchers in the TSDB analyze command. #12842 | ||
| * [ENHANCEMENT] PromQL: Prevent Prometheus from overallocating memory on subquery with large amount of steps. #12734 | ||
| * [ENHANCEMENT] PromQL: Add warning when monotonicity is forced in the input to histogram_quantile. #12931 | ||
| * [ENHANCEMENT] PromQL: Reduce inefficiency introduced by warnings/annotations, and temporarily remove NewPossibleNonCounterInfo. #13012 |
Member
There was a problem hiding this comment.
Everything else looks great, but not sure if this should go into the rc.0 annotations given that rc.0 has already been cut? Usually it would be the person cutting the next RC (rc.1) who would then add the new changelog lines, or did we agree to change that procedure now?
Member
There was a problem hiding this comment.
Sorry that's my sloppy phrasing.
We should leave the addition noted in rc0 and add a line noting the removal in rc1 in this changelog.
When it gets merged to main we should have either no line or a reference that some foundations were added.
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
Contributor
Author
|
With the latest commit, here's the benchmark with the same before/after branches as above (all overhead gone now): |
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
39ecacb to
430a6a0
Compare
Contributor
Author
zenador
added a commit
to grafana/mimir
that referenced
this pull request
Oct 24, 2023
3 tasks
zenador
added a commit
to grafana/mimir
that referenced
this pull request
Oct 24, 2023
beorn7
approved these changes
Oct 24, 2023
Member
beorn7
left a comment
There was a problem hiding this comment.
WRT the Changelog entry: In the final release notes, we shouldn't have any deltas relative to the RCs. So the final v2.48.0 will only feature whatever annotations we have then. In the changelog for v2.48.0, we don't have to document that the non-counter-info was added in rc.0 and removed again in rc.1.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow up to #12959 as a quick fix to temporarily remove
NewPossibleNonCounterInfoand avoid creating empty annotations unnecessarily.Followed up with #13022 to put
NewPossibleNonCounterInfoback without running it every step, and apply a similar treatment for other warnings which don't look at the data likeNewInvalidQuantileWarning.According to benchmarks, this should restore most of the efficiency:
Before is run on a branch on main that reverts the commits that added and modified annotations, and after is run on a branch on main with this commit applied.