Skip to content

Commit 93a6535

Browse files
committed
feat(storage,runtime): connect real StorageStatsCollector at storage I/O points
Replace the NoopStorageStatsCollector stub with a per-request RealStorageStatsCollector installed via the STORAGE_STATS_COLLECTOR task-local, so per-request storage metrics (keys/bytes read, written, deleted, cache hit, compaction level) are recorded at the actual I/O points instead of being inferred from command arguments. - Move StorageStats / StorageStatsCollector trait into the client leaf crate to avoid a storage <-> runtime dependency cycle; runtime::message re-exports them for backward compatibility. - Instrument the Storage facade methods in storage_impl.rs (the single convergence layer the cmd layer calls), requiring zero Cmd::execute signature changes. - Add StorageStats.bytes_deleted per issue #312. Fixes #312
1 parent 0f8d962 commit 93a6535

8 files changed

Lines changed: 375 additions & 72 deletions

File tree

Cargo.lock

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/client/Cargo.toml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,4 +11,5 @@ rust-version.workspace = true
1111
async-trait = "0.1"
1212
resp = { path = "../resp" }
1313
parking_lot = { workspace = true }
14-
tokio = { version = "1.50", features = ["sync"] }
14+
serde = { workspace = true, features = ["derive"] }
15+
tokio = { version = "1.50", features = ["sync", "rt"] }

src/client/src/lib.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@ use async_trait::async_trait;
2121
use resp::{ProtocolNegotiator, RespCommand, RespData, RespResult};
2222
use tokio::sync::Mutex;
2323

