feat(dashboard): add back_pressure_rate UI #14863

yufansong · 2024-01-30T06:57:41Z

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 plan to add a UI page to show page pressure rate without using prometheus. We already add the API in meta to get the current back pressure. In this PR

Add the calculation function to calculatethe bp rate from bp. The interval is 20s. This is a rough calculation. Only call API for every 20 seconds and then divided the 20s when calculate the rate.
Integrate the rate into the Fragement Graph. If select the current, will shift from prometheus data into this API.
What we get from meta node is actor-id, fragement id, next fragement id back pressure. We convert the back pressure into back pressure rate, and also aggregate by fragement id, next fragement id on actor id. Then apply the value in streaming graph.
The reason why not impelement p50, p99 for it: we need to storage and maintain a structure in frontend for back pressure data. It will occupied memory in meta node. If data become large, it will be bad.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

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.

fuyufjh · 2024-01-30T07:15:48Z

Hmmm, for the UI part, this is duplicated with #14446 (which was a refactor on #11696).

#14446 looks more user-friendly. Can we adapt to that? For example, add a GUI switch or url query paramter to allow it to fetch data from Meta instead of Promethues.

yufansong · 2024-01-31T05:13:16Z

Hmmm, for the UI part, this is duplicated with #14446 (which was a refactor on #11696).

#14446 looks more user-friendly. Can we adapt to that? For example, add a GUI switch or url query paramter to allow it to fetch data from Meta instead of Promethues.

I add a GUI switch for it. If selecct current, will change to the data from this new API.

fuyufjh · 2024-02-01T03:49:49Z

dashboard/pages/fragment_graph.tsx

+    }
+    doFetch()
+    return () => { }
+  }, [toast])


The second argument of useEffect - dependencies, tells React to re-run the function once any value of it is changed. Here you pass a toast? Seems to be a misunderstanding

I follow the code style in the codebase.

risingwave/dashboard/components/BackPressureTable.tsx

Line 59 in 4500a08

toast(e, "warning")

risingwave/dashboard/pages/cluster.tsx

Line 173 in 4500a08

toast(e, "warning")

I know it will trigger re-run. But in current logic, no parameters suitable for here. So I just put the toast.

Oh, do some more investigation. Maybe this is the problem why there is a mismatch on rate between my impelmentation and the previous impelementation. Will remove the toast. This will result multiple thread keep call and calculate the back pressure. Then the interval of the call changed. The final rate become inaccurate.

You can run npm run lint and the linter will help to fix that. Generally all outside variables captured by the closure should be included.

https://github.com/risingwavelabs/risingwave/compare/eric/no_toast

I tried removing it from dependencies and everything seemed to work. cc. @BugenZhao Can you also take a try?

I already tried to remove toast here, everything still seems to works.

You can run npm run lint and the linter will help to fix that. Generally all outside variables captured by the closure should be included.

I tried npm run lint, and it will remind me to add the dependencies previousBP. I applied the suggestion, but it will cause the function do infinite quickly re-trigger which is not we want here.
So in current logic, the previousBP are not suitable to put into dependencies.

NVM, find a way to refactor it.

fuyufjh · 2024-02-01T04:13:13Z

dashboard/pages/fragment_graph.tsx

@@ -168,8 +169,8 @@ function buildFragmentDependencyAsEdges(

 const SIDEBAR_WIDTH = 200

-type BackPressureAlgo = "p50" | "p90" | "p95" | "p99"
-const backPressureAlgos: BackPressureAlgo[] = ["p50", "p90", "p95", "p99"]
+type BackPressureAlgo = "p50" | "p90" | "p95" | "p99" | "current"


Recommend to make current the default option. Previously we use Disabled by default because Prometheus is not always available, but now current don't depend on it.

Also, it seems too obscure to users that p50/90/99 is from Prometheus while current is from Meta.

A better way is probably making two ListBox: 1) Data source: Embedded or Prometheus 2) Only shows for Prometheus: average/p50/p99

fuyufjh · 2024-02-01T04:14:57Z

I tried locally. The result of Prometheus and the embedded metrics were so different. (I have waited for 1 min before capturing the screenshot). Does it really work? 🤔

