mertrics: report region flow #1517

zhangjinpeng87 · 2017-01-14T09:40:34Z

BusyJay · 2017-01-14T11:00:50Z

src/raftstore/store/peer.rs

@@ -1372,6 +1375,7 @@ impl Peer {
        }

        metrics.keys_written += ctx.wb.count() as u64;
+        self.bytes_written += ctx.wb.data_size() as u64;


Why not put it into RaftReadyMetrics？And we may need to count the size of entries written into rocksdb too.

If we put it into RaftReadyMetrics , each region need a variable.

I don't think so. RaftReadyMetrics is stored in Store.

I think it is ok here, we may report this to PD later.

siddontang · 2017-01-14T10:10:10Z

src/raftstore/store/store.rs

+                continue;
+            }
+
+            REGION_BYTES_WRITTEN_HISTOGRAM.observe(peer.bytes_written as f64);


use a local histogram then flush, see rust Prometheus
LocalHistogram

LocalHistogram not export yet.

merged, you can use it now.

siddontang · 2017-01-14T10:10:47Z

src/raftstore/store/metrics.rs

@@ -140,4 +140,11 @@ lazy_static! {
            "tikv_engine_keys_written_count",
            "Count of keys has been written for this interval"
        ).unwrap();
+
+    pub static ref REGION_BYTES_WRITTEN_HISTOGRAM: Histogram =


We can check kv count too

siddontang · 2017-01-15T07:07:49Z

src/raftstore/store/msg.rs