24+
pub mod storage_stats;
25+
pub use storage_stats::*;
26+
2427
#[async_trait]
2528
pub trait StreamTrait: Send + Sync {
2629
async fn read(&mut self, buf: &mut [u8]) -> Result<usize, std::io::Error>;

src/client/src/storage_stats.rs

Lines changed: 246 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,246 @@
1+
// Copyright (c) 2024-present, arana-db Community. All rights reserved.
2+
//
3+
// Licensed to the Apache Software Foundation (ASF) under one or more
4+
// contributor license agreements. See the NOTICE file distributed with
5+
// this work for additional information regarding copyright ownership.
6+
// The ASF licenses this file to You under the Apache License, Version 2.0
7+
// (the "License"); you may not use this file except in compliance with
8+
// the License. You may obtain a copy of the License at
9+
//
10+
// http://www.apache.org/licenses/LICENSE-2.0
11+
//
12+
// Unless required by applicable law or agreed to in writing, software
13+
// distributed under the License is distributed on an "AS IS" BASIS,
14+
// WITHOUT WARRANTIES OR CONDITIONS OF KIND, either express or implied.
15+
// See the License for the specific language governing permissions and
16+
// limitations under the License.
17+
18+
//! Request-local storage-layer instrumentation.
19+
//!
20+
//! These types were originally declared in `runtime::message` (see
21+
//! <https://github.com/arana-db/kiwi/issues/312>). They live in the `client`
22+
//! crate so that `storage`, `runtime` and `cmd` can share them without
23+
//! creating a dependency cycle (`client` is a leaf crate that everything else
24+
//! already depends on).
25+
//!
26+
//! The real collector ([`RealStorageStatsCollector`]) is installed once per
27+
//! storage request via the [`STORAGE_STATS_COLLECTOR`] task-local and read by
28+
//! the storage engine at the point where reads, writes and deletes actually
29+
//! happen, so the byte/key counters reflect true I/O rather than values
30+
//! inferred from Redis command arguments.
31+
32+
use std::sync::atomic::{AtomicBool, AtomicU64, Ordering};
33+
use std::sync::{Arc, Mutex as StdMutex};
34+
35+
use serde::{Deserialize, Serialize};
36+
37+
/// Statistics about storage operations for monitoring.
38+
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default)]
39+
pub struct StorageStats {
40+
/// Number of keys read during the operation
41+
pub keys_read: u64,
42+
/// Number of keys written during the operation
43+
pub keys_written: u64,
44+
/// Number of keys deleted during the operation
45+
pub keys_deleted: u64,
46+
/// Size of data read in bytes (key bytes + value bytes)
47+
pub bytes_read: u64,
48+
/// Size of data written in bytes (key bytes + value bytes)
49+
pub bytes_written: u64,
50+
/// Size of data removed by delete operations in bytes
51+
pub bytes_deleted: u64,
52+
/// Whether the operation hit a cache (e.g. block/moka cache)
53+
pub cache_hit: bool,
54+
/// RocksDB compaction level accessed
55+
pub compaction_level: Option<u32>,
56+
}
57+
58+
/// Request-local collector for storage-layer instrumentation.
59+
///
60+
/// Implementations must record the *actual* byte sizes measured by the storage
61+
/// layer, not sizes inferred from Redis command arguments.
62+
pub trait StorageStatsCollector: Send + Sync {
63+
/// Record a storage read. `key_bytes` and `value_bytes` are measured by the
64+
/// storage layer.
65+
fn record_read(&self, key_bytes: u64, value_bytes: u64);
66+
67+
/// Record a storage write. `key_bytes` and `value_bytes` are measured after
68+
/// the storage layer accepts the mutation.
69+
fn record_write(&self, key_bytes: u64, value_bytes: u64);
70+
71+
/// Record a storage delete. `key_bytes` is measured by the storage layer.
72+
fn record_delete(&self, key_bytes: u64);
73+
74+
/// Return the accumulated statistics for the request.
75+
fn finish(&self) -> StorageStats;
76+
}
77+
78+
/// Placeholder collector used until storage-layer instrumentation is wired in.
79+
#[derive(Debug, Default, Clone, Copy)]
80+
pub struct NoopStorageStatsCollector;
81+
82+
impl StorageStatsCollector for NoopStorageStatsCollector {
83+
fn record_read(&self, _key_bytes: u64, _value_bytes: u64) {}
84+
85+
fn record_write(&self, _key_bytes: u64, _value_bytes: u64) {}
86+
87+
fn record_delete(&self, _key_bytes: u64) {}
88+
89+
fn finish(&self) -> StorageStats {
90+
StorageStats::default()
91+
}
92+
}
93+
94+
/// A real per-request collector backed by lock-free atomic counters.
95+
///
96+
/// Safe to share across the (single) task that executes one storage request:
97+
/// every field is an atomic and `finish` snapshots the accumulated values.
98+
pub struct RealStorageStatsCollector {
99+
keys_read: AtomicU64,
100+
keys_written: AtomicU64,
101+
keys_deleted: AtomicU64,
102+
bytes_read: AtomicU64,
103+
bytes_written: AtomicU64,
104+
bytes_deleted: AtomicU64,
105+
cache_hit: AtomicBool,
106+
compaction_level: StdMutex<Option<u32>>,
107+
}
108+
109+
impl Default for RealStorageStatsCollector {
110+
fn default() -> Self {
111+
Self::new()
112+
}
113+
}
114+
115+
impl RealStorageStatsCollector {
116+
/// Create an empty collector ready to accumulate one request's stats.
117+
pub fn new() -> Self {
118+
Self {
119+
keys_read: AtomicU64::new(0),
120+
keys_written: AtomicU64::new(0),
121+
keys_deleted: AtomicU64::new(0),
122+
bytes_read: AtomicU64::new(0),
123+
bytes_written: AtomicU64::new(0),
124+
bytes_deleted: AtomicU64::new(0),
125+
cache_hit: AtomicBool::new(false),
126+
compaction_level: StdMutex::new(None),
127+
}
128+
}
129+
130+
/// Mark that this request hit a cache (e.g. block/moka cache).
131+
pub fn mark_cache_hit(&self) {
132+
self.cache_hit.store(true, Ordering::Relaxed);
133+
}
134+
135+
/// Record the RocksDB compaction level that served this request.
136+
pub fn set_compaction_level(&self, level: u32) {
137+
*self.compaction_level.lock().expect("compaction_level poisoned") = Some(level);
138+
}
139+
}
140+
141+
impl StorageStatsCollector for RealStorageStatsCollector {
142+
fn record_read(&self, key_bytes: u64, value_bytes: u64) {
143+
self.keys_read.fetch_add(1, Ordering::Relaxed);
144+
self.bytes_read
145+
.fetch_add(key_bytes.saturating_add(value_bytes), Ordering::Relaxed);
146+
}
147+
148+
fn record_write(&self, key_bytes: u64, value_bytes: u64) {
149+
self.keys_written.fetch_add(1, Ordering::Relaxed);
150+
self.bytes_written
151+
.fetch_add(key_bytes.saturating_add(value_bytes), Ordering::Relaxed);
152+
}
153+
154+
fn record_delete(&self, key_bytes: u64) {
155+
self.keys_deleted.fetch_add(1, Ordering::Relaxed);
156+
self.bytes_deleted.fetch_add(key_bytes, Ordering::Relaxed);
157+
}
158+
159+
fn finish(&self) -> StorageStats {
160+
StorageStats {
161+
keys_read: self.keys_read.load(Ordering::Relaxed),
162+
keys_written: self.keys_written.load(Ordering::Relaxed),
163+
keys_deleted: self.keys_deleted.load(Ordering::Relaxed),
164+
bytes_read: self.bytes_read.load(Ordering::Relaxed),
165+
bytes_written: self.bytes_written.load(Ordering::Relaxed),
166+
bytes_deleted: self.bytes_deleted.load(Ordering::Relaxed),
167+
cache_hit: self.cache_hit.load(Ordering::Relaxed),
168+
compaction_level: *self.compaction_level.lock().expect("compaction_level poisoned"),
169+
}
170+
}
171+
}
172+
173+
tokio::task_local! {
174+
/// Per-request storage stats collector, scoped for the duration of a single
175+
/// storage command execution. The storage layer reads it via
176+
/// [`try_collector`].
177+
pub static STORAGE_STATS_COLLECTOR: Arc<dyn StorageStatsCollector + Send + Sync>;
178+
}
179+
180+
/// Returns the active request-local collector, if one was installed for the
181+
/// current async task.
182+
///
183+
/// Returns `None` when no collector is scoped (e.g. unit tests that call the
184+
/// storage engine directly, or background storage tasks), so callers can record
185+
/// unconditionally without special-casing those paths.
186+
pub fn try_collector() -> Option<Arc<dyn StorageStatsCollector + Send + Sync>> {
187+
STORAGE_STATS_COLLECTOR.try_with(|c| c.clone()).ok()
188+
}
189+
190+
#[cfg(test)]
191+
mod tests {
192+
use super::*;
193+
194+
#[test]
195+
fn real_collector_accumulates_reads_writes_deletes() {
196+
let c = RealStorageStatsCollector::new();
197+
c.record_read(3, 5);
198+
c.record_write(3, 7);
199+
c.record_read(2, 4);
200+
c.record_delete(3);
201+
202+
let stats = c.finish();
203+
assert_eq!(stats.keys_read, 2);
204+
assert_eq!(stats.keys_written, 1);
205+
assert_eq!(stats.keys_deleted, 1);
206+
assert_eq!(stats.bytes_read, (3 + 5) + (2 + 4));
207+
assert_eq!(stats.bytes_written, 3 + 7);
208+
assert_eq!(stats.bytes_deleted, 3);
209+
assert!(!stats.cache_hit);
210+
}
211+
212+
#[test]
213+
fn noop_yields_empty_stats() {
214+
assert_eq!(NoopStorageStatsCollector.finish(), StorageStats::default());
215+
}
216+
217+
#[test]
218+
fn scoped_collector_is_visible_to_try_collector() {
219+
// Mirrors how `storage_server` installs the collector and how the
220+
// storage facade reads it via `try_collector` — without needing a real
221+
// RocksDB instance.
222+
let runtime = tokio::runtime::Builder::new_current_thread()
223+
.enable_all()
224+
.build()
225+
.unwrap();
226+
runtime.block_on(async {
227+
let collector: Arc<dyn StorageStatsCollector + Send + Sync> =
228+
Arc::new(RealStorageStatsCollector::new());
229+
STORAGE_STATS_COLLECTOR
230+
.scope(Arc::clone(&collector), async {
231+
if let Some(c) = try_collector() {
232+
c.record_write(3, 5);
233+
c.record_read(2, 4);
234+
} else {
235+
panic!("try_collector() must return the scoped collector");
236+
}
237+
})
238+
.await;
239+
let stats = collector.finish();
240+
assert_eq!(stats.keys_written, 1);
241+
assert_eq!(stats.bytes_written, 8);
242+
assert_eq!(stats.keys_read, 1);
243+
assert_eq!(stats.bytes_read, 6);
244+
});
245+
}
246+
}

