Support remote write v2 by converting request #6330

SungJin1212 · 2024-11-11T06:13:58Z

This PR supports Prometheus remote write 2.0 by converting the v2 request to v1 at the API.

Which issue(s) this PR fixes:
Fixes #6324

Checklist

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

yeya24 · 2024-11-14T21:16:53Z

Looks promising. Thanks!

FYI we have prometheus/client_golang#1658 which exports remote write handler. Not a blocker for this PR but we should keep it on our radar to switch to use the library

SungJin1212 · 2024-11-15T00:53:02Z

@yeya24
Thanks for letting me know.
Should we make the issue to track it?

alanprot · 2024-11-15T19:16:47Z

Maybe we can open a issue for someone give a try to use the client_golang handler even before it get merged so we can give feedback on the open PR. Changing that handler after is merged probably will be more difficult as it could potentially break all projects that are already using it.

SungJin1212 · 2024-11-15T22:28:59Z

@alanprot
I added a comment here: #6324

yeya24 · 2024-11-17T20:19:11Z

I took a breif look at prometheus/client_golang#1658. Left some comments there and we have some changes Cortex specific that might not make sense for Prometheus. I think we are ok to proceed with this PR first.

SungJin1212 · 2024-11-18T00:34:20Z

@yeya24
Thanks. I read it, and it would be good if we could reuse its functions!

yeya24 · 2024-11-19T05:36:35Z

pkg/util/push/push.go

+			}
+		case config.RemoteWriteProtoMsgV2:
+			var req writev2.Request
+			err := util.ParseProtoReader(ctx, r.Body, int(r.ContentLength), maxRecvMsgSize, &req, util.RawSnappy)


@alanprot @danielblando
I wonder if we want to introduce a feature flag to control the behavior for RW v2 request. We can either ignore the request or convert to v1 and in the future maybe just accept as is.

Let's add a feature flag for the purpose of rollout. If RW 2.0 conversion is enabled right away, then Ingesters need to be rolled out first because of the protocol change to return stats. If we want to rollout Ingester and Distributor the same time then things can go wrong without a feature flag.

yeya24 · 2024-11-24T18:14:13Z

pkg/ingester/ingester.go

-	return &cortexpb.WriteResponse{}, nil
+	writeResponse := &cortexpb.WriteResponse{
+		Samples:    int64(succeededSamplesCount),
+		Histograms: int64(succeededSamplesCount), //TODO(Sungjin1212): Should we count histogram?


How is this implemented in Prometheus. Why we don't count histogram here?

I left TODO since we are counting histograms just as we count the sample.
But, the Prometheus is counting native histogram https://github.com./prometheus/prometheus/blob/main/storage/remote/write_handler.go#L424.

How about starting to count histogram when we introduce PushV2?

I don't understand the concern here. There is nothing prevent us doing it. We should count native histograms

yes, I agree with counting native histograms by changing to Histograms: int64(nativeHistogramCount).
My concern is we are counting samples instead of native histograms
https://github.com./cortexproject/cortex/blob/master/pkg/ingester/ingester.go#L1269

I am fine to split but why it needs a separate PR? We can just add a new int64 variable to count succeeded histograms

Maybe some changes are needed like we are tracking ingestionRate by calculating succeededSamplesCount + ingestedMetadata.
We should change the calculation to sustain existing behavior to succeededSamplesCount + succeededHistogramCount + ingestedMetadata
Also, we can introduce new metrics like cortex_ingester_ingested_native_histograms_total and cortex_ingester_ingested_histograms_failures_total.
WDYT?

I am ok to add the new metrics. But they are not blocking this PR so can be done either now or after this change.

If we don't add new metrics just track succeeded histogram samples, it is a simple change.

Ok, I will make PR soon!

@yeya24
I make the PR addressing it! (#6370)

yeya24 · 2024-11-24T18:17:07Z

pkg/distributor/distributor.go

@@ -816,7 +824,7 @@ func (d *Distributor) doBatch(ctx context.Context, req *cortexpb.WriteRequest, s
 			}
 		}

-		return d.send(localCtx, ingester, timeseries, metadata, req.Source)
+		return d.send(localCtx, ingester, timeseries, metadata, req.Source, stats)


Do we need to sum the samples pulled from stats? Now I see we just overwrite stats for every request.

If so, isn't there a good chance the returned header value (X-Prometheus-Remote-Write-Samples-Written) would be multiple of samples in a write request?

Yeah. With replication factor it is expected to have more samples. I think this is fine.

yeya24 · 2024-11-24T18:18:50Z

pkg/util/push/push.go

+			}
+		case config.RemoteWriteProtoMsgV2:
+			var req writev2.Request
+			err := util.ParseProtoReader(ctx, r.Body, int(r.ContentLength), maxRecvMsgSize, &req, util.RawSnappy)


