-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
lru cache logql.ParseLabels #3092
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3092 +/- ##
==========================================
+ Coverage 62.78% 62.85% +0.06%
==========================================
Files 186 186
Lines 15949 15960 +11
==========================================
+ Hits 10014 10031 +17
+ Misses 5009 5000 -9
- Partials 926 929 +3
|
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.
Thanks for the PR!
I have left some feedback.
pkg/distributor/distributor.go
Outdated
label, ok := d.labelCache.Get(stream.Labels) | ||
if !ok { | ||
ls, err := logql.ParseLabels(stream.Labels) | ||
if err != nil { | ||
validationErr = httpgrpc.Errorf(http.StatusBadRequest, "error parsing labels: %v", err) | ||
continue | ||
} | ||
// ensure labels are correctly sorted. | ||
// todo(ctovena) we should lru cache this | ||
stream.Labels = ls.String() | ||
if err := d.validator.ValidateLabels(userID, ls, stream); err != nil { | ||
validationErr = err | ||
continue | ||
} | ||
d.labelCache.Add(stream.Labels, stream.Labels) | ||
} else { | ||
stream.Labels = label.(string) |
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 think it would be good to move this to a separate function and write a benchmark for it.
pkg/distributor/distributor.go
Outdated
validationErr = err | ||
continue | ||
} | ||
d.labelCache.Add(stream.Labels, stream.Labels) |
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 doesn't look right to me. The key should be raw labels and the value should be the sorted labels, no?
pkg/distributor/distributor.go
Outdated
continue | ||
} | ||
// ensure labels are correctly sorted. | ||
// todo(ctovena) we should lru cache this |
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.
We should remove this todo now that it is being taken care of in this PR.
thanks , done goos: darwin cache result no cache result |
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.
A few small things, but this looks great. Really excited to see the lru cache here!
pkg/distributor/distributor_test.go
Outdated
d := prepare(&testing.T{}, limits, nil, func(addr string) (ring_client.PoolClient, error) { return ingester, nil }) | ||
defer services.StopAndAwaitTerminated(context.Background(), d) //nolint:errcheck | ||
for n := 0; n < b.N; n++ { | ||
request := makeWriteRequest(10, 10) |
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.
nit: these can be created/reused outside the loop to interfere less with benchmarking (initialization doesn't need to be part of the benchmarking).
pkg/distributor/distributor.go
Outdated
@@ -51,6 +52,8 @@ var ( | |||
Name: "distributor_lines_received_total", | |||
Help: "The total number of lines received per tenant", | |||
}, []string{"tenant"}) | |||
|
|||
maxLabelCacheSize = 10000 |
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.
maxLabelCacheSize = 10000 | |
maxLabelCacheSize = 100000 |
I think 100k might be a better starting point for this. Unlike ingesters, distributors handle all incoming traffic and are therefore exposed to many more unique label sets.
pkg/distributor/distributor.go
Outdated
return "", httpgrpc.Errorf(http.StatusBadRequest, "error parsing labels: %v", err) | ||
} | ||
// ensure labels are correctly sorted. | ||
lsVal := ls.String() |
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.
nit: lsVal
can be created after the validation occurs as a tiny optimization.
thanks , done |
…locks (grafana#3092) Signed-off-by: Peter Štibraný <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3092 +/- ##
==========================================
+ Coverage 62.78% 62.86% +0.08%
==========================================
Files 186 186
Lines 15949 15960 +11
==========================================
+ Hits 10014 10034 +20
+ Misses 5009 4997 -12
- Partials 926 929 +3
|
lru cache logql.ParseLabels