src/common/runtime/message.rs

Lines changed: 10 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -73,62 +73,17 @@ pub enum StorageCommand {
7373
Batch { commands: Vec<StorageCommand> },
7474
}
7575

76-
/// Statistics about storage operations for monitoring
77-
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default)]
78-
pub struct StorageStats {
79-
/// Number of keys read during the operation
80-
pub keys_read: u64,
81-
/// Number of keys written during the operation
82-
pub keys_written: u64,
83-
/// Number of keys deleted during the operation
84-
pub keys_deleted: u64,
85-
/// Size of data read in bytes
86-
pub bytes_read: u64,
87-
/// Size of data written in bytes
88-
pub bytes_written: u64,
89-
/// Whether the operation hit the cache
90-
pub cache_hit: bool,
91-
/// RocksDB compaction level accessed
92-
pub compaction_level: Option<u32>,
93-
}
94-
95-
/// Request-local collector for storage-layer instrumentation.
76+
/// Storage-layer instrumentation types.
9677
///
97-
/// TODO(storage-stats): Thread a real collector through `Cmd::execute` and the
98-
/// `storage` crate APIs so these counters are recorded at the point where
99-
/// reads, writes, deletes, cache hits, and RocksDB details actually happen.
100-
/// Tracked in <https://github.com/arana-db/kiwi/issues/312>.
101-
pub trait StorageStatsCollector: Send + Sync {
102-
/// Record a storage read. `key_bytes` and `value_bytes` should be measured
103-
/// by the storage layer, not inferred from Redis command arguments.
104-
fn record_read(&self, key_bytes: u64, value_bytes: u64);
105-
106-
/// Record a storage write. `key_bytes` and `value_bytes` should be measured
107-
/// after the storage layer accepts the mutation.
108-
fn record_write(&self, key_bytes: u64, value_bytes: u64);
109-
110-
/// Record a storage delete. `key_bytes` should be measured by the storage layer.
111-
fn record_delete(&self, key_bytes: u64);
112-
113-
/// Return the accumulated statistics for the request.
114-
fn finish(&self) -> StorageStats;
115-
}
116-
117-
/// Placeholder collector used until storage-layer instrumentation is wired in.
118-
#[derive(Debug, Default, Clone, Copy)]
119-
pub struct NoopStorageStatsCollector;
120-
121-
impl StorageStatsCollector for NoopStorageStatsCollector {
122-
fn record_read(&self, _key_bytes: u64, _value_bytes: u64) {}
123-
124-
fn record_write(&self, _key_bytes: u64, _value_bytes: u64) {}
125-
126-
fn record_delete(&self, _key_bytes: u64) {}
127-
128-
fn finish(&self) -> StorageStats {
129-
StorageStats::default()
130-
}
131-
}
78+
/// These were originally defined here (see
79+
/// <https://github.com/arana-db/kiwi/issues/312>) but now live in the `client`
80+
/// crate so that `storage`, `runtime` and `cmd` can all depend on them without
81+
/// creating a dependency cycle. Re-exported here for backward compatibility with
82+
/// existing `runtime::message::*` importers.
83+
pub use client::storage_stats::{
84+
RealStorageStatsCollector, STORAGE_STATS_COLLECTOR, StorageStats, StorageStatsCollector,
85+
NoopStorageStatsCollector, try_collector,
86+
};
13287

