-
Notifications
You must be signed in to change notification settings - Fork 591
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
feat(meta): add get back pressure RPC for UI dashboard #14790
Conversation
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.
The rest LGTM
proto/stream_service.proto
Outdated
string actor_id = 1; | ||
string fragment_id = 2; | ||
string downstream_fragment_id = 3; |
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.
The data types can be integers.
We use string in Prometheus because Prometheus require all labels to be string. But sicne this interface is particularly designed for back-pressire (as it's named BackPressureInfo
), it's not that case.
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.
Sorry for my late response.
Extension(srv): Extension<Service>, | ||
) -> Result<Json<GetBackPressureResponse>> { | ||
let back_pressure_infos = match &srv.metadata_manager { | ||
MetadataManager::V1(mgr) => { |
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 list all streaming worker nodes directly here by metadata manager, then we can call the RPC here and avoid to add interface for worker manager and controller.
.unwrap(); | ||
let mut back_pressure_infos: Vec<BackPressureInfo> = Vec::new(); | ||
for node in nodes { | ||
let client = self.env.stream_client_pool().get(&node).await.unwrap(); |
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 perform RPC calls concurrently.
@@ -112,6 +126,7 @@ service StreamService { | |||
rpc InjectBarrier(InjectBarrierRequest) returns (InjectBarrierResponse); | |||
rpc BarrierComplete(BarrierCompleteRequest) returns (BarrierCompleteResponse); | |||
rpc WaitEpochCommit(WaitEpochCommitRequest) returns (WaitEpochCommitResponse); | |||
rpc GetBackPressure(GetBackPressureRequest) returns (GetBackPressureResponse); |
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 believe it's better to put it in MonitorService
.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
In #13830, we want to embedded a back pressure rate metrics for dashbaord UI. In this PR, I add a API to get the back pressure from all compute node to meta node.
But I still need to implement the calculation steps. The way to calculate the rate:
(metrics 2 - metrics 1)/(time2 - time1)
.In meta node, I didn't find a good way to do this crobjob, also not find a good place to storage the previous metrics in memory. May try to implement the logic in ui frontend or some place in meta in following PR.
Result:
If you deploy and request the
http://127.0.0.1:5691/api/metrics/back_pressures
, will get result:Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.