Add streaming remote read to ReadClient #11379
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
leizor
force-pushed
the
leizor/streaming-remote-read
branch
2 times, most recently
from
6349bc9
to
714f69b
Compare
leizor
requested review from
codesome,
csmarchbanks,
cstyan,
bwplotka and
tomwilkie
as code owners
leizor
force-pushed
the
leizor/streaming-remote-read
branch
7 times, most recently
from
5e7735f
to
350226f
Compare
|
cc @bwplotka can you please review this? Thanks |
bwplotka
approved these changes
Jul 13, 2023
leizor
force-pushed
the
leizor/streaming-remote-read
branch
from
e373a94
to
d082eec
Compare
bwplotka
approved these changes
Jul 25, 2023
storage/remote/client.go
Outdated
| @@ -35,12 +35,23 @@ import ( | |||
| "go.opentelemetry.io/otel" | |||
| "go.opentelemetry.io/otel/trace" | |||
|
|
|||
| "github.com/prometheus/prometheus/storage" | |||
|
|
|||
storage/remote/client.go
Outdated
| prompb.ReadRequest_STREAMED_XOR_CHUNKS, | ||
| prompb.ReadRequest_SAMPLES, | ||
| } | ||
| ) |
There was a problem hiding this comment.
Let's merge those var sections together?
storage/remote/client.go
Outdated
| @@ -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.", | |||
There was a problem hiding this comment.
I wished we could actually move this to be full latency including receiving all bytes etc, but good enough for now I guess.
leizor
force-pushed
the
leizor/streaming-remote-read
branch
2 times, most recently
from
e70669b
to
957b07e
Compare
|
Thanks for all the feedback @bwplotka! |
|
cc @bwplotka can you look at this PR? |
leizor
force-pushed
the
leizor/streaming-remote-read
branch
2 times, most recently
from
18a0dac
to
6f91a36
Compare
|
Ayay, sorry for delay. Will try to look this/next week. Thanks! |
bwplotka
approved these changes
Dec 14, 2023
storage/remote/client.go
Outdated
| @@ -298,7 +309,11 @@ func (c *Client) Read(ctx context.Context, query *prompb.Query) (*prompb.QueryRe | |||
| httpReq.Header.Set("X-Prometheus-Remote-Read-Version", "0.1.0") | |||
|
|
|||
| ctx, cancel := context.WithTimeout(ctx, c.timeout) | |||
| defer cancel() | |||
| defer func() { | |||
There was a problem hiding this comment.
I think it would be cleaner at this point to avoid copying cancel but explictly cancel on errors as needed as you did for status code
There was a problem hiding this comment.
Good point - will do!
storage/remote/client.go
Outdated
| return FromQueryResult(sortSeries, res), nil | ||
| } | ||
|
|
||
| func (c *Client) handleChunkedResponse(httpResp *http.Response, mint, maxt int64, cancel func(error)) storage.SeriesSet { |
There was a problem hiding this comment.
Let's inline this method to the caller, too shallow to have it TBH
leizor
force-pushed
the
leizor/streaming-remote-read
branch
4 times, most recently
from
6049a11
to
09bca48
Compare
|
@bwplotka Thanks for all the feedback, I believe this is now ready to merge! |
|
@bwplotka did this fall through the cracks? Could you have a look? |
bwplotka
approved these changes
Mar 26, 2024
bwplotka
reviewed
Mar 26, 2024
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>
Signed-off-by: Justin Lei <lei.justin@gmail.com>
Signed-off-by: Justin Lei <lei.justin@gmail.com>
leizor
force-pushed
the
leizor/streaming-remote-read
branch
2 times, most recently
from
dd088cc
to
21891f6
Compare
|
@bwplotka Yep, not a problem - done! |
Signed-off-by: Justin Lei <lei.justin@gmail.com>
Signed-off-by: Justin Lei <lei.justin@gmail.com>
leizor
force-pushed
the
leizor/streaming-remote-read
branch
from
870b1d8
to
c6dec94
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here's another attempt at closing #5926.
I noticed #8351 was stale, and I think this PR has a few improvements over it:
chunkSeriesIterator.Seekwasn't correct (it allowed for backwards seeking and does not seek past the current chunk).chunkedSeriesSet,chunkedSeries, andchunkedSeriesIterator.sizeLimitparameter for theChunkedReaderused by the read client is configurable.read_queries_totalandread_request_duration_secondsmetrics have an addedresponse_typelabel that track whether the response from the server wassampledorchunked.