revert: remove enable_stream_row_count config #10261 #11328

lmatz · 2023-07-31T09:32:02Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
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 contains user-facing changes.

Click here for Documentation

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

Installation and deployment
Connector (sources & sinks)
SQL commands, functions, and operators
RisingWave cluster configuration changes
Other (please specify in the release note below)

Release note

BugenZhao · 2023-08-01T03:50:06Z

Interesting. May I ask for the motivation?

lmatz · 2023-08-01T04:05:29Z

With 500+ MV on a single machine, one user's Prometheus takes 16GB.
And Grafana just cannot show most of the metrics, always loading

lmatz · 2023-08-01T04:08:02Z

I am thinking just giving MV and sinks two dedicated metrics, or judging actor_id_string or other similar things to enable row_executor_count just for MV and sinks.

lmatz · 2023-08-17T13:49:02Z

Added a sink_info metric into Prom so that we can get the actor_id -> sink_name when building the panel.

Sink and MV still always get the metrics. Other executors do not, but it is configurable.

@BugenZhao Please take another look after and only after you come back from the vacation

codecov · 2023-08-17T15:22:29Z

Codecov Report

Merging #11328 (21933ff) into main (0874a48) will decrease coverage by 0.03%.
Report is 2 commits behind head on main.
The diff coverage is 27.27%.

@@            Coverage Diff             @@
##             main   #11328      +/-   ##
==========================================
- Coverage   70.28%   70.26%   -0.03%     
==========================================
  Files        1366     1366              
  Lines      228417   228458      +41     
==========================================
- Hits       160541   160515      -26     
- Misses      67876    67943      +67