current:

p50:

yufansong · 2024-02-01T04:19:44Z

I tried locally. The result of Prometheus and the embedded metrics were so different. (I have waited for 1 min before capturing the screenshot). Does it really work? 🤔

Let me try some large data test locally. Only try some small data before.

dashboard/pages/fragment_graph.tsx

dashboard/components/BackPressureTableWithoutPrometheus.tsx

dashboard/mock-server.js

dashboard/pages/api/metric.ts

yufansong · 2024-02-02T09:45:43Z

@fuyufjh @BugenZhao
Do the following changes:

Change the interval to 5s.
Remove useless code.
Change the UI, add a new list box. Default is Embedded. When selected, on option will show on Back Pressure. When select Prometheus, will show disable | pxx. The reason why keep disable is to remind user select pxx. Otherwise at the beginning, the Ui will default show xx selected but value actually is NULL

Refactor the code logic to solve the closure dependencies problem. Already pass lint check.

Fix the BP rate calculation bug. Want to check one thing, in the previous BP API, we average the rate across the actor id. Should we use the AVG or SUM on actor id? Curerntly I do the calculation the same with the AVG logic.

risingwave/src/meta/src/dashboard/prometheus.rs

Line 139 in 8fdd0ba

    
           format!("avg(rate(stream_actor_output_buffer_blocking_duration_ns{{{}}}[60s])) by (fragment_id, downstream_fragment_id) / 1000000000", srv.prometheus_selector);

Do some test between new api and old api. Also welcome more extra checks. The first page show the accurate value from the new API {'2_1' => 0.0000726248, '3_1' => 0.0000513166} which means fragement 2 -> fragement 1 = 0.0000726248 and fragement 3 -> fragement 1 = 0.0000513166 .

We should not compared with the pxx because these value already did some calculation. I directly check the prometheus, their value has some gap, but I think the majority is similiar and can be accepted :

dashboard/pages/fragment_graph.tsx

Co-authored-by: Bugen Zhao <[email protected]>

fuyufjh · 2024-02-21T04:57:25Z

5. Fix the BP rate calculation bug. Want to check one thing, in the previous BP API, we average the rate across the actor id. Should we use the AVG or SUM on actor id? Curerntly I do the calculation the same with the AVG logic.

Are you asking that whether we should show back-pressure on actor-level or fragment-level granularity? I think actor-level information is useless and too verbose in most cases. The only cases I can think of is inbalance i.e. data skew, this happens sometimes, so it would be nice if we allow users to click to dive into actor-level information in the dashabord.

gitguardian · 2024-02-21T05:06:46Z

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
9425213	Triggered	Generic Password	`fbc0e59`	ci/scripts/regress-test.sh	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secret safely. Learn here the best practices.
Revoke and rotate this secret.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!}

yufansong · 2024-02-21T06:03:43Z

Fix the BP rate calculation bug. Want to check one thing, in the previous BP API, we average the rate across the actor id. Should we use the AVG or SUM on actor id? Curerntly I do the calculation the same with the AVG logic.

Are you asking that whether we should show back-pressure on actor-level or fragment-level granularity? I think actor-level information is useless and too verbose in most cases. The only cases I can think of is inbalance i.e. data skew, this happens sometimes, so it would be nice if we allow users to click to dive into actor-level information in the dashabord.

Oh, I mean this, in old api, it directly use the following code to get metrics from prometheus:

format!("avg(rate(stream_actor_output_buffer_blocking_duration_ns{{{}}}[60s])) by (fragment_id, downstream_fragment_id) / 1000000000", srv.prometheus_selector);

My understand is it will do average on actor id, and group by fragment_id, downstream_fragment_id. I implememnt the new api following this old logic. Just want to confirm Average logic is what we want.

dashboard/pages/api/metric.ts

yufansong · 2024-02-22T20:10:14Z

Fix the bug. In previous time, the back pressure rate calculation is correct. But when drawing graph in frontend, there is a bug, forget to convert the back pressure rate into the value used for drawing. Here is the example of embeded and prometheus under the large workload:

dashboard/pages/fragment_graph.tsx

yufansong · 2024-02-23T07:34:22Z

