Receive: add per request limits for remote write

[douglascamata]

Signed-off-by: Douglas Camata 159076+douglascamata@users.noreply.github.com

• I added CHANGELOG entry for this change.
• Change is not relevant to the end user.

Changes

Implement write requests size limits in Thanos Receive.

These limits are applied per request and can be configured with the following
command line arguments:

• --receive.write-request-limits.max-size-bytes: the maximum body size.
• --receive.write-request-limits.max-series: the maximum amount of series in a
  single remote write request.
• --receive.write-request-limits.max-samples: the maximum amount of samples in a
  single remote write request (summed from all series).

Any request above these limits will cause an 413 HTTP response (Entity Too Large)
and should not be retried without modifications. It's up to remote write clients to
split up the data and retry or completely drop it.

Another limit that was added to control concurrency on the remote write requests:

• --receive.write-request-limits.max-concurrency: the maximum amount remote write
  requests that will be concurrently processed while others wait.

By default all these limits are disabled.

The flags are also hidden until per-tenant configuration in a file is implemented. A mention that this feature is WIP and might change whenever the per-tenant configuration file lands is present in the Receive docs.

This is a part of the work planned in #5404.

Verification

• Added some cases to the Receive's handler tests and they pass.
• Running it locally.

Future work

• Add support for per-tenant limit configuration

[yeya24]

Do we want to support limits on the tenant level?

[douglascamata]

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.

[yeya24]

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?

[douglascamata]

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.

[saswatamcode]

Thanks for amazing work! 🌟

Some questions/suggestions!

[saswatamcode]

Is there a reason for having separate sizeLimit, seriesLimit and samplesLimit variables here instead of directly using h.options._Limit ? 🙂

[douglascamata]

I can make the condition inside the if much shorter and easier to read no matter your screen's resolution. 😄

[saswatamcode]

I wonder if we can isolate limiting logic into a dedicated function (apart from h.writeGate) and use that in receiveHTTP. Maybe to a function like requestUnderLimit which takes in request as argument and returns parsed request and bool.

As these are a lot of ifs which would happen every time a remote write request is received, I think benchmarking this logic would be nice! Separate requestUnderLimit function would allow this! 🙂

[douglascamata]

Let me see what I can do to isolate the logic. 👍

I have no worries regarding the ifs, they should add such a tiny amount of time to the handler's execution that the benchmark would show mostly noise. Most of the time spent in limiting logic will be due to the metrics increment and writing the error response back.

If others also worry about this I can try to work out on a benchmark and see whether it's useful.

[saswatamcode]

Maybe an e2e test could help in understanding some real-world client behavior eg Prometheus, avalanche. WDYT?

[douglascamata]

Will add 👍

[douglascamata]

@saswatamcode actually, what do you mean with understanding some real-world client behavior eg Prometheus, avalanche.?

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.

[saswatamcode]

I meant if seeing how a hashring would behave, with such request limits and multiple tenants, would be useful?
But yes let's see what maintainers have to say! Unit tests might be sufficient here. 🙂

[bwplotka]

Less + simpler tests while having the same verifications level = better, so I think a unit tests will suffice here.

It means we're testing Prometheus/Avalanche and not Thanos.

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

[fpetkovski]

Nice, this looks quite solid.

[fpetkovski]

Instead of extending the existing handler, is it possible to make the limiter a middleware that wraps the handler? This could be cleaner to maintain long term.

[douglascamata]

Unfortunately I don't think that's a good idea, because then the limiter middleware will have to do work that is done again in the main handler:

• Checking content length header is easy, but when it's not there we have to read the request body into a byte slice to check its size, which involves trying a snappy decompression too.
• Unmarshalling the request body and looping over the data to count timeseries and samples.

I wanted to avoid doing extra work at all costs to keep the "hot path" of Receive fast.

[fpetkovski]

I see, that's a good point. Well initially I thought the middleware would be on this handler which already has the decoded protobuf request, but here we don't have the raw body correct?