13388
/// Request sent from network runtime to storage runtime
13489
#[derive(Debug)]

src/common/runtime/storage_server.rs

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@ use storage::storage::Storage;
3535
use crate::error::DualRuntimeError;
3636
use crate::global_storage::GlobalStorage;
3737
use crate::message::{
38-
NoopStorageStatsCollector, RequestPriority, StorageCommand, StorageRequest, StorageResponse,
39-
StorageStatsCollector,
38+
RealStorageStatsCollector, RequestPriority, STORAGE_STATS_COLLECTOR, StorageCommand,
39+
StorageRequest, StorageResponse, StorageStatsCollector,
4040
};
4141
use crate::metrics::StorageMetricsTracker;
4242

@@ -563,13 +563,21 @@ impl StorageServer {
563563
// tracker.record_operation_started();
564564
}
565565

566-
// TODO(storage-stats): Pass this request-local collector through
567-
// `Cmd::execute` and into the storage crate so stats are recorded by
568-
// actual storage operations instead of inferred from command arguments.
569-
let stats_collector = NoopStorageStatsCollector;
570-
571-
// Route the request based on command type
572-
let result = Self::execute_storage_command(&storage, &request.command).await;
566+
// Per-request storage stats collector, scoped for the duration of the
567+
// command execution so the storage engine records real I/O counters
568+
// instead of values inferred from command arguments.
569+
// See <https://github.com/arana-db/kiwi/issues/312>.
570+
let stats_collector: Arc<dyn StorageStatsCollector + Send + Sync> =
571+
Arc::new(RealStorageStatsCollector::new());
572+
573+
// Route the request based on command type. The collector is installed as a
574+
// task-local so the storage facade methods can read it without changing
575+
// every `Cmd::execute` signature.
576+
let result = STORAGE_STATS_COLLECTOR
577+
.scope(Arc::clone(&stats_collector), async move {
578+
Self::execute_storage_command(&storage, &request.command).await
579+
})
580+
.await;
573581

574582
let execution_time = start_time.elapsed();
575583
debug!(

0 commit comments

Comments
 (0)