BTW, I find some compile problem when marge the latest main branch. I find the problem also exist in the latest main branch which not include my PR. So it should not introduced in this PR. Record it in #15219 , I guess it should related to the recent merged PRs in past 1 month.

fuyufjh

LGTM for the rest

dashboard/pages/api/metric.ts

Co-authored-by: Eric Fu <[email protected]>

fuyufjh · 2024-02-26T06:56:24Z

dashboard/pages/api/metric.ts

+
+// Get back pressure from meta node -> compute node
+export async function getBackPressureWithoutPrometheus() {
+  const response = await api.get("/metrics/back_pressures")


Shall we move it to another path such as "embedded_metrics" or sth? It's mixed with the metrics fetched from Prometheus, which looks confusing to me.

risingwave/src/meta/src/dashboard/mod.rs

Lines 423 to 428 in ae7d7ba

.route("/metrics/cluster", get(prometheus::list_prometheus_cluster))

.route(

"/metrics/actor/back_pressures",

get(prometheus::list_prometheus_actor_back_pressure),

)

.route("/metrics/back_pressures", get(get_back_pressure))

Btw, /metrics/actor/back_pressures actually output fragment-level metrics instead of actor-level... 😂 Legacy issues

Will solve the problem in a seperate PR

yufansong added 2 commits January 25, 2024 20:02

add frontend test code

bd213e5

Merge branch 'main' into yufan/dashboard-frontend

2f1a053

github-actions bot added the type/feature label Jan 30, 2024

yufansong requested review from yezizp2012, BugenZhao and fuyufjh January 30, 2024 07:10

yufansong force-pushed the yufan/dashboard-frontend branch from 73d9ad4 to 2772601 Compare January 31, 2024 04:47

refactor frontend code, adopt to existing streaming graph

9ae89d7

yufansong force-pushed the yufan/dashboard-frontend branch from b6e6f83 to 9ae89d7 Compare January 31, 2024 05:11

fuyufjh reviewed Feb 1, 2024

View reviewed changes

BugenZhao reviewed Feb 1, 2024

View reviewed changes

dashboard/pages/fragment_graph.tsx Show resolved Hide resolved

dashboard/components/BackPressureTableWithoutPrometheus.tsx Outdated Show resolved Hide resolved

dashboard/mock-server.js Outdated Show resolved Hide resolved

dashboard/pages/api/metric.ts Outdated Show resolved Hide resolved

yufansong added 2 commits February 2, 2024 01:46

apply suggestions

62c0bea

fix nits

1cb5a83

BugenZhao reviewed Feb 5, 2024

View reviewed changes

yufansong and others added 5 commits February 5, 2024 15:35

Update dashboard/pages/fragment_graph.tsx

28b20bf

Co-authored-by: Bugen Zhao <[email protected]>

Update dashboard/pages/fragment_graph.tsx

c6b3073

Co-authored-by: Bugen Zhao <[email protected]>

Update dashboard/pages/fragment_graph.tsx

4e0138e

Co-authored-by: Bugen Zhao <[email protected]>

change select box logic, fix nits format

eec1389

fix select box default showing

37b4b93

yufansong requested review from fuyufjh and BugenZhao February 20, 2024 08:55

Merge remote-tracking branch 'origin/main' into yufan/dashboard-frontend

fbc0e59

fuyufjh reviewed Feb 21, 2024

View reviewed changes

dashboard/pages/api/metric.ts Outdated Show resolved Hide resolved

dashboard/pages/api/metric.ts Outdated Show resolved Hide resolved

dashboard/pages/api/metric.ts Show resolved Hide resolved

solve *100 bug and refactor

67480d7

yufansong added 2 commits February 22, 2024 12:18

fix bug

acfe6ad

reduce useless call

5ab8509

BugenZhao reviewed Feb 23, 2024

View reviewed changes

dashboard/pages/fragment_graph.tsx Outdated Show resolved Hide resolved

refactor

8d96ac2

yufansong requested review from BugenZhao and fuyufjh February 26, 2024 05:16

Merge branch 'main' into yufan/dashboard-frontend

ae7d7ba

fuyufjh approved these changes Feb 26, 2024

View reviewed changes

dashboard/pages/api/metric.ts Outdated Show resolved Hide resolved

Update dashboard/pages/api/metric.ts

0e2b6a4

Co-authored-by: Eric Fu <[email protected]>

fuyufjh reviewed Feb 26, 2024

View reviewed changes

yufansong added this pull request to the merge queue Feb 26, 2024

Merged via the queue into main with commit 5f185d9 Feb 26, 2024
7 of 9 checks passed

yufansong deleted the yufan/dashboard-frontend branch February 26, 2024 20:14

yufansong mentioned this pull request Feb 26, 2024

refactor(dashboard): refactor back pressure api path name #15277

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dashboard): add back_pressure_rate UI #14863

