Add streaming remote read to ReadClient #11379

leizor · 2022-09-30T22:02:58Z

Here's another attempt at closing #5926.

I noticed #8351 was stale, and I think this PR has a few improvements over it:

I believe the implementation of chunkSeriesIterator.Seek wasn't correct (it allowed for backwards seeking and does not seek past the current chunk).
I think I've simplified the implementations of chunkedSeriesSet, chunkedSeries, and chunkedSeriesIterator.
The sizeLimit parameter for the ChunkedReader used by the read client is configurable.
The read_queries_total and read_request_duration_seconds metrics have an added response_type label that track whether the response from the server was sampled or chunked.
The client filters returned samples in the chunked response that are outside of the queried range.

roidelapluie · 2023-07-11T11:56:33Z

cc @bwplotka can you please review this? Thanks

bwplotka

Looks solid, thanks! 💪🏽

LGTM, just the issue with instrumentation of this. Streaming changes a little bit in semantics of those metrics. Let's ensure those metric remain useful.

storage/remote/client.go

storage/remote/codec.go

storage/remote/codec_test.go

bwplotka

Last nit, otherwise LGTM, thanks!

bwplotka · 2023-07-25T12:20:25Z

storage/remote/client.go

@@ -35,12 +35,23 @@ import (
 	"go.opentelemetry.io/otel"
 	"go.opentelemetry.io/otel/trace"

+	"github.com/prometheus/prometheus/storage"
+


blank line not needed

bwplotka · 2023-07-25T12:20:51Z

storage/remote/client.go

+		prompb.ReadRequest_STREAMED_XOR_CHUNKS,
+		prompb.ReadRequest_SAMPLES,
+	}
+)


Let's merge those var sections together?

bwplotka · 2023-07-25T12:22:02Z

storage/remote/client.go

