Receive: Allow remote write request limits to be defined per file and tenant #5565

douglascamata · 2022-08-02T16:00:59Z

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

Changes

This is part of the broader work outlined by #5404.

Allow request limits & gates to be configured using a file.
A user can save the file to disk and pass its path to receive.limits-config-file or pass the file content inline to receive.limits-config.
Documentation was updated with an example and explanation about how default and tenant limits interact.

Follow ups

Add a mechanism to reload the tenant limit configuration to avoid having to restart the Receive and reprocess the WAL.
- I am already preparing a solution for this. It'll come in a separate PR soon after this is merged.
Eventually move the configuration for the active series limit added in Meta-monitoring based active series limiting #5520 to the same file.

Verification

Unit tests.
Ran it locally.

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

…s-per-tenant-config-file Signed-off-by: GitHub <noreply@github.com>

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

squat · 2022-08-02T16:28:39Z

xref: #5527 (comment)

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

…s-per-tenant-config-file

douglascamata · 2022-08-03T13:00:29Z

@yeya24 based on the conversation we had at #5527 (comment), after I tried to add CLI args to configure the default limits in this PR while keeping also the file my opinion on this has changed for the following reasons:

It adds complexity for users of Thanos to understand the configuration
1. The logic becomes convoluted. There would be CLI args that override missing or present default values that override missing tenant values.
2. It even allows for partial configuration of defaults via CLI and file at the same time, creating potential confusing setups for users (i.e. CLI defines default limit of series while config file defines default limit of request size).
It adds complexity in the code without clear benefits
1. Each one of these levels have different default values (zero or nil) that have to be handled.
2. The type we use as a standard for passing configuration files (extflag.PathOrContent) also supports inlining of the configuration as a CLI argument. It's not difficult to use it to inline the configuration proposed in this PR in case you only want to set defaults.

I don't think adding extra CLI args is worth the price in extra complexity given the features and simplicity that we get from extflag.PathOrContent. What do you think?

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

bwplotka · 2022-08-04T11:28:49Z

docs/components/receive.md

