Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
bwplotka opened this issue Aug 19, 2019 · 20 comments · May be fixed by #8351
Open

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

bwplotka opened this issue Aug 19, 2019 · 20 comments · May be fixed by #8351

Comments

@bwplotka
Copy link
Member

@bwplotka bwplotka commented Aug 19, 2019

#5703 extended remote read protocol and added ReadRequest_STREAMED_XOR_CHUNKS

Prometheus implements remote read API, but it can also be configured to use/call other remote read API. Enable streaming remote read if the server supports it.

AC:

  • Prometheus remote read client is capable to use ReadRequest_STREAMED_XOR_CHUNKS

Similar client code for inspiration: thanos-io/thanos#1268

@YaoZengzeng
Copy link
Contributor

@YaoZengzeng YaoZengzeng commented Aug 29, 2019

I'd like to fix this issue 😃 @bwplotka

Maybe we can solve this problem by following steps:

  1. Extend the Read method of Client in storage/remote/client.go to support both SAMPLE and STREAM, the return type changed to *prompb.QueryResult to *http.Response would be better.

  2. Refactor FromQueryResult to decode both type of response and expose unified storage.SeriesSet.

  3. Encapsulate the chunks into some struct to make it implement storage.SeriesSet. Actually the implementation method is similar to chunkSeries stuff in tsdb/query.go. In addition, this step may easy to be implemented if #5882 being merged :)
    ( At first I‘d encapsulate the chunks to make the code run)

My initial idea is probably like this, any comments? @bwplotka

Test is also needed and now I'm trying to figure out how to do it properly.

@bwplotka
Copy link
Member Author

@bwplotka bwplotka commented Aug 29, 2019

Sorry for lag and thanks @YaoZengzeng

Yes that makes sense to me 👍

I think step 2 would be the most work, especially packing SAMPLED method to SeriesSet, but I guess we can't really avoid this.

For Streaming be careful as we might need to implement something like this we did in Thanos in SeriesSet.

@YaoZengzeng
Copy link
Contributor

@YaoZengzeng YaoZengzeng commented Aug 30, 2019

@bwplotka Thanks for your comments and I'll fix this issue step by step 😃

@YaoZengzeng
Copy link
Contributor

@YaoZengzeng YaoZengzeng commented Sep 4, 2019

@bwplotka Please review #5974 again, otherwise the fixing of this issue can't move on 😂

@bwplotka
Copy link
Member Author

@bwplotka bwplotka commented Sep 11, 2019

Sorry for massive lag, proposed solution (: Also reach me on IRC bwplotka or CNCF slack channel -> bwplotka if you need to ping me - bit spammed on email.

@bwplotka
Copy link
Member Author

@bwplotka bwplotka commented Mar 13, 2020

Hi @YaoZengzeng this would be nice to have (: Any progress? What are the blockers? 🤗

@Sudhar287
Copy link

@Sudhar287 Sudhar287 commented Apr 5, 2020

Hello @YaoZengzeng. If you're not going to continue this, can I please take your work forward?

@YaoZengzeng
Copy link
Contributor

@YaoZengzeng YaoZengzeng commented Apr 6, 2020

@Sudhar287 Yes, Please :)

@Sudhar287
Copy link

@Sudhar287 Sudhar287 commented Apr 6, 2020

Thank you for the/your quick response. :)

@bwplotka
Copy link
Member Author

@bwplotka bwplotka commented Apr 24, 2020

@Sudhar287 any update maybe? (: We kind of slowly need that for various tooling. (:

If not I think it might be good Community Bridge issue. cc @khyatisoneji

@Sudhar287
Copy link

@Sudhar287 Sudhar287 commented Apr 24, 2020

Hello @bwplotka! It seems that this PR #6042 cannot proceed until #6747 is completed according to this. I was waiting for that to be finished first...Do you have a different proposal? :)

@bwplotka
Copy link
Member Author

@bwplotka bwplotka commented Apr 24, 2020

Wow, I think there is misunderstanding, I don't think anything is blocked and we can start working on it right way.

@bwplotka
Copy link
Member Author

@bwplotka bwplotka commented Apr 24, 2020

I commented #6042 (comment)

I don't think there was anything blocked... Sorry for confusion 🤦 We are definitely good to go.

We kind of need this as we wrote on Thanos tool for serving CSV / JSON as some our API and we want to extend it to remote-read. Would be nice if we can chunks to Prometheus though 🤗

@Sudhar287
Copy link

@Sudhar287 Sudhar287 commented Apr 24, 2020

Thanks for the response and clarification. I'll start working on it! :)

@Sudhar287
Copy link

@Sudhar287 Sudhar287 commented May 28, 2020

fyi, working on this actively. Will have the PR next week or so. :)

@bwplotka
Copy link
Member Author

@bwplotka bwplotka commented May 28, 2020

@Tanemahuta
Copy link

@Tanemahuta Tanemahuta commented Apr 5, 2021

Hello there,

I have an implementation ready for which I refactored to ReadClient 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
}

This also solves // TODO: Support batching multiple queries into one read request, as the protobuf interface allows for it.

Is the sketched solution suitable for you? - If so, I will prepare a PR.

@bwplotka
Copy link
Member Author

@bwplotka bwplotka commented Apr 6, 2021

It looks great @Tanemahuta, but we already have another PR #8351 (review)

Maybe you can help us review it and maybe give your design as suggestions in the comments? 🤗

@Tanemahuta
Copy link

@Tanemahuta Tanemahuta commented Apr 6, 2021

Will do. This changes the complete design and breaks the API though.

@detailyang
Copy link
Contributor

@detailyang detailyang commented Dec 30, 2021

@bwplotka any update? read STREAMED_XOR_CHUNKS response is important to reduce memory overhead and query latency:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants