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

wycbox · 2021-01-08T09:02:46Z

The origin pull request(#7354) is already stale, so I fork it and try to finish it.

wycbox · 2021-01-08T09:18:06Z

@bwplotka please review it.

tomwilkie

Thanks for resurrecting this @wwformat! Bunch of minor nits, nothing major.

I moved the remote read server code into the remote package in #8536, so I think this might need rebasing against that. It might also make writing an e2e test easier?

tomwilkie · 2021-02-27T10:14:39Z

storage/remote/client.go

@@ -99,7 +100,7 @@ type ClientConfig struct {
 // ReadClient uses the SAMPLES method of remote read to read series samples from remote server.
 // TODO(bwplotka): Add streamed chunked remote read method as well (https://github.com/prometheus/prometheus/issues/5926).
 type ReadClient interface {
-	Read(ctx context.Context, query *prompb.Query) (*prompb.QueryResult, error)
+	StartRead(ctx context.Context, query *prompb.Query, sortSeries bool) (storage.SeriesSet, error)


Do we need to rename the method?

I think StartRead is asynchronous method name, it imply that the result storage.SeriesSet must be handled.

We do this all the time, we have method Query and Select that gives SeriesSet so it might be ok to have just Read for simplicity and consistency (:

tomwilkie · 2021-02-27T10:14:46Z

storage/remote/client.go

@@ -99,7 +100,7 @@ type ClientConfig struct {
 // ReadClient uses the SAMPLES method of remote read to read series samples from remote server.
 // TODO(bwplotka): Add streamed chunked remote read method as well (https://github.com/prometheus/prometheus/issues/5926).


This comment can be removed now.

tomwilkie · 2021-02-27T10:18:45Z

storage/remote/client.go

-	if httpResp.StatusCode/100 != 2 {
-		return nil, errors.Errorf("remote server %s returned HTTP status %s: %s", c.url.String(), httpResp.Status, strings.TrimSpace(string(compressed)))
-	}
-


I'm not sure removing this is correct - for the sample response path, we never seen to check the status code?

How? We still have it removed (:

I moved this code to Client.Read to handle sample/stream response path, like:

start := time.Now() httpResp, err := c.Client.Do(httpReq) if err != nil { return nil, errors.Wrap(err, "error sending request") } if httpResp.StatusCode/100 != 2 { httpResp.Body.Close() return nil, errors.Errorf("remote server %s returned HTTP status %s: %s", c.url.String(), httpResp.Status, strings.TrimSpace(string(compressed))) }

tomwilkie · 2021-02-27T10:20:40Z

storage/remote/codec_test.go

 	"github.com/prometheus/prometheus/pkg/labels"
 	"github.com/prometheus/prometheus/pkg/textparse"
 	"github.com/prometheus/prometheus/prompb"
 	"github.com/prometheus/prometheus/storage"
+	"github.com/prometheus/prometheus/tsdb/tsdbutil"
+	"github.com/stretchr/testify/require"


We try and keep non-prometheus/prometheus imports in a separate block.

bwplotka

Looks good! LGTM overall, just minor suggestions.

Sorry for the massive lag on review. Would you like to fix the last nits?

bwplotka · 2021-04-06T11:41:13Z

storage/remote/client.go

@@ -99,7 +100,7 @@ type ClientConfig struct {
 // ReadClient uses the SAMPLES method of remote read to read series samples from remote server.
 // TODO(bwplotka): Add streamed chunked remote read method as well (https://github.com/prometheus/prometheus/issues/5926).
 type ReadClient interface {
-	Read(ctx context.Context, query *prompb.Query) (*prompb.QueryResult, error)
+	StartRead(ctx context.Context, query *prompb.Query, sortSeries bool) (storage.SeriesSet, error)


We do this all the time, we have method Query and Select that gives SeriesSet so it might be ok to have just Read for simplicity and consistency (:

bwplotka · 2021-04-06T11:41:36Z

storage/remote/client.go

+		return nil, errors.Errorf("not supported remote read content type: %s", contentType)
+	}
+
+	// Handling streamed response


Let's have comments a full sentence 🤗

Suggested change

// Handling streamed response

// Handling streamed response.

bwplotka · 2021-04-06T11:42:08Z

storage/remote/client.go

-	if httpResp.StatusCode/100 != 2 {
-		return nil, errors.Errorf("remote server %s returned HTTP status %s: %s", c.url.String(), httpResp.Status, strings.TrimSpace(string(compressed)))
-	}
-


How? We still have it removed (:

Tanemahuta · 2021-04-06T14:47:58Z

Hi there. I have a suggestion for refactoring the complete ReadClient.
My suggestions breaks the interface though, but unifies the way to read the time series either streamed or unstreamed.
It will also solve the problem of executing multiple queries at once (as mentioned the TODO in the client).

The API looks as follows:

type ReadClient interface {
	Read(ctx context.Context, stream bool, queries ...*prompb.Query) (ReadResponse, error)
}

// ReadResponse is the interface for an iterable response
type ReadResponse interface {
	// Next returns true in case there is another time series
	Next() bool
	// At returns a partial or full TimeSeries or nil
	At() *TimeSeries
	// Err denotes the error that iteration as failed with. When an error occurs, set cannot continue to iterate.
	Err() error
	// IsPartial returns true if the TimeSeries in this response are chunked
	IsPartial() bool
}

// TimeSeries represents a partial or complete time series from a query
type TimeSeries struct {
	// Labels denote the identifier of the TimeSeries
	Labels []prompb.Label
	// Samples denote the time stamps and values
	Samples []prompb.Sample
	// QueryIdx denotes the index of the original query
	QueryIdx int64
}

The implementation is done, but I have to create tests for it.

Since this would most probably replace 100% of the code of this PR, I would like all of the participants to decide if this approach or the one of PR is preferred.

Best regards, Christian

Signed-off-by: Yunchuan Wen <yunchuanwen@gmail.com>

pstibrany · 2022-04-25T08:53:15Z

What is the status of this PR? It seems like in the current state it was good-enough to be merged. Is there anything we can do to help?

juansc · 2022-07-19T23:10:22Z

We're also looking forward to this feature. Any updates?

Sniper91 · 2022-11-14T11:42:50Z

I think both #8351 and #11379 are not good enough.
The read client interface below isn't properly designed.

StartRead(ctx context.Context, query *prompb.Query, sortSeries bool) (storage.SeriesSet, error)

if response type is sampled, it's ok to return storage.SeriesSet.
But if response type is chunked, the chunked data must be decompressed to return storage.SeriesSet.
In ChunkQuerier method

prometheus/storage/remote/read.go

Line 232 in 93e451c

    
           func (q *chunkQuerier) Select(sortSeries bool, hints *storage.SelectHints, matchers ...*labels.Matcher) storage.ChunkSeriesSet {

the storage.SeriesSet is compressed to return storage.ChunkSeriesSet.
So the chunked data firstly are decompressed and then compressed if storage.ChunkQuerier.Select is invoked.
I think it depends on the response type to return storage.SeriesSet or storage.ChunkedSeriesSet.

Base automatically changed from master to main February 23, 2021 19:36

tomwilkie reviewed Feb 27, 2021

View reviewed changes

tomwilkie mentioned this pull request Feb 27, 2021

added streaming remote read client side functionality #7354

Closed

tomwilkie added the component/remote storage label Feb 27, 2021

wycbox force-pushed the streamread branch from f00c96a to 69eb0a4 Compare March 11, 2021 08:05

wycbox requested review from bwplotka, csmarchbanks and cstyan as code owners March 11, 2021 08:05

wycbox force-pushed the streamread branch 4 times, most recently from 8a632c4 to 7587a51 Compare March 26, 2021 06:05

bwplotka approved these changes Apr 6, 2021

View reviewed changes

bwplotka mentioned this pull request Apr 6, 2021

Allow Prometheus to use ReadRequest_STREAMED_XOR_CHUNKS when calling remote read API. #5926

Open

streaming remote read

bac0a57

Signed-off-by: Yunchuan Wen <yunchuanwen@gmail.com>

wycbox force-pushed the streamread branch from 7587a51 to bac0a57 Compare April 7, 2021 11:44

stale bot added the stale label Jun 6, 2021

leizor mentioned this pull request Sep 30, 2022

Add streaming remote read to ReadClient #11379

Open

wycbox closed this Mar 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wycbox commented Jan 8, 2021 •

edited by tomwilkie

wycbox commented Jan 8, 2021

tomwilkie left a comment

tomwilkie Feb 27, 2021

wycbox Mar 11, 2021

bwplotka Apr 6, 2021

wycbox Apr 7, 2021

tomwilkie Feb 27, 2021

tomwilkie Feb 27, 2021

wycbox Mar 11, 2021

bwplotka Apr 6, 2021

wycbox Apr 7, 2021

tomwilkie Feb 27, 2021

bwplotka left a comment

bwplotka Apr 6, 2021

bwplotka Apr 6, 2021

bwplotka Apr 6, 2021

Tanemahuta commented Apr 6, 2021

pstibrany commented Apr 25, 2022

juansc commented Jul 19, 2022

Sniper91 commented Nov 14, 2022

		@@ -99,7 +100,7 @@ type ClientConfig struct {
		// ReadClient uses the SAMPLES method of remote read to read series samples from remote server.
		// TODO(bwplotka): Add streamed chunked remote read method as well (https://github.com/prometheus/prometheus/issues/5926).

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

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

Conversation

wycbox commented Jan 8, 2021 • edited by tomwilkie

wycbox commented Jan 8, 2021

tomwilkie 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka 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

Tanemahuta commented Apr 6, 2021

pstibrany commented Apr 25, 2022

juansc commented Jul 19, 2022

Sniper91 commented Nov 14, 2022

wycbox commented Jan 8, 2021 •

edited by tomwilkie