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
Cleanup tenant data in ingester #3782
Cleanup tenant data in ingester #3782
Conversation
- Remove metadata - Metadata metrics - Validation metrics Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
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.
You did great! Good job 👏
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
continue | ||
} | ||
|
||
lbls := labels.NewBuilder(nil) |
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.
what about re-using the same *labels.Builder
here and calling lbls.Reset(nil)
at the end of the loop, like this:
lbls := labels.NewBuilder(nil)
nextMetric:
for m := range ch {
// ...
result = append(result, lbls.Labels())
lbls.Reset(nil)
}
This improves the overall runtime by GetLabels
by ~24% for small label sets and ~50% for large label sets because it avoids allocating in a loop.
Compare these two benchmarks:
A
func BenchmarkGetLabels_SmallSet(b *testing.B) {
m := prometheus.NewCounterVec(prometheus.CounterOpts{
Name: "test",
ConstLabels: map[string]string{
"cluster": "abc",
},
}, []string{"reason", "user"})
m.WithLabelValues("bad", "user1").Inc()
m.WithLabelValues("worse", "user1").Inc()
m.WithLabelValues("worst", "user1").Inc()
m.WithLabelValues("bad", "user2").Inc()
m.WithLabelValues("worst", "user2").Inc()
m.WithLabelValues("worst", "user3").Inc()
for i := 0; i < b.N; i++ {
GetLabels(m, map[string]string{"user": "user1", "reason": "worse"})
}
}
BEFORE
pkg: github.com/cortexproject/cortex/pkg/util
BenchmarkGetLabels_SmallSet
BenchmarkGetLabels_SmallSet-8 269169 3818 ns/op
PASS
AFTER
pkg: github.com/cortexproject/cortex/pkg/util
BenchmarkGetLabels_SmallSet
BenchmarkGetLabels_SmallSet-8 445689 2639 ns/op
PASS
B
func BenchmarkGetLabels_LargeSet(b *testing.B) {
m := prometheus.NewCounterVec(prometheus.CounterOpts{
Name: "test",
ConstLabels: map[string]string{
"cluster": "abc",
},
}, []string{"reason", "user"})
for i := 1; i <= 1000; i++ {
m.WithLabelValues("bad", fmt.Sprintf("user%d", i)).Inc()
m.WithLabelValues("worse", fmt.Sprintf("user%d", i)).Inc()
m.WithLabelValues("worst", fmt.Sprintf("user%d", i)).Inc()
if i % 2 == 0 {
m.WithLabelValues("bad", fmt.Sprintf("user%d", i)).Inc()
m.WithLabelValues("worst", fmt.Sprintf("user%d", i)).Inc()
} else {
m.WithLabelValues("worst", fmt.Sprintf("user%d", i)).Inc()
}
}
for i := 0; i < b.N; i++ {
GetLabels(m, map[string]string{"user": "user1", "reason": "worse"})
}
}
BEFORE
pkg: github.com/cortexproject/cortex/pkg/util
BenchmarkGetLabels_LargeSet
BenchmarkGetLabels_LargeSet-8 710 1698894 ns/op
PASS
AFTER
pkg: github.com/cortexproject/cortex/pkg/util
BenchmarkGetLabels_LargeSet
BenchmarkGetLabels_LargeSet-8 1431 876509 ns/op
PASS
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.
This is not performance-critical code, since it's called pretty rarely. But low-hanging optimization like this looks good. Would you like to send PR? (I think we need to call Reset
at the beginning of the loop, because not all iterations get to the end. Interestingly unit tests don't catch this problem.)
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.
I'll post a PR for this today. Thanks!
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.
posted #3863 which just contains the 2 line change I mentioned, plus the benchmarks to show the diff.
What this PR does: This PR does additional cleanup of in-memory data when tenant's TSDB is closed in ingester. Specifically:
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]