-
Notifications
You must be signed in to change notification settings - Fork 791
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
Optimised /api/v1/series for blocks storage #2976
Optimised /api/v1/series for blocks storage #2976
Conversation
90cc4fb
to
0ee87df
Compare
pkg/ingester/ingester_v2.go
Outdated
@@ -695,37 +692,24 @@ func (i *Ingester) v2MetricsForLabelMatchers(ctx context.Context, req *client.Me | |||
} | |||
|
|||
seriesSet := q.Select(false, nil, matchers...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sets with series are not sorted, so merging them later doesn't work properly. This can be shown by using modified request in the benchmark/test:
req := &client.MetricsForLabelMatchersRequest{
StartTimestampMs: now,
EndTimestampMs: now,
MatchersSet: []*client.LabelMatchers{
{Matchers: []*client.LabelMatcher{
{Type: client.REGEX_MATCH, Name: model.MetricNameLabel, Value: "test.*"},
}},
{Matchers: []*client.LabelMatcher{
{Type: client.REGEX_MATCH, Name: model.MetricNameLabel, Value: "test.*0"}, // ending with 0
}},
},
}
This should still return all numSeries
series, but returns more of them.
Can you rerun benchmarks with sorting, and see whether it is still an improvement or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, thanks for spotting it! I copied the logic from Prometheus:
https://github.com/prometheus/prometheus/blob/b521612042ec87103088b45b7b5dfee3bb8dc732/web/api/v1/api.go#L604-L608
Either Prometheus /series API endpoint doesn't guarantee any deduplication, or there's a bug in Prometheus too. I will open an issue there.
Back to us, I modified the logic to use .Select(true, ...)
(sorted) for now and re-run the benchmark (updated PR description). There's still some benefit, less then before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened this issue in Prometheus: prometheus/prometheus#7801. Will follow up this PR accordingly, if any change is required.
Signed-off-by: Marco Pracucci <marco@pracucci.com>
…ber of series Signed-off-by: Marco Pracucci <marco@pracucci.com>
0ee87df
to
c0376e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Still looks like a nice improvement to me.
What this PR does:
While working on #2794, I've compared our implementation to fetch series with Prometheus one and I've noticed Prometheus is easier. I've done the refactoring and written and benchmark and turned out it performs better too.
Which issue(s) this PR fixes:
N/A
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]