@@ -34,6 +34,7 @@ pub enum Tick {
    SnapGc,
    CompactLockCf,
    ConsistencyCheck,
+    ReportWriteBytes,


it is not only for Bytes now.

siddontang · 2017-01-15T07:10:13Z

src/raftstore/store/config.rs

@@ -113,6 +115,8 @@ pub struct Config {

    // Interval (ms) to check region whether the data is consistent.
    pub consistency_check_tick_interval: u64,
+
+    pub report_write_bytes_interval: u64,


use a short value in test so we can cover this feature.

siddontang · 2017-01-15T08:30:16Z

src/raftstore/store/store.rs

+        };
+    }
+
+    fn on_report_region_flow(&mut self, event_loop: &mut EventLoop<Self>) {


please cover it in test

siddontang

Rest LGTM

siddontang · 2017-01-15T08:30:40Z

PTAL @BusyJay @hhkbp2

hhkbp2 · 2017-01-15T09:55:50Z

LGTM

siddontang · 2017-01-15T12:54:45Z

tests/storage/test_raft_storage.rs

@@ -28,6 +28,7 @@ use super::sync_storage::SyncStorage;

 fn new_raft_storage() -> (Cluster<ServerCluster>, SyncStorage, Context) {
    let mut cluster = new_server_cluster_with_cfs(0, 1, ALL_CFS);
+    cluster.cfg.raft_store.report_region_flow_interval = 100; // 100ms


initialize this in new_store_cfg directly.

siddontang · 2017-01-16T01:09:15Z

PTAL @BusyJay

siddontang · 2017-01-16T01:11:20Z

src/raftstore/store/local_metrics.rs

@@ -23,7 +23,7 @@ pub struct RaftReadyMetrics {
    pub commit: u64,
    pub append: u64,
    pub snapshot: u64,
-    pub keys_written: u64,
+    pub store_keys_written: u64,


written_keys,written_bytes may be better?

/cc @BusyJay @hhkbp2

peer.bytes_written peer.keys_written store.region_bytes_written store. region_keys_written metrics.store_keys_written

The prefix levels look fine to me.

…s-region-write-bytes

zhangjinpeng87 · 2017-01-16T03:29:17Z

PING

BusyJay · 2017-01-16T03:35:48Z

src/raftstore/store/store.rs

+
+    fn on_report_region_flow(&mut self, event_loop: &mut EventLoop<Self>) {
+        for (_, peer) in &mut self.region_peers {
+            if !peer.is_leader() {


If peer steps down, all records will be lost.

This is ok, we don't need exact result.

BusyJay · 2017-01-16T03:36:53Z

src/raftstore/store/metrics.rs

@@ -143,9 +143,23 @@ lazy_static! {
            &["cf"]
        ).unwrap();

-    pub static ref STORE_KEYS_WRITTEN_COUNTER: Counter =
+    pub static ref STORE_WRITTEN_KEYS_COUNTER: Counter =


I think just REGION_WRITTEN_KEY_HISTOGRAM is enough.

Em, here we may just want to know the written keys rate, if too high, the store may have a high load, so Counter is ok.

siddontang · 2017-01-16T05:26:46Z

PTAL @BusyJay

BusyJay · 2017-01-16T05:38:00Z

src/raftstore/store/store.rs

+                continue;
+            }
+
+            self.region_written_bytes.observe(peer.written_bytes as f64);


Why observe it here rather every time handle ready?

+1， then we can remove the timer here.

on_raft_ready know nothing about time, what we want is to get region flow in a period of time.

BusyJay · 2017-01-16T06:14:33Z

LGTM

mertrics: region bytes written

5b5792a

BusyJay reviewed Jan 14, 2017

View reviewed changes

siddontang reviewed Jan 15, 2017

View reviewed changes

use local histogram

af98ebe

siddontang reviewed Jan 15, 2017

View reviewed changes

report region flow

00c38d3

zhangjinpeng87 changed the title ~~mertrics: region bytes written~~ mertrics: report region flow Jan 15, 2017

siddontang reviewed Jan 15, 2017

View reviewed changes

siddontang approved these changes Jan 15, 2017

View reviewed changes

let exist tests cover the case

292adc6

siddontang reviewed Jan 15, 2017

View reviewed changes

address comments

6da818c

siddontang reviewed Jan 16, 2017

View reviewed changes

zhangjinpeng87 added 2 commits January 16, 2017 11:10

Merge remote-tracking branch 'origin/master' into zhangjinpeng/metric…

01e3a96

…s-region-write-bytes

address comments

8f337fe

zhangjinpeng87 force-pushed the zhangjinpeng/metrics-region-write-bytes branch from 4d3ce7a to 8f337fe Compare January 16, 2017 03:28

BusyJay reviewed Jan 16, 2017

View reviewed changes

address comments, remove duplicated metric

8c94727

Merge branch 'master' into zhangjinpeng/metrics-region-write-bytes

bb11eae

BusyJay reviewed Jan 16, 2017

View reviewed changes

Merge branch 'master' into zhangjinpeng/metrics-region-write-bytes

54d8f88

siddontang merged commit 1b42363 into master Jan 16, 2017

siddontang deleted the zhangjinpeng/metrics-region-write-bytes branch January 16, 2017 06:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mertrics: report region flow #1517

mertrics: report region flow #1517

zhangjinpeng87 commented Jan 14, 2017

BusyJay Jan 14, 2017

zhangjinpeng87 Jan 14, 2017

BusyJay Jan 14, 2017

siddontang Jan 15, 2017

siddontang Jan 14, 2017

zhangjinpeng87 Jan 15, 2017

siddontang Jan 15, 2017

siddontang Jan 14, 2017

siddontang Jan 15, 2017

siddontang Jan 15, 2017

siddontang Jan 15, 2017

siddontang left a comment

siddontang commented Jan 15, 2017

hhkbp2 commented Jan 15, 2017

siddontang Jan 15, 2017

siddontang commented Jan 16, 2017

siddontang Jan 16, 2017

hhkbp2 Jan 16, 2017

zhangjinpeng87 commented Jan 16, 2017

BusyJay Jan 16, 2017

zhangjinpeng87 Jan 16, 2017

BusyJay Jan 16, 2017

siddontang Jan 16, 2017

siddontang commented Jan 16, 2017

BusyJay Jan 16, 2017

siddontang Jan 16, 2017

zhangjinpeng87 Jan 16, 2017

BusyJay commented Jan 16, 2017

mertrics: report region flow #1517

mertrics: report region flow #1517

Conversation

zhangjinpeng87 commented Jan 14, 2017

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

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

siddontang left a comment

Choose a reason for hiding this comment

siddontang commented Jan 15, 2017

hhkbp2 commented Jan 15, 2017

Choose a reason for hiding this comment

siddontang commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangjinpeng87 commented Jan 16, 2017

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

siddontang commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BusyJay commented Jan 16, 2017