Flag	Coverage Δ
rust	`70.26% <27.27%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/stream/src/executor/wrapper.rs	`0.00% <0.00%> (ø)`
src/stream/src/executor/wrapper/trace.rs	`0.00% <ø> (ø)`
src/stream/src/task/stream_manager.rs	`1.74% <0.00%> (-0.01%)`	⬇️
src/meta/src/rpc/metrics.rs	`77.06% <45.00%> (-1.02%)`	⬇️
src/common/src/config.rs	`83.11% <100.00%> (+0.09%)`	⬆️

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

BugenZhao

LGTM. PTAL @hzxa21 🥰

BugenZhao · 2023-08-18T03:15:57Z

src/stream/src/executor/wrapper/trace.rs

@@ -38,14 +39,16 @@ pub async fn trace(

    let span_name = pretty_identity(&info.identity, actor_id, executor_id);

+    let is_sink_or_mv = info.identity.contains("Materialize") || info.identity.contains("Sink");


This seems fragile but I'm unsure if there's a better way.

let me open an issue to track this, a quick fix for Kaito

+1. It seems a little bit hacky here.

hzxa21 · 2023-08-18T03:45:10Z

src/stream/src/executor/wrapper/trace.rs

@@ -38,14 +39,16 @@ pub async fn trace(

    let span_name = pretty_identity(&info.identity, actor_id, executor_id);

+    let is_sink_or_mv = info.identity.contains("Materialize") || info.identity.contains("Sink");


+1. It seems a little bit hacky here.

zwang28 · 2023-08-18T05:35:41Z

After this PR, the resulted file size can still be as large as 400MB.
curl -v http://127.0.0.1:[cn prometheus port]/api/v1/query

~~After this PR, the resulted file size reduces from 600MB to 100MB:~~
~~Though it's still large, at least grafana can present it now~~.

The file is too large to attach here.

zwang28 · 2023-08-18T05:59:15Z

Storage metric takes up 80%.

zzzzz:~ z28wang$ grep "state_store" cn.prom_query.2.log |wc -l
 4236008
zzzzz:~ z28wang$ grep "actor" cn.prom_query.2.log | wc -l
 1572093
zzzzz:~ z28wang$ grep "stream" cn.prom_query.2.log |wc -l
 1435195
zzzzz:~ z28wang$ cat cn.prom_query.2.log | wc -l
 5821215
zzzzz:~ z28wang$ du -h cn.prom_query.2.log
464M	cn.prom_query.2.log

lmatz requested review from neverchanje and BugenZhao July 31, 2023 09:32

github-actions bot added the type/revert label Jul 31, 2023

lmatz force-pushed the lz/revert branch from 0aef8bc to 1509d2c Compare August 17, 2023 13:44

lmatz force-pushed the lz/revert branch from 1509d2c to 3f75af1 Compare August 17, 2023 13:52

lmatz requested a review from zwang28 August 17, 2023 13:52

revert: remove enable_stream_row_count config #10261

21933ff

lmatz force-pushed the lz/revert branch from 3f75af1 to 21933ff Compare August 17, 2023 14:15

BugenZhao approved these changes Aug 18, 2023

View reviewed changes

zwang28 approved these changes Aug 18, 2023

View reviewed changes

lmatz added this pull request to the merge queue Aug 18, 2023

hzxa21 approved these changes Aug 18, 2023

View reviewed changes

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 18, 2023

lmatz added this pull request to the merge queue Aug 18, 2023

zwang28 added the need-cherry-pick-v1.1 label Aug 18, 2023

Merged via the queue into main with commit 4c04b9e Aug 18, 2023

lmatz deleted the lz/revert branch August 18, 2023 04:27

github-actions bot mentioned this pull request Aug 18, 2023

cherrypick revert: remove enable_stream_row_count config #10261 (#11328) to branch v1.1-rc #11770

Closed

zwang28 mentioned this pull request Aug 22, 2023

refactor: slim down storage metric size #11822

Closed

Li0k pushed a commit that referenced this pull request Sep 15, 2023

revert: remove enable_stream_row_count config #10261 (#11328)

607ad97

fuyufjh mentioned this pull request Oct 4, 2023

feat: improve sink mview throughput metrics #12622

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

revert: remove enable_stream_row_count config #10261 #11328

revert: remove enable_stream_row_count config #10261 #11328

lmatz commented Jul 31, 2023

BugenZhao commented Aug 1, 2023

lmatz commented Aug 1, 2023 •

edited

Loading

lmatz commented Aug 1, 2023

lmatz commented Aug 17, 2023 •

edited

Loading

codecov bot commented Aug 17, 2023

BugenZhao left a comment

BugenZhao Aug 18, 2023

lmatz Aug 18, 2023

hzxa21 Aug 18, 2023

hzxa21 Aug 18, 2023

zwang28 commented Aug 18, 2023 •

edited

Loading

zwang28 commented Aug 18, 2023 •

edited

Loading

		@@ -38,14 +39,16 @@ pub async fn trace(

		let span_name = pretty_identity(&info.identity, actor_id, executor_id);

		let is_sink_or_mv = info.identity.contains("Materialize") \|\| info.identity.contains("Sink");

revert: remove enable_stream_row_count config #10261 #11328

revert: remove enable_stream_row_count config #10261 #11328

Conversation

lmatz commented Jul 31, 2023

What's changed and what's your intention?

Checklist

Documentation

Types of user-facing changes

Release note

BugenZhao commented Aug 1, 2023

lmatz commented Aug 1, 2023 • edited Loading

lmatz commented Aug 1, 2023

lmatz commented Aug 17, 2023 • edited Loading

codecov bot commented Aug 17, 2023

Codecov Report

BugenZhao left a comment

Choose a reason for hiding this comment

BugenZhao Aug 18, 2023

Choose a reason for hiding this comment

lmatz Aug 18, 2023

Choose a reason for hiding this comment

hzxa21 Aug 18, 2023

Choose a reason for hiding this comment

hzxa21 Aug 18, 2023

Choose a reason for hiding this comment

zwang28 commented Aug 18, 2023 • edited Loading

zwang28 commented Aug 18, 2023 • edited Loading

lmatz commented Aug 1, 2023 •

edited

Loading

lmatz commented Aug 17, 2023 •

edited

Loading

zwang28 commented Aug 18, 2023 •

edited

Loading

zwang28 commented Aug 18, 2023 •

edited

Loading