-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
remote-read: [1] Extended remote read proto with negotiation and chunked response. #5701
Conversation
Pinged Cortex team for feedback: cortexproject/cortex#1472 |
prompb/remote.proto
Outdated
@@ -43,3 +65,12 @@ message QueryResult { | |||
// Samples within a time series must be ordered by time. | |||
repeated prometheus.TimeSeries timeseries = 1; | |||
} | |||
|
|||
// ChunkedReadResponse is a response when response_type equals STREAMED_XOR_CHUNKS. | |||
// We strictly stream full series by series. Single frame can contain partion of the single series. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what "We strictly stream full series by series" means. I thought it means that each single series wouldn't be broken up, but then the next sentence says the opposite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It rather means that we expect series to be be sequentially send.
So you cannot send series ABC [1 10] then XYZ [1-20] then ABC [11-20] but you can ABC [1 10] then ABC [11-20] then XYZ [1-20] (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to be more explicit. PTAL. Also not 100% sure if we want to be that strict on interface side.
Comments fixed, PTAL. |
@brian-brazil Moved to |
@tomwilkie any feedback on this? |
|
||
// We require this to match chunkenc.Encoding. | ||
enum Encoding { | ||
UNKNOWN = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is UNKOWN ever used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, but please see comment, it helps to match chunkenc.Encoding
exact values of enum (int).
// ChunkedReadResponse is a response when response_type equals STREAMED_XOR_CHUNKS. | ||
// We strictly stream full series after series, optionally split by time. This means that a single frame can contain | ||
// partition of the single series, but once a new series is started to be streamed it means that no more chunks will | ||
// be sent for previous one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whilst this might be true, it seems like an unnecessary restriction that we might want to change in the future (esp for very long queries). Should we consider not having this restriction as part of the interface?
Can we say anything about the order of the series we return?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's something that the other end can rely on for efficiency, and if there's a new TSDB implementation at some point where a different way makes sense we can always have a new type covering it. For now I'd say let users depend on this, rather than sacrificing this to an unknown future change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. I can agree with @tomwilkie that it will change soon and then we will need to do deprecation process or different type which might be painful, but without that restriction as @brian-brazil says callers cannot assume that useful invariant and gain benefits which would have bad conequences on Prometheus PromQL current shape.
This means that I think I am bit more on restricting it as @brian-brazil suggests, but maybe it makes sense then to discuss / be prepared / have an idea how we will do it if we will need to change that restriction.
Maybe leave the current shape and once new restriction come in, add some enum SeriesOrder
to ReadRequest
? In theory we can do that gradually once the different restriction arise? And keep it YAGNI for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not seeing how we'd be changing this, and we can always extend the proto.
// ChunkedSeries represents single, encoded time series. | ||
message ChunkedSeries { | ||
repeated Label labels = 1 [(gogoproto.nullable) = false]; | ||
repeated Chunk chunks = 2 [(gogoproto.nullable) = false]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything to be said for the order of the chunks we return? Will they overlap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I think the Prometheus server implementation will not give overlaps, but not sure if all known caller implementation assumes. I think both Prometheus and Thanos handles overlaps well.
In terms of sorting 🤷♂️ need to double check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say that they shouldn't overlay, and will be sorted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented.
|
||
// ChunkedSeries represents single, encoded time series. | ||
message ChunkedSeries { | ||
repeated Label labels = 1 [(gogoproto.nullable) = false]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tried elsewhere in the remote interfaces to guarantee ordering of the labels IIRC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. Also the in most other places in TSDB. Happy to add such comment and check implementation on later chained PRs.
However, what do you think about having more complex logic - if we send consequtive chunks for exactly the same series in multiple frames it is enough to send labels just on first one, first? But might be optimization we can add later (which will be another interface change ;p)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'd require a bit more state on the reader side for a pretty small saving, I'd keep it simple and always include it.
Addressed comments @tomwilkie @brian-brazil |
…response. Context: https://docs.google.com/document/d/1JqrU3NjM9HoGLSTPYOvR217f5HBKBiJTqikEB9UiJL0/edit# Signed-off-by: Bartek Plotka <[email protected]>
Signed-off-by: Bartek Plotka <[email protected]>
Signed-off-by: Bartek Plotka <[email protected]>
Signed-off-by: Bartek Plotka <[email protected]>
Signed-off-by: Bartek Plotka <[email protected]>
Signed-off-by: Bartek Plotka <[email protected]>
Signed-off-by: Bartek Plotka <[email protected]>
Closing -> the squashed commits are now part of this PR: #5703 |
First part of chained PRs to add streamed, chunked remote read.
Part of: #4517 and thanos-io/thanos#488
Notes for reviewers:
To fully understand decisions and how usage will look like feel free to see all changes together here.
.. and caller side changes (Thanos): thanos-io/thanos#1268
I thought it will be much easier to discuss changes in much smaller chunks.
PTAL @brian-brazil @tomwilkie @gouthamve @krasi-georgiev and others. Feedback very welcome.
Concerns from my side:
streaming
encoding
andcompression
in three different enums /ReadRequest
parameters? It will be awful on implementation site and user will expect every permutation to work. That's why I left only two options:SAMPLES
andSTREAMED_XOR_CHUNKS
Initial benchmarks are available here: #5703
More details on some decisions: https://docs.google.com/document/d/1JqrU3NjM9HoGLSTPYOvR217f5HBKBiJTqikEB9UiJL0/edit#
Signed-off-by: Bartek Plotka [email protected]