Distributor: Add limits for Otel write request byte size #8574

ying-jeanne · 2024-07-01T20:42:08Z

What this PR does

Which issue(s) this PR fixes or relates to

The current distributor message limit is 100MB See doc, Which demonstrates too much for compressed Otel write request. This could cause problem for otel, end up in big decompressed message and timeout. Knowing that otlp collector could have different compression algorithm and also can also be configured to be no compression. See doc, introduce new uncompressed request byte size in order to reject abusive write request as early as possible.

Why not using existing distributor.max-recv-msg-size?

The remote write and otlp request doesn't write request with same format, doesn't contains same quantity samples when requests are the same size.
In the same container, we can have several users who use otel and remote write during the same time, they need to be limited with different values

About default value

The current big request is rejected by 413 error code, which is not retryable, just to be on the safe side, put 100M as limit, we saw 100MiB request end up executing 10s. This needs to be carefully tested/observed in order to find the final sweet default value.

Adding runbook dedicated to the error

Checklist

Tests updated.
Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
about-versioning.md updated with experimental features.

pstibrany

Thank you for working on this.

pkg/distributor/distributor.go

pkg/distributor/otel.go

pkg/distributor/otel_test.go

pkg/distributor/push.go

pkg/util/globalerror/user.go

aknuds1

Please see comments. I did a quick review since I saw some fundamental problems. Please ping me for another review when my comments have been addressed.

CHANGELOG.md

docs/sources/mimir/configure/about-versioning.md

pkg/distributor/distributor.go

pkg/distributor/otel.go

pkg/distributor/otel_test.go

docs/sources/mimir/manage/mimir-runbooks/_index.md

pkg/distributor/push.go

tacole02

Thanks for updating these docs!

CHANGELOG.md

docs/sources/mimir/configure/configuration-parameters/index.md

docs/sources/mimir/manage/mimir-runbooks/_index.md

docs/sources/mimir/configure/configuration-parameters/index.md

docs/sources/mimir/manage/mimir-runbooks/_index.md

pstibrany · 2024-07-04T07:37:46Z

docs/sources/mimir/manage/mimir-runbooks/_index.md