Let's add a feature flag for the purpose of rollout. If RW 2.0 conversion is enabled right away, then Ingesters need to be rolled out first because of the protocol change to return stats. If we want to rollout Ingester and Distributor the same time then things can go wrong without a feature flag.

SungJin1212 · 2024-11-27T02:02:30Z

@yeya24
I added -distributor.remote-write2-enabled flags to configure whether the Distributor can accept PRW2.0.
I added the TestIngesterRollingUpdate e2e test, where the Distributor can accept PRW2.0 and the Ingester uses the v1.18.1 image.
The result is PRW2.0 push is a success, but the response header (X-Prometheus-Remote-Write-xxx) values are all "0".
Is it expecting behavior?

yeya24 · 2024-12-03T02:08:39Z

The result is PRW2.0 push is a success, but the response header (X-Prometheus-Remote-Write-xxx) values are all "0".
Is it expecting behavior?

This doesn't sound like the right behavior. Is that what you got with this PR?

SungJin1212 · 2024-12-03T02:17:20Z

@yeya24
Yes, the test condition is that Ingester uses a v1.18.1 image and Distributor uses a PRW2.0-implemented one. The PRW2.0 push then gets that result.
If the Ingester and Distributor use the same images (PRW 2.0 implemented), we can get the expected response header.

yeya24 · 2024-12-03T03:48:40Z

Yes, the test condition is that Ingester uses a v1.18.1 image and Distributor uses a PRW2.0-implemented one. The PRW2.0 push then gets that result.

I see what you meant. Then it is expected to get that result if you use Ingester of old version and Distributor of new version.
That's why we introduce the PRW 2.0 feature flag in distributor to only enable PRW 2.0 request if backend Ingester is running the newer version.

SungJin1212 · 2024-12-03T06:50:49Z

@yeya24
Should I add comments to -distributor.remote-write2-enabled so that the user can do a rolling update of the Ingesters first and then update the Distributor afterward?

yeya24 · 2024-12-03T17:51:49Z

We can mention it in the flag description but I doubt users really look at it.
I prefer to create a dedicated doc/guide for users to migrate to Prometheus 3.0

SungJin1212 · 2024-12-04T00:55:48Z

@yeya24
Yes, the guide docs would be more good.

CharlieTLe · 2025-01-23T03:18:14Z

Hello @SungJin1212, thank you for opening this PR.

There is a release in progress. As such, please rebase your CHANGELOG entry on top of the master branch and move the CHANGELOG entry to the top under ## master / unreleased.

Thanks,
Charlie

yeya24 · 2025-03-03T23:58:34Z

I think it is time to revisit this now : )

yeya24 · 2025-04-08T23:32:46Z

Ping for review. @danielblando @alanprot @friedrichg

alanprot · 2025-04-11T05:26:16Z

pkg/util/push/push.go

+		contentType := r.Header.Get("Content-Type")
+		if contentType == "" {
+			contentType = appProtoContentType
+		}
+
+		msgType, err := parseProtoMsg(contentType)
 		if err != nil {
-			level.Error(logger).Log("err", err.Error())
-			http.Error(w, err.Error(), http.StatusBadRequest)
+			level.Error(logger).Log("Error decoding remote write request", "err", err)
+			http.Error(w, err.Error(), http.StatusUnsupportedMediaType)
 			return
 		}