feat(dashboard): add back_pressure_rate UI #14863

yufansong commented Jan 30, 2024 •

edited

Loading

fuyufjh commented Jan 30, 2024

yufansong commented Jan 31, 2024

fuyufjh Feb 1, 2024

yufansong Feb 2, 2024

yufansong Feb 2, 2024 •

edited

Loading

BugenZhao Feb 2, 2024 •

edited

Loading

fuyufjh Feb 2, 2024

yufansong Feb 2, 2024 •

edited

Loading

fuyufjh Feb 1, 2024 •

edited

Loading

fuyufjh Feb 1, 2024 •

edited

Loading

fuyufjh commented Feb 1, 2024

yufansong commented Feb 1, 2024 •

edited

Loading

yufansong commented Feb 2, 2024 •

edited

Loading

fuyufjh commented Feb 21, 2024

gitguardian bot commented Feb 21, 2024 •

edited

Loading

yufansong commented Feb 21, 2024 •

edited

Loading

yufansong commented Feb 22, 2024

yufansong commented Feb 23, 2024 •

edited

Loading

fuyufjh left a comment

fuyufjh Feb 26, 2024 •

edited

Loading

fuyufjh Feb 26, 2024

yufansong Feb 26, 2024

	.route("/metrics/cluster", get(prometheus::list_prometheus_cluster))
	.route(
	"/metrics/actor/back_pressures",
	get(prometheus::list_prometheus_actor_back_pressure),
	)
	.route("/metrics/back_pressures", get(get_back_pressure))

feat(dashboard): add back_pressure_rate UI #14863

feat(dashboard): add back_pressure_rate UI #14863

Conversation

yufansong commented Jan 30, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

fuyufjh commented Jan 30, 2024

yufansong commented Jan 31, 2024

fuyufjh Feb 1, 2024

Choose a reason for hiding this comment

yufansong Feb 2, 2024

Choose a reason for hiding this comment

yufansong Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

BugenZhao Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

fuyufjh Feb 2, 2024

Choose a reason for hiding this comment

yufansong Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

fuyufjh Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

fuyufjh Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

fuyufjh commented Feb 1, 2024

yufansong commented Feb 1, 2024 • edited Loading

yufansong commented Feb 2, 2024 • edited Loading

fuyufjh commented Feb 21, 2024

gitguardian bot commented Feb 21, 2024 • edited Loading

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

yufansong commented Feb 21, 2024 • edited Loading

yufansong commented Feb 22, 2024

yufansong commented Feb 23, 2024 • edited Loading

fuyufjh left a comment

Choose a reason for hiding this comment

fuyufjh Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

fuyufjh Feb 26, 2024

Choose a reason for hiding this comment

yufansong Feb 26, 2024

Choose a reason for hiding this comment

yufansong commented Jan 30, 2024 •

edited

Loading

yufansong Feb 2, 2024 •

edited

Loading

BugenZhao Feb 2, 2024 •

edited

Loading

yufansong Feb 2, 2024 •

edited

Loading

fuyufjh Feb 1, 2024 •

edited

Loading

fuyufjh Feb 1, 2024 •

edited

Loading

yufansong commented Feb 1, 2024 •

edited

Loading

yufansong commented Feb 2, 2024 •

edited

Loading

gitguardian bot commented Feb 21, 2024 •

edited

Loading

yufansong commented Feb 21, 2024 •

edited

Loading

yufansong commented Feb 23, 2024 •

edited

Loading

fuyufjh Feb 26, 2024 •

edited

Loading