feat(stream): add dedicated metrics for sync log store #20907

kwannoel · 2025-03-14T03:08:19Z

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 test labels as necessary.
I have added fuzzing tests or opened an issue to track them.
My PR contains breaking changes.
My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
My PR contains critical fixes that are necessary to be merged into the latest release.

Documentation

My PR needs documentation updates.

Release note

kwannoel · 2025-03-14T03:08:42Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

wenym1

Can you include a screenshot of the grafana dashboard applied with this PR?

src/stream/src/executor/monitor/streaming_stats.rs

kwannoel · 2025-03-25T15:15:32Z

src/stream/src/executor/sync_kv_log_store.rs

wenym1 · 2025-03-27T11:56:04Z

src/stream/src/executor/sync_kv_log_store.rs

+        /// `target`: refers to the target of the log store,
+        ///           for instance `MySql` Sink, PG sink, etc...
+        ///           or unaligned join.
+        pub(crate) fn new(


I think metrics here should only be used by synced kv log store, and should have a fixed target? For metrics not shared with unsynced kv log store, we don't even need the target label.

It can be used by decoupled sink into table.

How are we going to specify the target in the future when we have multiple usages on synced kv log store executor? From the current stream node, we don't have field to store the target yet.

We can add it in the stream node later, when we use it for decoupled sink.

wenym1 · 2025-03-27T11:57:49Z

grafana/dashboard/dev/sync_logstore_metrics.py

+                    "",
+                    [
+                        panels.target(
+                            f"sum({metric('sync_kv_log_store_state')}) by (type, fragment_id, relation)",


We should use rate to measure the rate of transition, instead of the counter value itself.

Or if we want to monitor the current state rather than the transition rate, we may use gauge rather than counter.

I prefer to store 2 counters, and subtract them from each other. If we directly use gauge, if the state transitions to dirty, and in a short instant quickly changes to clean, but only does so occasionally, it might be missed when prometheus collects metrics. This is because prometheus only collects at some fixed interval. The metric value will just stay at 0. On the other hand, if I collect both clean and dirty, I'm able to differentiate this, since I can check the clean and dirty state transitions.

I will hide the first panel in this group, and only expose the current state metric panel.

I see. If so, I think we can replace the sum in the first panel with rate, and unhide it to shows the transition rate into the two states.

grafana/dashboard/dev/sync_logstore_metrics.py

src/stream/src/executor/sync_kv_log_store.rs

grafana/dashboard/dev/sync_logstore_metrics.py

wenym1

Can you share a screenshot on grafana of the latest code?

src/stream/src/executor/sync_kv_log_store.rs

wenym1 · 2025-03-28T12:45:34Z

src/stream/src/executor/sync_kv_log_store.rs

                                            yield Message::Barrier(barrier);
+                                            self.metrics


In current implementation we have to carefully ensure that we do this measurement in every yield point. We may make this logic of measuring back-pressure to be more general and simpler.

It can be a wrapper over any inner stream. When it implement Stream, it start a timer every time its poll_next returns ready, and record the time elpased between its next call on poll_next.

wenym1 · 2025-03-28T12:48:17Z

src/stream/src/executor/sync_kv_log_store.rs

+                                tracing::trace!("resuming paused future");
+                                if let Some(sleep_future) = sleep_future {
+                                    let deadline = sleep_future.deadline();
+                                    let now = Instant::now();


The check here seems unnecessary. Neither the warning log nor the trace log seems to provide notable information.

wenym1 · 2025-03-28T12:54:08Z

grafana/dashboard/dev/sync_logstore_metrics.py

+                    "",
+                    [
+                        panels.target(
+                            f"sum({metric('sync_kv_log_store_state')}) by (type, fragment_id, relation)",


I see. If so, I think we can replace the sum in the first panel with rate, and unhide it to shows the transition rate into the two states.

kwannoel · 2025-03-28T16:27:28Z

wenym1

Rest LGTM.

wenym1 · 2025-04-01T06:50:17Z

src/stream/src/executor/sync_kv_log_store.rs

@@ -113,12 +113,12 @@ pub mod metrics {
        // state of the log store
        pub unclean_state: LabelGuardedIntCounter<5>,
        pub clean_state: LabelGuardedIntCounter<5>,
-        pub wait_next_poll_ns: LabelGuardedIntCounter<4>,
+        pub wait_next_poll_ns: Option<LabelGuardedIntCounter<4>>, // Allow us to take it later.


Can clone it for simplicity when we use it.

wenym1 · 2025-04-01T06:51:38Z

src/stream/src/executor/sync_kv_log_store.rs

@@ -427,10 +430,10 @@ impl<S: LocalStateStore> WriteFuture<S> {
        stream: BoxedMessageStream,
        write_state: LogStoreWriteState<S>,
    ) -> Self {
-        let instant = Instant::now() + duration;
-        tracing::trace!(?instant, ?duration, "write_future_pause");
+        tracing::trace!(now = ?Instant::now(), ?duration, "write_future_pause");


Seems redundant to call Instant::now() in consecutively 3 lines. Can call once and reuse later.

wenym1 · 2025-04-01T06:56:04Z

src/stream/src/executor/sync_kv_log_store.rs

+        /// `target`: refers to the target of the log store,
+        ///           for instance `MySql` Sink, PG sink, etc...
+        ///           or unaligned join.
+        pub(crate) fn new(


How are we going to specify the target in the future when we have multiple usages on synced kv log store executor? From the current stream node, we don't have field to store the target yet.

Co-authored-by: William Wen <[email protected]>

kwannoel mentioned this pull request Mar 14, 2025

feat(stream): limit sync log store flushed item read size + only pause on checkpoint barrier #20908

Merged

8 tasks

kwannoel changed the title ~~record more metrics~~ feat(stream): record more metrics Mar 14, 2025

kwannoel changed the title ~~feat(stream): record more metrics~~ feat(stream): add dedicated metrics for sync log store Mar 14, 2025

kwannoel force-pushed the kwannoel/sync-log-store-metrics branch 2 times, most recently from 3fd1c42 to 9315111 Compare March 17, 2025 09:59

github-actions bot added the type/feature label Mar 17, 2025

kwannoel force-pushed the kwannoel/sync-log-store-metrics branch 2 times, most recently from 3f41c4e to 0cbff0f Compare March 21, 2025 18:16

kwannoel marked this pull request as ready for review March 22, 2025 04:19

kwannoel requested review from wenym1 and chenzl25 March 22, 2025 04:20

wenym1 reviewed Mar 24, 2025

View reviewed changes

src/stream/src/executor/monitor/streaming_stats.rs Outdated Show resolved Hide resolved

kwannoel force-pushed the kwannoel/sync-log-store-metrics branch from 0cbff0f to 83e5903 Compare March 25, 2025 13:39

kwannoel force-pushed the kwannoel/sync-log-store-metrics branch 2 times, most recently from 32698a4 to fc13ccd Compare March 25, 2025 15:18

kwannoel requested a review from wenym1 March 27, 2025 04:42

kwannoel force-pushed the kwannoel/sync-log-store-metrics branch from fc13ccd to 8d14284 Compare March 27, 2025 04:43

wenym1 reviewed Mar 27, 2025

View reviewed changes

kwannoel force-pushed the kwannoel/sync-log-store-metrics branch 2 times, most recently from 003ed21 to 34ad60a Compare March 28, 2025 06:30

kwannoel requested a review from wenym1 March 28, 2025 09:29

wenym1 reviewed Mar 28, 2025

View reviewed changes

kwannoel force-pushed the kwannoel/sync-log-store-metrics branch from 16c7e1e to 96449c2 Compare March 28, 2025 15:31

kwannoel requested a review from wenym1 April 1, 2025 02:12

kwannoel force-pushed the kwannoel/sync-log-store-metrics branch from 7152be4 to 7a074cf Compare April 1, 2025 06:26

wenym1 approved these changes Apr 1, 2025

View reviewed changes

record more metrics

f0ff784

kwannoel and others added 17 commits April 1, 2025 16:16

decouple metrics

7b0a51c

record unclean state metrics

13238d2

update dashboards

1e93f13

update dashboard

f07cfe0

merge common metrics

248b0e8

update dashboard

114fbc5

fix clean/dirty

c26fb76

address review comments

7c85f45

rename

dc1addb

improve pause measurement

ce6aada

Update src/stream/src/executor/sync_kv_log_store.rs

1e244bb

Co-authored-by: William Wen <[email protected]>

cleanup

8fbe992

wrap backpressure monitoring

7fe151c

use rate

9026773

fix metrics

340a3a7

fix clean/dirty state metric legend

cd80bc9

clone wait_next_poll + reuse Instant::now()

364fba8

kwannoel force-pushed the kwannoel/sync-log-store-metrics branch from c064c27 to 364fba8 Compare April 1, 2025 08:20

kwannoel added this pull request to the merge queue Apr 1, 2025

Merged via the queue into main with commit 7f11530 Apr 1, 2025
28 of 29 checks passed

kwannoel deleted the kwannoel/sync-log-store-metrics branch April 1, 2025 11:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stream): add dedicated metrics for sync log store #20907

feat(stream): add dedicated metrics for sync log store #20907

kwannoel commented Mar 14, 2025

kwannoel commented Mar 14, 2025 •

edited

Loading

wenym1 left a comment

kwannoel commented Mar 25, 2025

wenym1 Mar 27, 2025

kwannoel Mar 27, 2025

wenym1 Apr 1, 2025

kwannoel Apr 1, 2025

wenym1 Mar 27, 2025

wenym1 Mar 27, 2025

kwannoel Mar 28, 2025 •

edited

Loading

wenym1 Mar 28, 2025

kwannoel Mar 28, 2025

wenym1 left a comment

wenym1 Mar 28, 2025

kwannoel Mar 28, 2025

wenym1 Mar 28, 2025

kwannoel Mar 28, 2025

wenym1 Mar 28, 2025

kwannoel commented Mar 28, 2025

wenym1 left a comment

wenym1 Apr 1, 2025

wenym1 Apr 1, 2025

wenym1 Apr 1, 2025

feat(stream): add dedicated metrics for sync log store #20907

feat(stream): add dedicated metrics for sync log store #20907

Conversation

kwannoel commented Mar 14, 2025

What's changed and what's your intention?

Checklist

Documentation

kwannoel commented Mar 14, 2025 • edited Loading

wenym1 left a comment

Choose a reason for hiding this comment

kwannoel commented Mar 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwannoel Mar 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenym1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwannoel commented Mar 28, 2025

wenym1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwannoel commented Mar 14, 2025 •

edited

Loading

kwannoel Mar 28, 2025 •

edited

Loading