Receive: add per request limits for remote write #5527
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -690,6 +690,108 @@ func TestReceiveQuorum(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| // TODO(dougalscamata): continue here | ||
douglascamata marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| func TestReceiveWriteRequestLimits(t *testing.T) { | ||
|
There was a problem hiding this comment. Maybe an e2e test could help in understanding some real-world client behavior eg Prometheus, avalanche. WDYT? There was a problem hiding this comment. @saswatamcode actually, what do you mean with Do you mean, for example, testing that a well configured Prometheus or Avalanche will retry on a 429? I don't think it's worth to add an e2e test only for this reason. It means we're testing Prometheus/Avalanche and not Thanos. I started working on the e2e tests and the further I progress on them the more I think they have no real value beyond what the handler test provides. The only additional value the e2e adds that I can see is to ensure that the CLI args are properly passed down to the request limiting code. Besides this, the request limiting code can be tested without having to add an e2e test. There was a problem hiding this comment. I meant if seeing how a hashring would behave, with such request limits and multiple tenants, would be useful? There was a problem hiding this comment. Less + simpler tests while having the same verifications level = better, so I think a unit tests will suffice here.
In some way yes, but also... are we sure those projects have those tests? 🙃 Anyway, let's investigate/create focus tests in separate stream of work |
||
| for _, tc := range []struct { | ||
| name string | ||
| status int | ||
| amountSeries int | ||
| amountSamples int | ||
| appendables []*fakeAppendable | ||
| }{ | ||
| { | ||
| name: "Request above limit of series", | ||
| status: http.StatusRequestEntityTooLarge, | ||
| amountSeries: 21, | ||
| appendables: []*fakeAppendable{ | ||
| { | ||
| appender: newFakeAppender(nil, nil, nil), | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| name: "Request under the limit of series", | ||
| status: http.StatusOK, | ||
| amountSeries: 20, | ||
| appendables: []*fakeAppendable{ | ||
| { | ||
| appender: newFakeAppender(nil, nil, nil), | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| name: "Request above limit of samples (series * samples)", | ||
| status: http.StatusRequestEntityTooLarge, | ||
| amountSeries: 30, | ||
| amountSamples: 15, | ||
| appendables: []*fakeAppendable{ | ||
| { | ||
| appender: newFakeAppender(nil, nil, nil), | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| name: "Request under the limit of samples (series * samples)", | ||
| status: http.StatusOK, | ||
| amountSeries: 10, | ||
| amountSamples: 2, | ||
| appendables: []*fakeAppendable{ | ||
| { | ||
| appender: newFakeAppender(nil, nil, nil), | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| name: "Request above body size limit", | ||
| status: http.StatusRequestEntityTooLarge, | ||
| amountSeries: 300, | ||
| amountSamples: 150, | ||
| appendables: []*fakeAppendable{ | ||
| { | ||
| appender: newFakeAppender(nil, nil, nil), | ||
| }, | ||
| }, | ||
| }, | ||
| } { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| if tc.amountSamples == 0 { | ||
| tc.amountSamples = 1 | ||
|
There was a problem hiding this comment. What's the reason for this mutation? There was a problem hiding this comment. This is to kind of have 1 as default value for the test scenarios that do not explicitly set |
||
| } | ||
| handlers, _ := newTestHandlerHashring(tc.appendables, 1) | ||
| handler := handlers[0] | ||
| handler.options.WriteRequestSizeLimit = 1 * 1024 * 1024 | ||
| handler.options.WriteSamplesLimit = 200 | ||
| handler.options.WriteSeriesLimit = 20 | ||
| tenant := "test" | ||
|
|
||
| wreq := &prompb.WriteRequest{ | ||
| Timeseries: []prompb.TimeSeries{}, | ||
| } | ||
|
|
||
| for i := 0; i < tc.amountSeries; i += 1 { | ||
| label := labelpb.ZLabel{Name: "foo", Value: "bar"} | ||
| series := prompb.TimeSeries{ | ||
| Labels: []labelpb.ZLabel{label}, | ||
| } | ||
| for j := 0; j < tc.amountSamples; j += 1 { | ||
| sample := prompb.Sample{Value: float64(j), Timestamp: int64(j)} | ||
| series.Samples = append(series.Samples, sample) | ||
| } | ||
| wreq.Timeseries = append(wreq.Timeseries, series) | ||
| } | ||
|
|
||
| // Test that the correct status is returned. | ||
| rec, err := makeRequest(handler, tenant, wreq) | ||
| if err != nil { | ||
| t.Fatalf("handler %d: unexpectedly failed making HTTP request: %v", tc.status, err) | ||
| } | ||
| if rec.Code != tc.status { | ||
| t.Errorf("handler: got unexpected HTTP status code: expected %d, got %d; body: %s", tc.status, rec.Code, rec.Body.String()) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestReceiveWithConsistencyDelay(t *testing.T) { | ||
| appenderErrFn := func() error { return errors.New("failed to get appender") } | ||
| conflictErrFn := func() error { return storage.ErrOutOfBounds } | ||
|
|
||
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.
Do we want to support limits on the tenant level?
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.
Yup, this is something that I plan to add as a follow up. I am also talking with @saswatamcode, because his active series work will also need different per-tenant limits. We will propose that all the limits should be configured using a yaml file to make it simpler.
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.
So I am curious if we are going the per-tenant limit way, do we still want to keep those flags in this PR? Is it working as the default value if no limit is explicitly configured in the config file?
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.
Good question, @yeya24. I would prefer to have everything in the same place, the file. But defining the default value quickly through CLI args seem convenient too. I have no strong opinion on this.
We can maybe support CLI and file for default limit and document that one of them has priority over the other.