@@ -66,10 +77,10 @@ var (
 			Namespace: namespace,
 			Subsystem: subsystem,
 			Name:      "read_request_duration_seconds",
-			Help:      "Histogram of the latency for remote read requests.",
+			Help:      "Histogram of the latency for remote read requests. Note that for streamed responses this is only the duration of the initial call and does not include the processing of the stream.",


I wished we could actually move this to be full latency including receiving all bytes etc, but good enough for now I guess.

storage/remote/client.go

leizor · 2023-07-26T20:03:37Z

Thanks for all the feedback @bwplotka!

roidelapluie · 2023-09-26T11:33:44Z

cc @bwplotka can you look at this PR?

bwplotka · 2023-12-12T11:46:36Z

Ayay, sorry for delay. Will try to look this/next week. Thanks!

bwplotka

Thanks, and sorry for lag. One non-blocking comment, otherwise LGTM, good to go.

storage/remote/codec.go

bwplotka

I tried to rebase + resolve conflicts, but it seems it got sideways. Do you mind rebasing that one last time? Sorry!

leizor · 2024-03-28T18:33:19Z

@bwplotka Yep, not a problem - done!

Signed-off-by: Justin Lei <justin.lei@grafana.com>

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Justin Lei <justin.lei@grafana.com>

Signed-off-by: Justin Lei <lei.justin@gmail.com>

leizor · 2024-07-17T23:48:14Z

@bwplotka I did another rebase/resolve conflicts cycle - would you mind taking a look again? Let me know if you'd like me to squash all the commits together or anything like that!

Kran001 · 2024-08-26T14:45:36Z

Hello everyone!
Any news on this issue?

beorn7 · 2024-08-27T19:45:21Z

I have tried to ping @bwplotka in other means. Sorry for the lag.

bwplotka

Thanks for amazing work here.

Apologies for lags, but remote-read went outside of my immediate work scope, limiting my time here a bit. Solid code, thanks 💪🏽

GiedriusS · 2024-08-28T09:25:38Z

storage/remote/client.go

+		c.readQueriesDuration.WithLabelValues("chunked").Observe(time.Since(start).Seconds())
+
+		s := NewChunkedReader(httpResp.Body, c.chunkedReadLimit, nil)
+		return NewChunkedSeriesSet(s, httpResp.Body, query.StartTimestampMs, query.EndTimestampMs, func(err error) {


You need to buffer everything here due to #12605 + resort. Is this being done somewhere? I can't find it.

Those comments are hard to track - would you like to add a issue about this if there's some work to improve things here?

leizor force-pushed the leizor/streaming-remote-read branch 2 times, most recently from 6349bc9 to 714f69b Compare September 30, 2022 22:14

leizor marked this pull request as ready for review October 1, 2022 00:39

leizor requested review from codesome, csmarchbanks, cstyan, bwplotka and tomwilkie as code owners October 1, 2022 00:39

Sniper91 mentioned this pull request Nov 14, 2022

added streaming remote read client side functionality (fork) #8351

Closed

leizor force-pushed the leizor/streaming-remote-read branch 7 times, most recently from 5e7735f to 350226f Compare April 24, 2023 16:08

codesome removed their request for review April 28, 2023 21:41

bwplotka approved these changes Jul 13, 2023

View reviewed changes

roidelapluie assigned jesusvazquez Jul 18, 2023

leizor force-pushed the leizor/streaming-remote-read branch from e373a94 to d082eec Compare July 18, 2023 19:30

bwplotka approved these changes Jul 25, 2023

View reviewed changes

leizor force-pushed the leizor/streaming-remote-read branch 2 times, most recently from e70669b to 957b07e Compare July 26, 2023 20:02

roidelapluie mentioned this pull request Sep 26, 2023

added streaming remote read client side functionality #7354

Closed

leizor force-pushed the leizor/streaming-remote-read branch 2 times, most recently from 18a0dac to 6f91a36 Compare October 18, 2023 23:14

bwplotka previously approved these changes Mar 26, 2024

View reviewed changes

storage/remote/codec.go Show resolved Hide resolved

bwplotka reviewed Mar 26, 2024

View reviewed changes

leizor force-pushed the leizor/streaming-remote-read branch 2 times, most recently from dd088cc to 21891f6 Compare March 28, 2024 17:58

leizor force-pushed the leizor/streaming-remote-read branch from 870b1d8 to c6dec94 Compare April 26, 2024 17:57

leizor dismissed bwplotka’s stale review via c6dec94 June 14, 2024 15:21

leizor and others added 8 commits July 17, 2024 14:03

Add streaming remote read to ReadClient

a905f56

Signed-off-by: Justin Lei <justin.lei@grafana.com>

Apply suggestions from code review

f3f7c94

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Justin Lei <justin.lei@grafana.com>

Remote read instrumentation tweaks

e8778af

Signed-off-by: Justin Lei <lei.justin@gmail.com>

Minor cleanups

7a8d43c

Signed-off-by: Justin Lei <lei.justin@gmail.com>

In-line handleChunkedResponse

3dce5ef

Signed-off-by: Justin Lei <lei.justin@gmail.com>

Fix lints

f587f60

Signed-off-by: Justin Lei <lei.justin@gmail.com>

Explicitly call cancel() when needed

bfbd6f1

Signed-off-by: Justin Lei <lei.justin@gmail.com>

Update chunkedSeries, chunkedSeriesIterator for new interfaces

6616a90

Signed-off-by: Justin Lei <lei.justin@gmail.com>

leizor force-pushed the leizor/streaming-remote-read branch from c6dec94 to 604a84e Compare July 17, 2024 22:35

Adapt remote.chunkedSeries to use prompb.ChunkedSeries

a86b8f9

Signed-off-by: Justin Lei <lei.justin@gmail.com>

leizor force-pushed the leizor/streaming-remote-read branch from 604a84e to a86b8f9 Compare July 17, 2024 22:37

Fix lint

65ba5fa

Signed-off-by: Justin Lei <lei.justin@gmail.com>

leizor force-pushed the leizor/streaming-remote-read branch from 7dd71cf to 65ba5fa Compare July 17, 2024 23:13

leizor requested a review from bwplotka July 18, 2024 17:41

bwplotka approved these changes Aug 28, 2024

View reviewed changes

bwplotka merged commit 3a82cd5 into prometheus:main Aug 28, 2024

GiedriusS reviewed Aug 28, 2024

View reviewed changes

leizor deleted the leizor/streaming-remote-read branch August 28, 2024 14:25

This was referenced Aug 30, 2024

update mimir-prometheus at grafana/mimir-prometheus@53cf3fb grafana/mimir#9148

Merged

update prometheus no client golang at grafana/mimir-prometheus@fdf902d grafana/mimir#9156

Merged

Add streaming remote read to ReadClient #11379

Add streaming remote read to ReadClient #11379

Conversation

leizor commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roidelapluie commented Jul 11, 2023

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

bwplotka Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

bwplotka Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

bwplotka Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leizor commented Jul 26, 2023

Uh oh!

roidelapluie commented Sep 26, 2023

Uh oh!

bwplotka commented Dec 12, 2023

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

leizor commented Mar 28, 2024

Uh oh!

leizor commented Jul 17, 2024

Uh oh!

Kran001 commented Aug 26, 2024

Uh oh!

beorn7 commented Aug 27, 2024

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

GiedriusS Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leizor commented Sep 30, 2022 •

edited

Loading

GiedriusS Aug 28, 2024 •

edited

Loading