-		req.SkipLabelNameValidation = false
-		if req.Source == 0 {
-			req.Source = cortexpb.API
+		if msgType != config.RemoteWriteProtoMsgV1 && msgType != config.RemoteWriteProtoMsgV2 {
+			level.Error(logger).Log("Not accepted msg type", "msgType", msgType, "err", err)
+			http.Error(w, err.Error(), http.StatusUnsupportedMediaType)
+			return
+		}


Thinking out loud:
Should we be in the safe side and fallback to v1 if the content-type is not expected?

I guess if right now u send a weird content type cortex will acccept? and after this change we would return "StatusUnsupportedMediaType"?

The PRW2.0 spec says the receiver should return StatusUnsupportedMediaType.

Would it be more nicer if we fallback to v1 when the StatusUnsupportedMediaType case?

I think so? we don’t wanna to break v1 clients I guess ? Maybe we should check what v1 protocol says in this case instead v2 … cause we cannot assume v2 protocol behavior if we don’t know what version the request is as the content type is “unexpected” ?

How about enabling check content-type if remoteWrite2Enabled is true?

I changed not to break existing behavior. Could you take a look?

alanprot · 2025-04-11T05:28:51Z

pkg/util/push/push.go

+			}
+		case config.RemoteWriteProtoMsgV2:
+			if remoteWrite2Enabled {
+				var req writev2.Request


Do you think we should also pool the "writev2.Request" like we do with the "PreallocWriteRequest", so we avoid lots of GC?

Maybe for this PR its ok to not do that but i imagine the GC will be quite high with v2 if we dont do that.

yeah, we need to introduce pool like v1. Let's update it to #6324.

I added benchmarks and here are the results:

Benchmark_Handler Benchmark_Handler/PRW1_with_10_series Benchmark_Handler/PRW1_with_10_series-10 109134 10406 ns/op 24970 B/op 246 allocs/op Benchmark_Handler/PRW2_with_10_series Benchmark_Handler/PRW2_with_10_series-10 73774 16280 ns/op 33920 B/op 374 allocs/op Benchmark_Handler/PRW1_with_100_series Benchmark_Handler/PRW1_with_100_series-10 13016 90841 ns/op 233753 B/op 2319 allocs/op Benchmark_Handler/PRW2_with_100_series Benchmark_Handler/PRW2_with_100_series-10 9264 131270 ns/op 297826 B/op 3350 allocs/op Benchmark_Handler/PRW1_with_500_series Benchmark_Handler/PRW1_with_500_series-10 2686 442439 ns/op 1148513 B/op 11523 allocs/op Benchmark_Handler/PRW2_with_500_series Benchmark_Handler/PRW2_with_500_series-10 1904 619988 ns/op 1423212 B/op 16554 allocs/op Benchmark_Handler/PRW1_with_1000_series Benchmark_Handler/PRW1_with_1000_series-10 1360 875548 ns/op 2305732 B/op 23027 allocs/op Benchmark_Handler/PRW2_with_1000_series Benchmark_Handler/PRW2_with_1000_series-10 952 1241676 ns/op 3047640 B/op 33057 allocs/op Benchmark_Handler/PRW1_with_2000_series Benchmark_Handler/PRW1_with_2000_series-10 674 1815843 ns/op 4627024 B/op 46032 allocs/op Benchmark_Handler/PRW2_with_2000_series Benchmark_Handler/PRW2_with_2000_series-10 478 2528554 ns/op 6302383 B/op 66062 allocs/op

SungJin1212 · 2025-04-14T01:22:42Z

@yeya24
Should we automatically enable -blocks-storage.tsdb.enable-native-histograms if the -distributor.remote-write2-enabled is true?
If we set the protobuf_message to io.prometheus.write.v2.Request on the Prometheus, the Prometheus sends CT and NH by default. (remote write config)

Signed-off-by: SungJin1212 <[email protected]>

pull-request-size bot added the size/XXL label Nov 11, 2024

dosubot bot added the component/distributor label Nov 11, 2024

SungJin1212 force-pushed the Add-remote-write-v2-api branch 6 times, most recently from 07bbbee to 83d0ba6 Compare November 11, 2024 11:25

SungJin1212 mentioned this pull request Nov 12, 2024

Prometheus Remote Write v2 Implementation #6324

Open

7 tasks

yeya24 reviewed Nov 19, 2024

View reviewed changes

yeya24 reviewed Nov 24, 2024

View reviewed changes

SungJin1212 force-pushed the Add-remote-write-v2-api branch from 83d0ba6 to 6ce5027 Compare November 27, 2024 01:57

SungJin1212 force-pushed the Add-remote-write-v2-api branch from 6ce5027 to 5344155 Compare November 28, 2024 09:28

SungJin1212 force-pushed the Add-remote-write-v2-api branch from 5344155 to a9231e4 Compare December 18, 2024 11:02

SungJin1212 force-pushed the Add-remote-write-v2-api branch from a9231e4 to 8ec7204 Compare January 14, 2025 06:34

SungJin1212 force-pushed the Add-remote-write-v2-api branch from 8ec7204 to 521ece7 Compare January 31, 2025 01:42

SungJin1212 force-pushed the Add-remote-write-v2-api branch 2 times, most recently from f269bc8 to 090be16 Compare February 4, 2025 06:19

SungJin1212 force-pushed the Add-remote-write-v2-api branch 6 times, most recently from d562e33 to c1e3e04 Compare March 10, 2025 02:02

SungJin1212 force-pushed the Add-remote-write-v2-api branch from c1e3e04 to cd9d024 Compare April 8, 2025 07:11

yeya24 requested review from danielblando and alanprot April 8, 2025 23:32

alanprot reviewed Apr 11, 2025

View reviewed changes

SungJin1212 force-pushed the Add-remote-write-v2-api branch 2 times, most recently from 6e75881 to 715d9c9 Compare April 15, 2025 04:36

SungJin1212 added 3 commits April 22, 2025 18:10

Support remote write v2 by converting request

a80f90b

Signed-off-by: SungJin1212 <[email protected]>

Change to not break exist behavior

3523a93

Signed-off-by: SungJin1212 <[email protected]>

Add benchmarks

e5b85ee

Signed-off-by: SungJin1212 <[email protected]>

SungJin1212 force-pushed the Add-remote-write-v2-api branch from 715d9c9 to e5b85ee Compare April 22, 2025 09:11

Support remote write v2 by converting request #6330

Are you sure you want to change the base?

Support remote write v2 by converting request #6330

Conversation

SungJin1212 commented Nov 11, 2024 • edited Loading

yeya24 commented Nov 14, 2024

SungJin1212 commented Nov 15, 2024

alanprot commented Nov 15, 2024

SungJin1212 commented Nov 15, 2024

yeya24 commented Nov 17, 2024

SungJin1212 commented Nov 18, 2024

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

SungJin1212 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

yeya24 Nov 25, 2024 • edited Loading

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

SungJin1212 commented Nov 27, 2024 • edited Loading

yeya24 commented Dec 3, 2024

SungJin1212 commented Dec 3, 2024 • edited Loading

yeya24 commented Dec 3, 2024

SungJin1212 commented Dec 3, 2024 • edited Loading

yeya24 commented Dec 3, 2024 • edited Loading

SungJin1212 commented Dec 4, 2024

CharlieTLe commented Jan 23, 2025

yeya24 commented Mar 3, 2025

yeya24 commented Apr 8, 2025

Choose a reason for hiding this comment

SungJin1212 Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

alanprot Apr 11, 2025 • edited Loading

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

SungJin1212 commented Apr 14, 2025 • edited Loading

SungJin1212 commented Nov 11, 2024 •

edited

Loading

SungJin1212 Nov 25, 2024 •

edited

Loading

yeya24 Nov 25, 2024 •

edited

Loading

SungJin1212 commented Nov 27, 2024 •

edited

Loading

SungJin1212 commented Dec 3, 2024 •

edited

Loading

SungJin1212 commented Dec 3, 2024 •

edited

Loading

yeya24 commented Dec 3, 2024 •

edited

Loading

SungJin1212 Apr 11, 2025 •

edited

Loading

alanprot Apr 11, 2025 •

edited

Loading

SungJin1212 commented Apr 14, 2025 •

edited

Loading