distributor: do inflight checks before reading request (for httpgrpc requests) #6300

pstibrany · 2023-10-09T14:07:08Z

What this PR does

This PR implements inflight checks in Distributor by using gRPC method limiter introduced in grafana/dskit#377. This PR relies on unmerged dskit PR grafana/dskit#392 (waiting for new gRPC-go release).

This PR also relies on clients passing the correct metadata headers for httpgrpc requests (using methods introduced in grafana/dskit#391).

To be rebased on top of #6301, after it's merged.

Checklist

Tests updated
[na] Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pracucci · 2023-10-12T09:04:57Z

pkg/mimir/grpc_push_check.go

+		httpUrl := getSingleMetadata(md, httpgrpc.MetadataURL)
+		msgSize := getMessageSize(md, grpcutil.MetadataMessageSize)
+
+		if httpMethod == http.MethodPost && (httpUrl == api.PrometheusPushEndpoint || httpUrl == api.OTLPPushEndpoint) {


I'm not sure checking the URL for equality will work. Reason is that the endpoint is exposed under a configurable prefix (and I think the prefix is still the URL received here). What if we check if the URL ends with the requested endpoint? That's what we do in the query-frontend.

Good point, done in 5cd002d.

pracucci · 2023-10-12T09:15:31Z

pkg/distributor/distributor.go

+	inflight := d.inflightPushRequests.Inc()
+	defer func() {
+		if cleanupInDefer {
+			d.inflightPushRequests.Dec()


I'm wondering if to keep the code more future proof, we should decrease d.inflightPushRequestsBytes here. My take is that in in the future we'll add more checks after d.checkRequestSize() then we'll miss subtracting it. The check here could be done on rs.requestSize.

Actually we could just change the defer function into a call to cleanupAfterPushFinished().

Done in adfcca0.

pkg/distributor/distributor.go

pstibrany · 2023-10-23T14:08:34Z

I think this is now ready for review. Compared to previous version of the PR, we now check size of httpgrpc.Request and mimirpb.WriteRequest separately, and they both contribute to inflight request size, because they are both in memory while request is being handled.

Extension of this (for future PR) could be to check for OTLP Request size too in OTLP Push handler.

pracucci

Good job Peter! I can't see any issue. I left a couple of nits and questions (don't block on me re-reviewing it).

pracucci · 2023-10-23T15:33:51Z

CHANGELOG.md

@@ -49,6 +49,7 @@
 * [ENHANCEMENT] Querier: reduce memory consumed for queries that hit store-gateways. #6348
 * [ENHANCEMENT] Ruler: include corresponding trace ID with log messages associated with rule evaluation. #6379
 * [ENHANCEMENT] Querier: clarify log messages and span events emitted while querying ingesters, and include both ingester name and address when relevant. #6381
+* [ENHANCEMENT] Ingester, Distributor: added experimental support for rejecting push requests received via gRPC before reading them into memory, if ingester or distributor is unable to accept the request. This is activated by using `-ingester.limit-inflight-requests-using-grpc-method-limiter` for ingester, and `-distributor.limit-inflight-requests-using-grpc-method-limiter` for distributor. #5976 #6300


Mention the experimental flags in docs/sources/mimir/configure/about-versioning.md too, please.

pracucci · 2023-10-23T15:37:03Z

pkg/mimir/grpc_push_check.go

+		// Let's check httpgrpc metadata, if present.
+		httpMethod := getSingleMetadata(md, httpgrpc.MetadataMethod)
+		httpUrl := getSingleMetadata(md, httpgrpc.MetadataURL)
+		msgSize := getMessageSize(md, grpcutil.MetadataMessageSize)


[nit] We could move this inside the if branch.

pracucci · 2023-10-23T15:41:08Z

pkg/mimir/grpc_push_check_test.go

+
+		ctx, err = l.RPCCallStarting(context.Background(), httpgrpcHandleMethod, metadata.New(map[string]string{
+			httpgrpc.MetadataMethod:      "POST",
+			httpgrpc.MetadataURL:         api.PrometheusPushEndpoint,


[nit] Would be nice to add a prefix to ensure it matches anyway.

pracucci · 2023-10-23T15:49:55Z

pkg/distributor/distributor.go

+	il := d.getInstanceLimits()
+	if il.MaxInflightPushRequests > 0 && inflight > int64(il.MaxInflightPushRequests) {
+		d.rejectedRequests.WithLabelValues(reasonDistributorMaxInflightPushRequests).Inc()
+		return ctx, nil, errMaxInflightRequestsReached


Can you confirm the reason why we removed the wrapping to util_log.DoNotLogError{} is because all errors returned by this function will not be logged anyway? If so, I would add a comment to clarify it.

Added comment in 00e660f. Wrapping is still done in limitsMiddleware.

pracucci · 2023-10-23T15:52:13Z

pkg/distributor/distributor.go

+
+func (d *Distributor) checkWriteRequestSize(rs *requestState, writeRequestSize int64) error {
+	// If writeRequestSize was already checked, don't check it again.
+	if rs.writeRequestSize > 0 {


Question: I'm good with this check, but could it ever happen that we call checkWriteRequestSize() twice for the same request? If yes, in which case? I can't see it.

I don't see how that would happen. It's only called from limitsMiddleware, and not from grpc method check. (We only call checkHttpgrpcRequestSize from there).

Signed-off-by: Peter Štibraný <[email protected]>

… requests. Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

Use prefix in tests. Signed-off-by: Peter Štibraný <[email protected]>

…otLogError. Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

pstibrany · 2023-10-24T13:07:06Z

Good job Peter! I can't see any issue. I left a couple of nits and questions (don't block on me re-reviewing it).

Thanks for review. I've addressed your feedback, setting auto-merge. 🎉

pracucci self-requested a review October 12, 2023 08:51

pracucci reviewed Oct 12, 2023

View reviewed changes

pstibrany force-pushed the distributor-push-grpc-check branch from dbb1f77 to 25e9f1f Compare October 23, 2023 12:32

pstibrany marked this pull request as ready for review October 23, 2023 14:06

pstibrany requested review from a team as code owners October 23, 2023 14:06

pracucci approved these changes Oct 23, 2023

View reviewed changes

pstibrany added 9 commits October 24, 2023 14:05

Allow performing inflight checks before push handler starts.

56588ac

Signed-off-by: Peter Štibraný <[email protected]>

Add check for ingestion rate.

11b5fba

Signed-off-by: Peter Štibraný <[email protected]>

distributor: when using httpgrpc check inflight limits before reading…

52ae5af

… requests. Signed-off-by: Peter Štibraný <[email protected]>

CHANGELOG.md entry.

60ea3bd

Signed-off-by: Peter Štibraný <[email protected]>

Use suffix check.

bbeb3a1

Signed-off-by: Peter Štibraný <[email protected]>

Don't return gRPC errors from Distributor.

00133d0

Signed-off-by: Peter Štibraný <[email protected]>

Call cleanupAfterPushFinished from defer.

8caa52b

Signed-off-by: Peter Štibraný <[email protected]>

Separate httpgrpcRequestSize and writeRequestSize.

da5c593

Signed-off-by: Peter Štibraný <[email protected]>

Mention new flags in docs/sources/mimir/configure/about-versioning.md.

304de04

Signed-off-by: Peter Štibraný <[email protected]>

pstibrany force-pushed the distributor-push-grpc-check branch from acc3661 to 304de04 Compare October 24, 2023 12:07

pstibrany added 3 commits October 24, 2023 14:10

Extract message size only when handling distributor push request.

e65144f

Use prefix in tests. Signed-off-by: Peter Štibraný <[email protected]>

Update comment for startPushRequest, mention wrapping to util_log.DoN…

00e660f

…otLogError. Signed-off-by: Peter Štibraný <[email protected]>

Fix linter unhappiness.

12ee2a2

Signed-off-by: Peter Štibraný <[email protected]>

pstibrany enabled auto-merge (squash) October 24, 2023 13:07

pstibrany merged commit 272e355 into main Oct 24, 2023

pstibrany deleted the distributor-push-grpc-check branch October 24, 2023 13:10

pstibrany mentioned this pull request Oct 25, 2023

Improve distributor's TestStartFinishRequest test. #6481

Merged

1 task

This was referenced Nov 1, 2023

distributor: check for inflight bytes before reading httpgrpc message without message size in metadata. #6534

Open

Show rejected distributor requests in the dashboard. #6556

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

distributor: do inflight checks before reading request (for httpgrpc requests) #6300

distributor: do inflight checks before reading request (for httpgrpc requests) #6300

pstibrany commented Oct 9, 2023 •

edited

Loading

pracucci Oct 12, 2023

pstibrany Oct 23, 2023

pracucci Oct 12, 2023

pracucci Oct 12, 2023

pstibrany Oct 23, 2023

pstibrany commented Oct 23, 2023

pracucci left a comment

pracucci Oct 23, 2023

pstibrany Oct 24, 2023

pracucci Oct 23, 2023

pstibrany Oct 24, 2023

pracucci Oct 23, 2023

pstibrany Oct 24, 2023

pracucci Oct 23, 2023

pstibrany Oct 24, 2023 •

edited

Loading

pracucci Oct 23, 2023

pstibrany Oct 24, 2023

pstibrany commented Oct 24, 2023

distributor: do inflight checks before reading request (for httpgrpc requests) #6300

distributor: do inflight checks before reading request (for httpgrpc requests) #6300

Conversation

pstibrany commented Oct 9, 2023 • edited Loading

What this PR does

Checklist

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

pstibrany commented Oct 23, 2023

pracucci 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

pstibrany Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany commented Oct 24, 2023

pstibrany commented Oct 9, 2023 •

edited

Loading

pstibrany Oct 24, 2023 •

edited

Loading