Also if I may ask, what is the practical use of setting limits on the request body? As an admin, I am not sure I would know what to set this limit to. I can understand the impact of limiting series or samples, but I would find it hard to know how to configure the body size limit.

[douglascamata]

I see, that's a good point. Well initially I thought the middleware would be on this handler which already has the decoded protobuf request, but here we don't have the raw body correct?

Got it. When it gets to that handler (it does only forward request across the hashring) almost all the "heavy" work is done and some of it will be wasted. Examples:

• If we have the content length header, we could have hit the request size limit and returned the error. But we still grow the copy buffer, copy body to buffer, try to snappy-decode it, unmarshall the protobuffer, apply relabel rules, and theeeen block the request.
• If we hit a timeseries/sample limit, we will apply relabel rules, and then block the request (this seems fine though).

Also if I may ask, what is the practical use of setting limits on the request body? As an admin, I am not sure I would know what to set this limit to. I can understand the impact of limiting series or samples, but I would find it hard to know how to configure the body size limit.

The request size limit is more geared towards networking concerns, I would say. It prevents clients from writing very big requests (even when the other limits aren't configured or hit, i.e. very long label name/values, or when clients are writing "junk"). This can still cause OOMs or possibly create a situation similar to a slow client attack.

This makes me wonder why Receive's remote write endpoint doesn't have a configurable server-side timeout... possibly could add this in a follow up too.

[fpetkovski]

Do we need to check for nil since the default gate is a no-op gate?

[fpetkovski]

You can use units.Megabytes from github.com/alecthomas/units for readability.

[fpetkovski]

What's the reason for this mutation?

[douglascamata]

This is to kind of have 1 as default value for the test scenarios that do not explicitly set amountSamples.

[douglascamata]

Thx for the review, @fpetkovski. Some comments were taken care off already and I replied to others. 🙇

[fpetkovski]

Thanks, this lgtm!

[bwplotka]

Amazing, looks good generally, great work! Leaving for @matej-g to check as well.

[matej-g]

Solid implementation, nice tests! 💪 I'm have only few notes about docs, otherwise I'm happy with this 😊

[matej-g]

Awesome addition and great docs! ❤️

[saswatamcode]

Awesome! 💪🏻
Just a tiny bit curious about how this would behave with hashring config with timeseries being batched, but maybe @douglascamata has already thought of this/tested this? 🙂

[douglascamata]

@saswatamcode I just pushed an update on 58d362b that runs the request limit tests with a 3 node hashring. This should prevent us from changing the batching logic and cause an incoming request that was accepted to hit limits when batched for fanout.

Seems like the fanout requests are now sent via gRPC though:

• From this part in the handler:

  thanos/pkg/receive/handler.go

  Line 656 in fd275f8

  _, err = cl.RemoteWrite(ctx, &storepb.WriteRequest{

• It sends this request:

  thanos/pkg/store/storepb/rpc.pb.go

  Line 1122 in fd275f8

  func (c *writeableStoreClient) RemoteWrite(ctx context.Context, in *WriteRequest, opts ...grpc.CallOption) (*WriteResponse, error) {

• Which is handled here:

  thanos/pkg/receive/handler.go

  Line 768 in fd275f8

  func (h *Handler) RemoteWrite(ctx context.Context, r *storepb.WriteRequest) (*storepb.WriteResponse, error) {

• And finally it ends up calling h.handleRequest:

  thanos/pkg/receive/handler.go

  Line 772 in fd275f8

  err := h.handleRequest(ctx, uint64(r.Replica), r.Tenant, &prompb.WriteRequest{Timeseries: r.Timeseries})

At this point, the limits logic is completely skipped.

[bwplotka]

Thanks 💪🏽 LGTM

[bwplotka]

Less + simpler tests while having the same verifications level = better, so I think a unit tests will suffice here.

It means we're testing Prometheus/Avalanche and not Thanos.

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