+The next sections explain what each configuration value means.
+
+```yaml mdox-exec="cat pkg/receive/testdata/limits_config/good_limits.yaml"
+write:


Idea from Community Hours: generalize on any label - not only scope to tenant labels.

This allows us to keep tenancy topic separate.

Perhaps this can help with other use cases 🤔

To make it clear for potential reviewers, from what I understood in the community hours, this PR and be reviewed as merged as is. The feature is documented as experimental and thus we can introduce changes to the configuration file and behavior in case we see fit, especially after the planned discussion regarding tenancy in Thanos.

douglascamata · 2022-08-04T14:04:45Z

I found out during verification tests that the default values for limits are not being exported in the Receive's metrics. Will fix this.

edit: Fixed!

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

…s-per-tenant-config-file

fpetkovski · 2022-08-09T14:29:35Z

Would changing the limits require receivers to be redeployed?

douglascamata · 2022-08-09T14:34:08Z

@fpetkovski yes. We might want to improve this with some mechanism to reload this configuration in a follow up PR, like the config reload endpoint other components have.

Probably during reload time the limits would be shortly disabled to avoid synchronization issues under high load. WDYT?

fpetkovski · 2022-08-09T15:00:41Z

Yeah I think config reloading will be more than a nice to have for this feature. Large receiver deployments can take hours to rollout, so we should try to speed up configuration changes if we can.

saswatamcode

Thanks for awesome work! 💪🏻
LGTM minus some comments!

saswatamcode · 2022-08-10T04:17:05Z

cmd/thanos/receive.go

@@ -194,6 +194,18 @@ func runReceive(
 		return errors.Wrap(err, "parse relabel configuration")
 	}

+	var limitsConfig *receive.RootLimitsConfig
+	if conf.limitsConfig != nil {
+		limitsContentYaml, err := conf.limitsConfig.Content()


As @fpetkovski mentioned, having some /~/reload endpoint like we have in Thanos Ruler, to reload such configuration would be amazing, as adding some tenant limits now, would mean redeploying.

Yup, leaving this for another PR to avoid this one getting too big.

saswatamcode · 2022-08-10T04:20:34Z

pkg/receive/limiter.go

+type limiter struct {
+	requestLimiter requestLimiter
+	writeGate      gate.Gate
+	// activeSeriesLimiter *activeSeriesLimiter `yaml:"active_series"`


Do we need these commented struct fields?
I can add an active series limit to this config in a follow-up PR, maybe a TODO would be better here? 🙂

Fixed in e5c171d. 👍

saswatamcode · 2022-08-10T04:25:49Z

pkg/receive/limiter_config.go

+)
+
+// RootLimitsConfig is the root configuration for limits.
+type RootLimitsConfig struct {


Why not just LimitsConfig? 🙂

Too many things end up being called by almost the same name and it gets confusing over time. This is already a result of many iterations on the naming. 😅

saswatamcode · 2022-08-10T04:32:21Z

pkg/receive/request_limiter.go

+// Copyright (c) The Thanos Authors.
+// Licensed under the Apache License 2.0.
+
+package receive


Is there a particular reason for having separate limiter and request_limiter files?

Limiter is mean to hold a bunch of different "limiters" (i.e. limits per request, the concurrency gate for writing data, active series limiter, and possibly a rate limiter in the future), parse the configuration of limits, initialize every limiter to avoid too much clutter all over around that code that does handler creation and setup (which also helps with reusability/dont-repeat-yourself principle plus encapsulation).

I also have an eye in the future, where we would be able to partially reuse a bunch of the code that is in these files in the Query component. It already has a similar gate and I plan to add similar per-request limits to it.

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

bwplotka

This is clean, however, it still is tenant aware, even though it could be just "per label". I assume the conclusion is to use per tenant limits at the end?

LGTM otherwise, great job! (:

douglascamata · 2022-08-11T18:31:37Z

it still is tenant aware, even though it could be just "per label". I assume the conclusion is to use per tenant limits at the end?

I can refactor this to be purely label-aware and take a few more days (or weeks) to get it ready, tested, and documented while the feature doesn't land.

Or land the feature as experimental (very likely to change) and then work on extending and changing it to be just label-aware afterwards. I thought this was our conclusion from the community meeting. Wasn't it, @bwplotka?

douglascamata · 2022-08-11T18:33:46Z

I can even already fill up the code, example configuration file, and logs with warnings that the configuration structure will be changing soon to work on a per-label basis.

douglascamata · 2022-08-11T18:35:53Z

A point of the discussion: being straightforward that this feature supports tenancy is a plus for people operating Thanos clusters. Changing it to be label aware is friendly to maintainers/contributors (keep tenancy logic away and "hidden") but not to users (tenancy logic is hidden even though Receive is tenant-aware).

douglascamata · 2022-08-16T14:05:00Z

There might be other points to consider before committing to make this purely label aware solution too:

Does it even make sense to apply a "request body size limit" to a subset of the request?
- Only applicable limits are maximum amount of timeseries and samples.
Does it make sense performance-wise?
- Regex matching or simple exact match?
- What would the request response look like if only part of it hit a limit?
Complex logic vs simple per tenant limits
- It becomes possible that two limit rules apply to given set of timeseries, which one would have priority?
- What if there are multiple labels per group? i.e. labelA=valueA && labelB=valueB.
- What if there are multiple values to group by, will regex performance be acceptable? i.e. labelA=valueA || labelA=valueB.
- How to enable configuration if each individual value should be its own group? i.e. apply the limits for each unique value of labelA.

So I prefer to not rush directly into it.

saswatamcode · 2022-08-29T12:27:09Z

Maybe this can be merged now (once conflicts are resolved) while discussion is under heavy progress, as this feature is experimental and would likely go through some iterations to get prod-ready? 🙂

…nant-config-file

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

…nant-config-file Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

bwplotka

Not much progress on long term goals for tenancy specific, so merging, since those are hidden flags anyway.

Thanks!

… tenant (thanos-io#5565) * Allow per-tenant limits to be configured via file Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Refactor Receive's limiting logic Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Fix some methods that were in plural Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Improve metric description Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Add a TODO for later Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Do some cleanup after moving limits to config file Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Isolate rest of limiting logic from the handler Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Small refactor to the request limiter Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Rename MergeWith -> OverlayWith Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Update changelog Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Update documentation Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Add missing copyright notice to few files Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Fix test after change in config file tenants Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Retrigger CI because of bundled-Cortex failing test Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Expose default limits as metrics Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Retrigger CI Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Replace comment with a TODOs Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Fix changelog after bad merge Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> Signed-off-by: GitHub <noreply@github.com> Signed-off-by: Prakul Jain <prakul.jain@udaan.com>

… tenant (thanos-io#5565) * Allow per-tenant limits to be configured via file Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Refactor Receive's limiting logic Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Fix some methods that were in plural Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Improve metric description Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Add a TODO for later Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Do some cleanup after moving limits to config file Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Isolate rest of limiting logic from the handler Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Small refactor to the request limiter Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Rename MergeWith -> OverlayWith Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Update changelog Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Update documentation Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Add missing copyright notice to few files Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Fix test after change in config file tenants Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Retrigger CI because of bundled-Cortex failing test Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Expose default limits as metrics Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Retrigger CI Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Replace comment with a TODOs Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Fix changelog after bad merge Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> Signed-off-by: GitHub <noreply@github.com>

douglascamata added 5 commits August 2, 2022 14:40

Allow per-tenant limits to be configured via file

e50c690

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

Refactor Receive's limiting logic

da60fe7

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

Fix some methods that were in plural

ddbde37

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

Improve metric description

62fa80b

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

Merge branch 'main' of https://github.com/thanos-io/thanos into limit…

bf6f7b7

…s-per-tenant-config-file Signed-off-by: GitHub <noreply@github.com>

pull-request-size bot added the size/XL label Aug 2, 2022

Add a TODO for later

6afb222

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

douglascamata added 3 commits August 2, 2022 16:30

Do some cleanup after moving limits to config file

9534029

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

Isolate rest of limiting logic from the handler

0bfd2ec

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

Small refactor to the request limiter

f67ff0b

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

douglascamata changed the title ~~Limits per tenant config file~~ Allow remote write request limits to be defined per file and tenant Aug 2, 2022

douglascamata changed the title ~~Allow remote write request limits to be defined per file and tenant~~ [receive] Allow remote write request limits to be defined per file and tenant Aug 2, 2022

douglascamata added 2 commits August 2, 2022 17:13

Rename MergeWith -> OverlayWith

4cc442d

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

Merge branch 'main' of https://github.com/thanos-io/thanos into limit…

0b3046f

…s-per-tenant-config-file

douglascamata changed the title ~~[receive] Allow remote write request limits to be defined per file and tenant~~ Receive: Allow remote write request limits to be defined per file and tenant Aug 3, 2022

douglascamata added 5 commits August 3, 2022 13:24

Update changelog

6926b0e

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

Update documentation

3e8df8d

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

Add missing copyright notice to few files

babbb22

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

Fix test after change in config file tenants

a4ae1d1

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

Retrigger CI because of bundled-Cortex failing test

b466ffa

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

bwplotka reviewed Aug 4, 2022

View reviewed changes

Expose default limits as metrics

9dd8644

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

douglascamata marked this pull request as ready for review August 5, 2022 17:26

douglascamata added 3 commits August 5, 2022 17:28

Retrigger CI

ffb9842

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

Merge branch 'main' of https://github.com/thanos-io/thanos into limit…

ff4efde

…s-per-tenant-config-file

Merge branch 'main' of https://github.com/thanos-io/thanos into limit…

dc6d25d

…s-per-tenant-config-file

saswatamcode previously approved these changes Aug 10, 2022

View reviewed changes

Replace comment with a TODOs

e5c171d

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

douglascamata dismissed saswatamcode’s stale review via e5c171d August 10, 2022 11:36

douglascamata mentioned this pull request Aug 10, 2022

Add write limits for tenants in Thanos Receiver #5404

Closed

6 tasks

bwplotka previously approved these changes Aug 11, 2022

View reviewed changes

Merge branch 'main' of github.com:thanos-io/thanos into limits-per-te…

246ea5c

…nant-config-file

douglascamata dismissed bwplotka’s stale review via 246ea5c August 29, 2022 15:53

douglascamata added 2 commits August 29, 2022 17:54

Fix changelog after bad merge

25ba51d

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

Merge branch 'main' of github.com:thanos-io/thanos into limits-per-te…

ca55514

…nant-config-file Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

bwplotka approved these changes Sep 6, 2022

View reviewed changes

bwplotka merged commit b8eb3d8 into thanos-io:main Sep 6, 2022

saswatamcode mentioned this pull request Sep 12, 2022

Receive: Make head series limiting config per tenant #5685

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Receive: Allow remote write request limits to be defined per file and tenant #5565

Receive: Allow remote write request limits to be defined per file and tenant #5565

douglascamata commented Aug 2, 2022 •

edited

Loading

squat commented Aug 2, 2022

douglascamata commented Aug 3, 2022 •

edited

Loading

bwplotka Aug 4, 2022

douglascamata Aug 9, 2022

douglascamata commented Aug 4, 2022 •

edited

Loading

fpetkovski commented Aug 9, 2022

douglascamata commented Aug 9, 2022

fpetkovski commented Aug 9, 2022

saswatamcode left a comment

saswatamcode Aug 10, 2022

douglascamata Aug 10, 2022

saswatamcode Aug 10, 2022

douglascamata Aug 10, 2022

saswatamcode Aug 10, 2022

douglascamata Aug 10, 2022

bwplotka Aug 11, 2022

saswatamcode Aug 10, 2022

douglascamata Aug 10, 2022 •

edited

Loading

douglascamata Aug 10, 2022 •

edited

Loading

bwplotka left a comment

douglascamata commented Aug 11, 2022

douglascamata commented Aug 11, 2022

douglascamata commented Aug 11, 2022 •

edited

Loading

douglascamata commented Aug 16, 2022

saswatamcode commented Aug 29, 2022

bwplotka left a comment

Receive: Allow remote write request limits to be defined per file and tenant #5565

Receive: Allow remote write request limits to be defined per file and tenant #5565

Conversation

douglascamata commented Aug 2, 2022 • edited Loading

Changes

Follow ups

Verification

squat commented Aug 2, 2022

douglascamata commented Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douglascamata commented Aug 4, 2022 • edited Loading

fpetkovski commented Aug 9, 2022

douglascamata commented Aug 9, 2022

fpetkovski commented Aug 9, 2022

saswatamcode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douglascamata Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

douglascamata Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

douglascamata commented Aug 11, 2022

douglascamata commented Aug 11, 2022

douglascamata commented Aug 11, 2022 • edited Loading

douglascamata commented Aug 16, 2022

saswatamcode commented Aug 29, 2022

bwplotka left a comment

Choose a reason for hiding this comment

douglascamata commented Aug 2, 2022 •

edited

Loading

douglascamata commented Aug 3, 2022 •

edited

Loading

douglascamata commented Aug 4, 2022 •

edited

Loading

douglascamata Aug 10, 2022 •

edited

Loading

douglascamata Aug 10, 2022 •

edited

Loading

douglascamata commented Aug 11, 2022 •

edited

Loading