-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Query: adding stats to the remote engine #7361
Query: adding stats to the remote engine #7361
Conversation
f1c734e
to
6f0aac1
Compare
bc46991
to
4dbaa42
Compare
There is one test e2e test failing, but cant reproduce locally: (also, it seems to be failing on main for CI)
|
251dc91
to
6614b86
Compare
@GiedriusS and/or @MichaHoffmann do you mind giving a second look here pls? Thx |
397d99f
to
a3924a3
Compare
We are currently losing track of query stats because the remote engine does not transmit performance stats on gRPC calls. In this PR I am adding some fields to the Query API response to include some stats. Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
Signed-off-by: Pedro Tanaka <pedro.tanaka@shopify.com>
a3924a3
to
c79c9ff
Compare
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.
lgtm
if s := msg.GetStats(); s != nil { | ||
qryStats = *s | ||
continue | ||
} | ||
|
||
ts := msg.GetTimeseries() |
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.
since old root queriers dont have the ==nil check ~ this will probably break if leaf queriers are updated to send stats first. One should update root queriers first and then leaf queriers probably.
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 can add this as a note to the CHANGELOG for the distributed engine use case. I don't think many people use this mode yet, but it is good to point out the rollout strategy.
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.
Yeah! I dont think its a big issue ~ just wanted to point it out; We can add a small warning in release notes later
We are currently losing track of query stats because the remote engine does not transmit performance stats on gRPC calls.
In this PR I am adding some fields to the Query API response to include some stats.
Changes
Stats()
method from the prometheus Query interface for theremoteQuery
so we can get stats on the thanos/promql-engine.Verification