MQE: add support for count_values
#10744
Merged
Conversation
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
lamida
reviewed
Feb 26, 2025
lamida
reviewed
Feb 27, 2025
| } | ||
|
|
||
| for _, p := range data.Floats { | ||
| if err := c.incrementCount(s.Labels, p.T, c.formatFloatValue(p.F), accumulator); err != nil { |
There was a problem hiding this comment.
Would caching c.formatFloatValue(p.F) going to be beneficial? I will imagine if we have many same values across the result, this will be formatted multiple times for that same values.
There was a problem hiding this comment.
The benchmarks show this implementation is already significantly faster than Prometheus' engine for the case where there are many series with the same value, and caching like this would make the worst case (many series with different values) slower, so I'm tempted to leave things as-is.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What this PR does
This PR adds support for
count_valuesin MQE.Benchmark results are mixed. MQE's implementation always consumes less memory than Prometheus' engine in our benchmarks, but in some cases runs slower, and in others runs faster. MQE is up to 36% faster for the common case where many series have only a handful of values, but up to 20% slower for the uncommon case where every sample has a different value.
The
count_values('value', h_X)cases exercise the scenario where every sample has a different value, resulting inX × 100output series. Thecount_values('value', h_X * 0)cases exercise the scenario where every sample has the same value, resulting in 1 output series.The difference in performance for the uncommon case is because Prometheus' engine uses a hash of the series labels to identify output series, which runs the risk of hash collisions, while MQE uses a string representation of the series labels to identify output series. In the case where there are many output series, this means MQE must allocate many strings, which causes the performance impact we see here.
Given
count_valuesis a relatively uncommonly used PromQL feature, the performance of the common case is significantly better than Prometheus' engine, and MQE's implementation avoids possible hash collisions, I'm OK with this performance regression.Which issue(s) this PR fixes or relates to
#10067
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.