+
+How it **works**:
+
+- The distributor implements an upper limit on the message size of incoming OTel write requests after decompression regardless of the compression type. Refer to [OTLP collector compression details](https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configgrpc/README.md#client-configuration) for more information.


Referenced documentation seems irrelevant here, because it points to gRPC client config, but Mimir doesn't accept OTLP/gRPC requests.

~~I don't think we need to mention "compression" at all in this runbook.~~ It doesn't do any harm.

nice catch, gracais

updated link to confighttp

docs/sources/mimir/manage/mimir-runbooks/_index.md

pkg/distributor/push.go

CHANGELOG.md

pstibrany · 2024-07-04T07:46:02Z

pkg/distributor/distributor.go

@@ -237,6 +240,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet, logger log.Logger) {
 	cfg.RetryConfig.RegisterFlags(f)

 	f.IntVar(&cfg.MaxRecvMsgSize, "distributor.max-recv-msg-size", 100<<20, "Max message size in bytes that the distributors will accept for incoming push requests to the remote write API. If exceeded, the request will be rejected.")
+	f.IntVar(&cfg.MaxOTLPRequestSize, maxOTLPRequestSizeFlag, 80<<20, "Maximum OTLP request size in bytes that the distributors accept. Requests exceeding this limit are rejected.")


How was the 80 MiB selected? Do we have some data about size of the request and processing time?

Intuitively 80 MiB still feels very large. With previous 100 MiB we sometimes see translation to Mimir request taking up to 10 seconds, so perhaps better value would be somewhere close to 10 MiB?

as mentioned in the description of the PR, it is tentative. Since 413 error is not retryable, we need to be cautious. if 100MiB/10s, how about 2s of response time ~ 20 MiB

I agree that we need to be careful when rolling this out. It may be safer to use original value of 100 MiB for now, so that this doesn't break existing setups, and then we can experiment with correct value in our setup.

let me revert my reverted commit

pstibrany

Thank you for addressing my feedback. I left few more minor comments.

pstibrany

Thank you!

CHANGELOG.md

docs/sources/mimir/configure/about-versioning.md

aknuds1 · 2024-07-04T11:16:06Z

Reviewing again

aknuds1

LGTM, with a couple of suggested clarifications wrt. compression.

docs/sources/mimir/manage/mimir-runbooks/_index.md

ying-jeanne · 2024-07-04T11:59:31Z

Thank you! @pstibrany @aknuds1 @tacole02. Let's merge and now set it lower along the time and reject some requests🤞

Co-authored-by: Peter Štibraný <pstibrany@gmail.com>

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>

ying-jeanne force-pushed the max-massage-request-bytes branch 7 times, most recently from 40c6a2e to 2f4d3d9 Compare July 3, 2024 09:32

ying-jeanne marked this pull request as ready for review July 3, 2024 10:00

ying-jeanne requested review from tacole02 and a team as code owners July 3, 2024 10:00

pstibrany reviewed Jul 3, 2024

View reviewed changes

aknuds1 requested changes Jul 3, 2024

View reviewed changes

ying-jeanne force-pushed the max-massage-request-bytes branch from d8183b2 to 6951714 Compare July 3, 2024 15:49

ying-jeanne requested review from stevesg and grafanabot as code owners July 3, 2024 15:49

tacole02 approved these changes Jul 3, 2024

View reviewed changes

ying-jeanne force-pushed the max-massage-request-bytes branch 4 times, most recently from 8534200 to 51e14cd Compare July 3, 2024 16:52

ying-jeanne requested review from pstibrany and aknuds1 July 3, 2024 16:54

ying-jeanne force-pushed the max-massage-request-bytes branch from 51e14cd to 2c9e58f Compare July 3, 2024 16:55

pstibrany reviewed Jul 4, 2024

View reviewed changes

docs/sources/mimir/manage/mimir-runbooks/_index.md Show resolved Hide resolved

pstibrany reviewed Jul 4, 2024

View reviewed changes

pkg/distributor/push.go Outdated Show resolved Hide resolved

pstibrany reviewed Jul 4, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pstibrany reviewed Jul 4, 2024

View reviewed changes

ying-jeanne force-pushed the max-massage-request-bytes branch from ff229ed to 59c7075 Compare July 4, 2024 10:50

ying-jeanne force-pushed the max-massage-request-bytes branch from 59c7075 to 1599188 Compare July 4, 2024 11:00

ying-jeanne requested a review from pstibrany July 4, 2024 11:02

pstibrany approved these changes Jul 4, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

docs/sources/mimir/configure/about-versioning.md Outdated Show resolved Hide resolved

aknuds1 approved these changes Jul 4, 2024

View reviewed changes

docs/sources/mimir/manage/mimir-runbooks/_index.md Outdated Show resolved Hide resolved

docs/sources/mimir/manage/mimir-runbooks/_index.md Outdated Show resolved Hide resolved

ying-jeanne force-pushed the max-massage-request-bytes branch from b83a721 to b101031 Compare July 4, 2024 13:32

ying-jeanne and others added 12 commits July 4, 2024 15:34

Distributor: Add limits for otel write request byte size

07bf037

remove uncompressed limit

e01ebbb

Addresss comments

050acba

Adddress comments on doc

f64f325

Update CHANGELOG.md

17bed53

Co-authored-by: Peter Štibraný <pstibrany@gmail.com>

Update pkg/distributor/push.go

d351221

Co-authored-by: Peter Štibraný <pstibrany@gmail.com>

update doc and set new default

df1af51

Update CHANGELOG.md

b1be7b2

Co-authored-by: Peter Štibraný <pstibrany@gmail.com>

Update docs/sources/mimir/configure/about-versioning.md

f7db46f

Co-authored-by: Peter Štibraný <pstibrany@gmail.com>

Update docs/sources/mimir/manage/mimir-runbooks/_index.md

d4fe693

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>

Update docs/sources/mimir/manage/mimir-runbooks/_index.md

ef51d7d

Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>

fix unittest

7e92bcc

ying-jeanne force-pushed the max-massage-request-bytes branch from b101031 to 7e92bcc Compare July 4, 2024 13:34

ying-jeanne merged commit e7ab906 into main Jul 4, 2024
29 checks passed

ying-jeanne deleted the max-massage-request-bytes branch July 4, 2024 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distributor: Add limits for Otel write request byte size #8574

Distributor: Add limits for Otel write request byte size #8574

ying-jeanne commented Jul 1, 2024 •

edited

Loading

pstibrany left a comment

aknuds1 left a comment •

edited

Loading

tacole02 left a comment

pstibrany Jul 4, 2024 •

edited

Loading

ying-jeanne Jul 4, 2024

ying-jeanne Jul 4, 2024

pstibrany Jul 4, 2024

ying-jeanne Jul 4, 2024

pstibrany Jul 4, 2024

ying-jeanne Jul 4, 2024

pstibrany left a comment

pstibrany left a comment

aknuds1 commented Jul 4, 2024

aknuds1 left a comment

ying-jeanne commented Jul 4, 2024 •

edited

Loading


		How it works:

		- The distributor implements an upper limit on the message size of incoming OTel write requests after decompression regardless of the compression type. Refer to [OTLP collector compression details](https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configgrpc/README.md#client-configuration) for more information.

Distributor: Add limits for Otel write request byte size #8574

Distributor: Add limits for Otel write request byte size #8574

Conversation

ying-jeanne commented Jul 1, 2024 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

pstibrany left a comment

Choose a reason for hiding this comment

aknuds1 left a comment • edited Loading

Choose a reason for hiding this comment

tacole02 left a comment

Choose a reason for hiding this comment

pstibrany Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

ying-jeanne Jul 4, 2024

Choose a reason for hiding this comment

ying-jeanne Jul 4, 2024

Choose a reason for hiding this comment

pstibrany Jul 4, 2024

Choose a reason for hiding this comment

ying-jeanne Jul 4, 2024

Choose a reason for hiding this comment

pstibrany Jul 4, 2024

Choose a reason for hiding this comment

ying-jeanne Jul 4, 2024

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

aknuds1 commented Jul 4, 2024

aknuds1 left a comment

Choose a reason for hiding this comment

ying-jeanne commented Jul 4, 2024 • edited Loading

ying-jeanne commented Jul 1, 2024 •

edited

Loading

aknuds1 left a comment •

edited

Loading

pstibrany Jul 4, 2024 •

edited

Loading

ying-jeanne commented Jul 4, 2024 •

edited

Loading