feat: improve sink mview throughput metrics #12622
Conversation
Codecov Report
@@ Coverage Diff @@
## main #12622 +/- ##
==========================================
- Coverage 69.37% 69.31% -0.06%
==========================================
Files 1470 1470
Lines 241241 241233 -8
==========================================
- Hits 167354 167216 -138
- Misses 73887 74017 +130
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 15 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| /// [`WrapperExecutor`] will do some sanity checks and logging for the wrapped executor. | ||
| pub struct WrapperExecutor { | ||
| input: BoxedExecutor, | ||
|
|
||
| extra: ExtraInfo, | ||
| actor_ctx: ActorContextRef, |
There was a problem hiding this comment.
Did some refactoring here: Replaced ExtraInfo with ActorContextRef.
| .streaming_metrics | ||
| .sink_input_row_count | ||
| .with_label_values(&[&sink_id_str, &actor_id_str, &fragment_id_str]) | ||
| .inc_by(c.capacity() as u64); |
There was a problem hiding this comment.
Note that the metrics is actually the input rows of Sink (previously, it's output). This is because we don't have a well definition for the output of sinks yet.
Technically, the input and output rows might be different because of force_append_only etc., but either can be a good reference.
fuyufjh
force-pushed
the
eric/improve_sink_mview_throughput_metrics
branch
from
01d08fe to
7bfa479
Compare
|
Introducing independent metrics for sink and materialized view is the approach taken at the very beginning...... It was argued not good |
Can you tell the reasons? Sorry I didn't know about the discussion. My motivation is that I hope to completely remove (disable) any executor-level metrics. |
|
ok not really bad, I suppose it is an argument for generality: #9874 |
…ew_throughput_metrics
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
This PR is a complete refactor of #11328.
This previous implementation reuses
stream_executor_row_count, but disables this metrics for all executors exceptSinkExecutorandMaterializeExecutor, which is a little bit ugly (also pointed by #11328 (comment)).risingwave-dev-dashboard:
risingwave-user-dashboard:
This PR simply introduces 2 new metrics
stream_sink_input_row_countstream_mview_input_row_countJust as the name says, they count the input rows for sink and materialized views respectively.
Meanwhile,
stream_executor_row_countis completely disabled.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.