From 91c00b496748e5f4e07ae2600ca1d99ddb20269a Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Tue, 14 Jan 2025 15:34:10 -0800 Subject: [PATCH 01/18] support histograms for command latency statistics --- CMakeLists.txt | 6 +++++ kvrocks.conf | 12 +++++++++ src/config/config.cc | 27 +++++++++++++++++++- src/config/config.h | 8 ++++++ src/server/server.cc | 39 ++++++++++++++++++++++++++++- src/stats/stats.cc | 28 +++++++++++++++++++++ src/stats/stats.h | 27 ++++++++++++++++++++ tests/cppunit/config_test.cc | 4 +++ tests/gocase/unit/info/info_test.go | 23 ++++++++++++++++- 9 files changed, 171 insertions(+), 3 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5f4f518d471..11fe463f923 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -29,6 +29,7 @@ option(ENABLE_STATIC_LIBSTDCXX "link kvrocks with static library of libstd++ ins option(ENABLE_LUAJIT "enable use of luaJIT instead of lua" ON) option(ENABLE_OPENSSL "enable openssl to support tls connection" OFF) option(ENABLE_IPO "enable interprocedural optimization" ON) +option(ENABLE_HISTOGRAMS "enable histograms to view the operation latencies" OFF) set(SYMBOLIZE_BACKEND "" CACHE STRING "symbolization backend library for cpptrace (libbacktrace, libdwarf, or empty)") set(PORTABLE 0 CACHE STRING "build a portable binary (disable arch-specific optimizations)") # TODO: set ENABLE_NEW_ENCODING to ON when we are ready @@ -288,6 +289,11 @@ if(ENABLE_IPO) endif() endif() +if(ENABLE_HISTOGRAMS) + target_compile_definitions(kvrocks_objs PUBLIC ENABLE_HISTOGRAMS) +endif() + + # kvrocks main target add_executable(kvrocks src/cli/main.cc) target_link_libraries(kvrocks PRIVATE kvrocks_objs ${EXTERNAL_LIBS}) diff --git a/kvrocks.conf b/kvrocks.conf index 16a980e5d51..a627164dd15 100644 --- a/kvrocks.conf +++ b/kvrocks.conf @@ -373,6 +373,18 @@ json-storage-format json # Default: no txn-context-enabled no +# Define the histogram bucket values. +# +# If enabled, those values will be used to store the command execution latency values +# in buckets defined below. The values should be integers and must be sorted. +# An implicit bucket (+Inf in prometheus jargon) will be added to track the highest values +# that are beyond the bucket limits. + +# NOTE: This is an experimental feature. There might be some performance overhead when using this +# feature, please be aware. +# Default: 10,20,40,60,80,100,150,250,350,500,750,1000,1500,2000,4000,8000 +# histogram-bucket-boundaries 10,20,40,60,80,100,150,250,350,500,750,1000,1500,2000,4000,8000 + ################################## TLS ################################### # By default, TLS/SSL is disabled, i.e. `tls-port` is set to 0. diff --git a/src/config/config.cc b/src/config/config.cc index 9d8e2a79272..b1988604481 100644 --- a/src/config/config.cc +++ b/src/config/config.cc @@ -240,7 +240,10 @@ Config::Config() { new EnumField(&json_storage_format, json_storage_formats, JsonStorageFormat::JSON)}, {"txn-context-enabled", true, new YesNoField(&txn_context_enabled, false)}, {"skip-block-cache-deallocation-on-close", false, new YesNoField(&skip_block_cache_deallocation_on_close, false)}, - +#ifdef ENABLE_HISTOGRAMS + {"histogram-bucket-boundaries", true, new StringField(&histogram_bucket_boundaries_str_, + "10,20,40,60,80,100,150,250,350,500,750,1000,1500,2000,4000,8000")}, +#endif /* rocksdb options */ {"rocksdb.compression", false, new EnumField(&rocks_db.compression, compression_types, @@ -754,6 +757,28 @@ void Config::initFieldCallback() { {"tls-session-caching", set_tls_option}, {"tls-session-cache-size", set_tls_option}, {"tls-session-cache-timeout", set_tls_option}, +#endif +#ifdef ENABLE_HISTOGRAMS + {"histogram-bucket-boundaries", + [this]([[maybe_unused]] Server *srv, [[maybe_unused]] const std::string &k, const std::string &v) -> Status { + std::vector buckets = util::Split(v, ","); + histogram_bucket_boundaries.clear(); + if (buckets.size() < 1) { + return {Status::NotOK, "Please provide at least 1 bucket value for histogram"}; + } + std::transform(buckets.begin(), buckets.end(), std::back_inserter(histogram_bucket_boundaries), [](const std::string& val) + { + return std::stod(val); + }); + if (histogram_bucket_boundaries.size() != buckets.size()) { + return {Status::NotOK, "All values for the bucket must be double or integer values"}; + } + + if (!std::is_sorted(histogram_bucket_boundaries.begin(), histogram_bucket_boundaries.end())) { + return {Status::NotOK, "The values for the histogram must be sorted"}; + } + return Status::OK(); + }}, #endif }; for (const auto &iter : callbacks) { diff --git a/src/config/config.h b/src/config/config.h index 9fa5d4168fc..e647013d041 100644 --- a/src/config/config.h +++ b/src/config/config.h @@ -177,6 +177,10 @@ struct Config { bool skip_block_cache_deallocation_on_close = false; +#ifdef ENABLE_HISTOGRAMS + std::vector histogram_bucket_boundaries; +#endif + struct RocksDB { int block_size; bool cache_index_and_filter_blocks; @@ -260,6 +264,10 @@ struct Config { std::string profiling_sample_commands_str_; std::map> fields_; std::vector rename_command_; +#ifdef ENABLE_HISTOGRAMS + std::string histogram_bucket_boundaries_str_; +#endif + void initFieldValidator(); void initFieldCallback(); diff --git a/src/server/server.cc b/src/server/server.cc index 5b52eb333fb..7070cb7d14f 100644 --- a/src/server/server.cc +++ b/src/server/server.cc @@ -52,7 +52,11 @@ #include "worker.h" Server::Server(engine::Storage *storage, Config *config) - : storage(storage), + : +#ifdef ENABLE_HISTOGRAMS + stats(config), +#endif + storage(storage), indexer(storage), index_mgr(&indexer, storage), start_time_secs_(util::GetTimeStamp()), @@ -63,6 +67,18 @@ Server::Server(engine::Storage *storage, Config *config) for (const auto &iter : *commands) { stats.commands_stats[iter.first].calls = 0; stats.commands_stats[iter.first].latency = 0; + +#ifdef ENABLE_HISTOGRAMS + //NB: Extra index for the last bucket (Inf) + for (std::size_t i{0}; i <= stats.bucket_boundaries.size(); ++i) { + //auto bucket_ptr = std::make_shared>(0); + auto bucket_ptr = std::shared_ptr>(new std::atomic(0)); + + stats.commands_histogram[iter.first].buckets.push_back(bucket_ptr); + } + stats.commands_histogram[iter.first].calls = 0; + stats.commands_histogram[iter.first].sum = 0; +#endif } // init cursor_dict_ @@ -1165,6 +1181,27 @@ void Server::GetCommandsStatsInfo(std::string *info) { << ",usec_per_call=" << static_cast(latency / calls) << "\r\n"; } +#ifdef ENABLE_HISTOGRAMS + for (const auto &cmd_hist : stats.commands_histogram) { + auto command_name = cmd_hist.first; + auto calls = stats.commands_histogram[command_name].calls.load(); + if (calls == 0) continue; + + auto sum = stats.commands_histogram[command_name].sum.load(); + string_stream << "cmdstathist_" << command_name << ":"; + for (std::size_t i{0}; i < stats.commands_histogram[command_name].buckets.size(); ++i) { + auto bucket_value = stats.commands_histogram[command_name].buckets[i]->load(); + auto bucket_bound = std::numeric_limits::infinity(); + if (i < stats.bucket_boundaries.size()) { + bucket_bound = stats.bucket_boundaries[i]; + } + + string_stream << bucket_bound << "=" << bucket_value << ","; + } + string_stream << "sum=" << sum << ",count=" << calls << "\r\n"; + } +#endif + *info = string_stream.str(); } diff --git a/src/stats/stats.cc b/src/stats/stats.cc index ae18638b221..a40d4b75f34 100644 --- a/src/stats/stats.cc +++ b/src/stats/stats.cc @@ -26,6 +26,23 @@ #include "fmt/format.h" #include "time_util.h" + +#ifdef ENABLE_HISTOGRAMS +Stats::Stats(Config *config) + : config_(config) { + for (int i = 0; i < STATS_METRIC_COUNT; i++) { + InstMetric im; + im.last_sample_time_ms = 0; + im.last_sample_count = 0; + im.idx = 0; + for (uint64_t &sample : im.samples) { + sample = 0; + } + inst_metrics.push_back(im); + } + bucket_boundaries = config_->histogram_bucket_boundaries; +} +#else Stats::Stats() { for (int i = 0; i < STATS_METRIC_COUNT; i++) { InstMetric im; @@ -38,6 +55,7 @@ Stats::Stats() { inst_metrics.push_back(im); } } +#endif #if defined(__APPLE__) #include @@ -86,10 +104,20 @@ int64_t Stats::GetMemoryRSS() { void Stats::IncrCalls(const std::string &command_name) { total_calls.fetch_add(1, std::memory_order_relaxed); commands_stats[command_name].calls.fetch_add(1, std::memory_order_relaxed); +#ifdef ENABLE_HISTOGRAMS + commands_histogram[command_name].calls.fetch_add(1, std::memory_order_relaxed); +#endif } void Stats::IncrLatency(uint64_t latency, const std::string &command_name) { commands_stats[command_name].latency.fetch_add(latency, std::memory_order_relaxed); +#ifdef ENABLE_HISTOGRAMS + commands_histogram[command_name].sum.fetch_add(latency, std::memory_order_relaxed); + + const auto bucket_index = static_cast(std::distance( + bucket_boundaries.begin(), std::lower_bound(bucket_boundaries.begin(), bucket_boundaries.end(), latency))); + commands_histogram[command_name].buckets[bucket_index]->fetch_add(1, std::memory_order_relaxed); +#endif } void Stats::TrackInstantaneousMetric(int metric, uint64_t current_reading) { diff --git a/src/stats/stats.h b/src/stats/stats.h index e00506a9672..0ee1b2f9a91 100644 --- a/src/stats/stats.h +++ b/src/stats/stats.h @@ -27,6 +27,10 @@ #include #include #include +#ifdef ENABLE_HISTOGRAMS +#include +#include "config/config.h" +#endif enum StatsMetricFlags { STATS_METRIC_COMMAND = 0, // Number of commands executed @@ -43,6 +47,15 @@ enum StatsMetricFlags { constexpr int STATS_METRIC_SAMPLES = 16; // Number of samples per metric +#ifdef ENABLE_HISTOGRAMS +// Experimental part to support histograms for cmd statistics +struct CommandHistogram { + std::vector>> buckets; + std::atomic calls; + std::atomic sum; +}; +#endif + struct CommandStat { std::atomic calls; std::atomic latency; @@ -57,6 +70,14 @@ struct InstMetric { class Stats { public: +#ifdef ENABLE_HISTOGRAMS + using BucketBoundaries = std::vector; + BucketBoundaries bucket_boundaries; + std::map commands_histogram; + + Config *config_ = nullptr; +#endif + std::atomic total_calls = {0}; std::atomic in_bytes = {0}; std::atomic out_bytes = {0}; @@ -69,7 +90,13 @@ class Stats { std::atomic psync_ok_count = {0}; std::map commands_stats; + +#ifdef ENABLE_HISTOGRAMS + explicit Stats(Config *config); +#else Stats(); +#endif + void IncrCalls(const std::string &command_name); void IncrLatency(uint64_t latency, const std::string &command_name); void IncrInboundBytes(uint64_t bytes) { in_bytes.fetch_add(bytes, std::memory_order_relaxed); } diff --git a/tests/cppunit/config_test.cc b/tests/cppunit/config_test.cc index 4e9b3817a9f..d0687c333a3 100644 --- a/tests/cppunit/config_test.cc +++ b/tests/cppunit/config_test.cc @@ -130,6 +130,10 @@ TEST(Config, GetAndSet) { {"rocksdb.rate_limiter_auto_tuned", "yes"}, {"rocksdb.compression_level", "32767"}, {"rocksdb.wal_compression", "no"}, +#ifdef ENABLE_HISTOGRAMS + {"histogram-bucket-boundaries", "10,100,1000,10000"}, +#endif + }; for (const auto &iter : immutable_cases) { s = config.Set(nullptr, iter.first, iter.second); diff --git a/tests/gocase/unit/info/info_test.go b/tests/gocase/unit/info/info_test.go index dd4b0cdcc78..6b706379a04 100644 --- a/tests/gocase/unit/info/info_test.go +++ b/tests/gocase/unit/info/info_test.go @@ -23,6 +23,7 @@ import ( "context" "fmt" "strconv" + "strings" "testing" "time" @@ -33,7 +34,10 @@ import ( ) func TestInfo(t *testing.T) { - srv0 := util.StartServer(t, map[string]string{"cluster-enabled": "yes"}) + srv0 := util.StartServer(t, map[string]string{ + "cluster-enabled": "yes", + "histogram-bucket-boundaries": "10,20,30,50", + }) defer func() { srv0.Close() }() rdb0 := srv0.NewClient() defer func() { require.NoError(t, rdb0.Close()) }() @@ -102,6 +106,23 @@ func TestInfo(t *testing.T) { t.Run("get cluster information by INFO - cluster enabled", func(t *testing.T) { require.Equal(t, "1", util.FindInfoEntry(rdb0, "cluster_enabled", "cluster")) }) + + t.Run("get command latencies via histogram INFO - histogram-bucket-boundaries", func(t *testing.T) { + output := util.FindInfoEntry(rdb0, "cmdstathist", "cmdstathist_info") + if len(output) == 0 { + t.SkipNow() + } + + splitValues := strings.FieldsFunc(output, func(r rune) bool { + return r == '=' || r == ',' + }) + + // expected: 10=..,20=..,30=..,50=..,inf=..,sum=...,count=.. + require.GreaterOrEqual(t, len(splitValues), 15) + require.Contains(t, splitValues, "sum") + require.Contains(t, splitValues, "count") + require.Contains(t, splitValues, "info") + }) } func TestKeyspaceHitMiss(t *testing.T) { From 99d3f5cfc049cda15a38ad92bcc6437f04bd5e2c Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Tue, 14 Jan 2025 15:42:06 -0800 Subject: [PATCH 02/18] clean up left out comment --- src/server/server.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/server/server.cc b/src/server/server.cc index 7070cb7d14f..07fc1be3eaa 100644 --- a/src/server/server.cc +++ b/src/server/server.cc @@ -71,7 +71,6 @@ Server::Server(engine::Storage *storage, Config *config) #ifdef ENABLE_HISTOGRAMS //NB: Extra index for the last bucket (Inf) for (std::size_t i{0}; i <= stats.bucket_boundaries.size(); ++i) { - //auto bucket_ptr = std::make_shared>(0); auto bucket_ptr = std::shared_ptr>(new std::atomic(0)); stats.commands_histogram[iter.first].buckets.push_back(bucket_ptr); From f6fc1bca89d3d9b8ebba099df0188d7c1bc9c116 Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Wed, 15 Jan 2025 09:34:45 -0800 Subject: [PATCH 03/18] format --- src/config/config.cc | 37 ++++++++++++++++++------------------- src/config/config.h | 1 - src/server/server.cc | 2 +- src/stats/stats.cc | 4 +--- src/stats/stats.h | 2 +- 5 files changed, 21 insertions(+), 25 deletions(-) diff --git a/src/config/config.cc b/src/config/config.cc index b1988604481..4ba46a13d03 100644 --- a/src/config/config.cc +++ b/src/config/config.cc @@ -131,8 +131,8 @@ Status SetRocksdbCompression(Server *srv, const rocksdb::CompressionType compres for (size_t i = compression_start_level; i < KVROCKS_MAX_LSM_LEVEL; i++) { compression_per_level_builder.emplace_back(compression_option); } - const std::string compression_per_level = util::StringJoin( - compression_per_level_builder, [](const auto &s) -> decltype(auto) { return s; }, ":"); + const std::string compression_per_level = + util::StringJoin(compression_per_level_builder, [](const auto &s) -> decltype(auto) { return s; }, ":"); return srv->storage->SetOptionForAllColumnFamilies("compression_per_level", compression_per_level); }; @@ -241,8 +241,9 @@ Config::Config() { {"txn-context-enabled", true, new YesNoField(&txn_context_enabled, false)}, {"skip-block-cache-deallocation-on-close", false, new YesNoField(&skip_block_cache_deallocation_on_close, false)}, #ifdef ENABLE_HISTOGRAMS - {"histogram-bucket-boundaries", true, new StringField(&histogram_bucket_boundaries_str_, - "10,20,40,60,80,100,150,250,350,500,750,1000,1500,2000,4000,8000")}, + {"histogram-bucket-boundaries", true, + new StringField(&histogram_bucket_boundaries_str_, + "10,20,40,60,80,100,150,250,350,500,750,1000,1500,2000,4000,8000")}, #endif /* rocksdb options */ {"rocksdb.compression", false, @@ -761,22 +762,20 @@ void Config::initFieldCallback() { #ifdef ENABLE_HISTOGRAMS {"histogram-bucket-boundaries", [this]([[maybe_unused]] Server *srv, [[maybe_unused]] const std::string &k, const std::string &v) -> Status { - std::vector buckets = util::Split(v, ","); - histogram_bucket_boundaries.clear(); - if (buckets.size() < 1) { - return {Status::NotOK, "Please provide at least 1 bucket value for histogram"}; - } - std::transform(buckets.begin(), buckets.end(), std::back_inserter(histogram_bucket_boundaries), [](const std::string& val) - { - return std::stod(val); - }); - if (histogram_bucket_boundaries.size() != buckets.size()) { - return {Status::NotOK, "All values for the bucket must be double or integer values"}; - } + std::vector buckets = util::Split(v, ","); + histogram_bucket_boundaries.clear(); + if (buckets.size() < 1) { + return {Status::NotOK, "Please provide at least 1 bucket value for histogram"}; + } + std::transform(buckets.begin(), buckets.end(), std::back_inserter(histogram_bucket_boundaries), + [](const std::string &val) { return std::stod(val); }); + if (histogram_bucket_boundaries.size() != buckets.size()) { + return {Status::NotOK, "All values for the bucket must be double or integer values"}; + } - if (!std::is_sorted(histogram_bucket_boundaries.begin(), histogram_bucket_boundaries.end())) { - return {Status::NotOK, "The values for the histogram must be sorted"}; - } + if (!std::is_sorted(histogram_bucket_boundaries.begin(), histogram_bucket_boundaries.end())) { + return {Status::NotOK, "The values for the histogram must be sorted"}; + } return Status::OK(); }}, #endif diff --git a/src/config/config.h b/src/config/config.h index e647013d041..2b21b15c0a2 100644 --- a/src/config/config.h +++ b/src/config/config.h @@ -268,7 +268,6 @@ struct Config { std::string histogram_bucket_boundaries_str_; #endif - void initFieldValidator(); void initFieldCallback(); Status parseConfigFromPair(const std::pair &input, int line_number); diff --git a/src/server/server.cc b/src/server/server.cc index 07fc1be3eaa..2d6f4a7a63d 100644 --- a/src/server/server.cc +++ b/src/server/server.cc @@ -69,7 +69,7 @@ Server::Server(engine::Storage *storage, Config *config) stats.commands_stats[iter.first].latency = 0; #ifdef ENABLE_HISTOGRAMS - //NB: Extra index for the last bucket (Inf) + // NB: Extra index for the last bucket (Inf) for (std::size_t i{0}; i <= stats.bucket_boundaries.size(); ++i) { auto bucket_ptr = std::shared_ptr>(new std::atomic(0)); diff --git a/src/stats/stats.cc b/src/stats/stats.cc index a40d4b75f34..ffc34bb8ec6 100644 --- a/src/stats/stats.cc +++ b/src/stats/stats.cc @@ -26,10 +26,8 @@ #include "fmt/format.h" #include "time_util.h" - #ifdef ENABLE_HISTOGRAMS -Stats::Stats(Config *config) - : config_(config) { +Stats::Stats(Config *config) : config_(config) { for (int i = 0; i < STATS_METRIC_COUNT; i++) { InstMetric im; im.last_sample_time_ms = 0; diff --git a/src/stats/stats.h b/src/stats/stats.h index 0ee1b2f9a91..b18c5ee5782 100644 --- a/src/stats/stats.h +++ b/src/stats/stats.h @@ -29,6 +29,7 @@ #include #ifdef ENABLE_HISTOGRAMS #include + #include "config/config.h" #endif @@ -90,7 +91,6 @@ class Stats { std::atomic psync_ok_count = {0}; std::map commands_stats; - #ifdef ENABLE_HISTOGRAMS explicit Stats(Config *config); #else From a6e186894b25d356079bd15a593314fe5ca86526 Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Wed, 15 Jan 2025 10:53:46 -0800 Subject: [PATCH 04/18] config driven --- kvrocks.conf | 18 +++++++++--------- src/config/config.cc | 11 +++-------- src/config/config.h | 4 ---- src/server/server.cc | 24 ++++++++++-------------- src/stats/stats.cc | 37 +++++++++++++------------------------ src/stats/stats.h | 26 ++++++++------------------ 6 files changed, 43 insertions(+), 77 deletions(-) diff --git a/kvrocks.conf b/kvrocks.conf index a627164dd15..6f964f65629 100644 --- a/kvrocks.conf +++ b/kvrocks.conf @@ -20,13 +20,13 @@ bind 127.0.0.1 # unixsocket /tmp/kvrocks.sock # unixsocketperm 777 -# Allows a parent process to open a socket and pass its FD down to kvrocks as a child +# Allows a parent process to open a socket and pass its FD down to kvrocks as a child # process. Useful to reserve a port and prevent race conditions. -# -# PLEASE NOTE: -# If this is overridden to a value other than -1, the bind and tls* directives will be +# +# PLEASE NOTE: +# If this is overridden to a value other than -1, the bind and tls* directives will be # ignored. -# +# # Default: -1 (not overridden, defer to creating a connection to the specified port) socket-fd -1 @@ -369,7 +369,7 @@ json-storage-format json # NOTE: This is an experimental feature. If you find errors, performance degradation, # excessive memory usage, excessive disk I/O, etc. after enabling it, please try disabling it. # At the same time, we welcome feedback on related issues to help iterative improvements. -# +# # Default: no txn-context-enabled no @@ -382,8 +382,8 @@ txn-context-enabled no # NOTE: This is an experimental feature. There might be some performance overhead when using this # feature, please be aware. -# Default: 10,20,40,60,80,100,150,250,350,500,750,1000,1500,2000,4000,8000 -# histogram-bucket-boundaries 10,20,40,60,80,100,150,250,350,500,750,1000,1500,2000,4000,8000 +# Default: disabled +histogram-bucket-boundaries 10,20,40,60,80,100,150,250,350,500,750,1000,1500,2000,4000,8000 ################################## TLS ################################### @@ -1043,7 +1043,7 @@ rocksdb.partition_filters yes # Specifies the maximum size in bytes for a write batch in RocksDB. # If set to 0, there is no size limit for write batches. # This option can help control memory usage and manage large WriteBatch operations more effectively. -# +# # Default: 0 # rocksdb.write_options.write_batch_max_bytes 0 diff --git a/src/config/config.cc b/src/config/config.cc index 4ba46a13d03..14576d12be6 100644 --- a/src/config/config.cc +++ b/src/config/config.cc @@ -240,11 +240,8 @@ Config::Config() { new EnumField(&json_storage_format, json_storage_formats, JsonStorageFormat::JSON)}, {"txn-context-enabled", true, new YesNoField(&txn_context_enabled, false)}, {"skip-block-cache-deallocation-on-close", false, new YesNoField(&skip_block_cache_deallocation_on_close, false)}, -#ifdef ENABLE_HISTOGRAMS - {"histogram-bucket-boundaries", true, - new StringField(&histogram_bucket_boundaries_str_, - "10,20,40,60,80,100,150,250,350,500,750,1000,1500,2000,4000,8000")}, -#endif + {"histogram-bucket-boundaries", true, new StringField(&histogram_bucket_boundaries_str_, "")}, + /* rocksdb options */ {"rocksdb.compression", false, new EnumField(&rocks_db.compression, compression_types, @@ -759,13 +756,12 @@ void Config::initFieldCallback() { {"tls-session-cache-size", set_tls_option}, {"tls-session-cache-timeout", set_tls_option}, #endif -#ifdef ENABLE_HISTOGRAMS {"histogram-bucket-boundaries", [this]([[maybe_unused]] Server *srv, [[maybe_unused]] const std::string &k, const std::string &v) -> Status { std::vector buckets = util::Split(v, ","); histogram_bucket_boundaries.clear(); if (buckets.size() < 1) { - return {Status::NotOK, "Please provide at least 1 bucket value for histogram"}; + return Status::OK(); } std::transform(buckets.begin(), buckets.end(), std::back_inserter(histogram_bucket_boundaries), [](const std::string &val) { return std::stod(val); }); @@ -778,7 +774,6 @@ void Config::initFieldCallback() { } return Status::OK(); }}, -#endif }; for (const auto &iter : callbacks) { auto field_iter = fields_.find(iter.first); diff --git a/src/config/config.h b/src/config/config.h index 2b21b15c0a2..7c76759c241 100644 --- a/src/config/config.h +++ b/src/config/config.h @@ -177,9 +177,7 @@ struct Config { bool skip_block_cache_deallocation_on_close = false; -#ifdef ENABLE_HISTOGRAMS std::vector histogram_bucket_boundaries; -#endif struct RocksDB { int block_size; @@ -264,9 +262,7 @@ struct Config { std::string profiling_sample_commands_str_; std::map> fields_; std::vector rename_command_; -#ifdef ENABLE_HISTOGRAMS std::string histogram_bucket_boundaries_str_; -#endif void initFieldValidator(); void initFieldCallback(); diff --git a/src/server/server.cc b/src/server/server.cc index 2d6f4a7a63d..7442561fedc 100644 --- a/src/server/server.cc +++ b/src/server/server.cc @@ -52,10 +52,7 @@ #include "worker.h" Server::Server(engine::Storage *storage, Config *config) - : -#ifdef ENABLE_HISTOGRAMS - stats(config), -#endif + : stats(config), storage(storage), indexer(storage), index_mgr(&indexer, storage), @@ -64,20 +61,21 @@ Server::Server(engine::Storage *storage, Config *config) namespace_(storage) { // init commands stats here to prevent concurrent insert, and cause core auto commands = redis::CommandTable::GetOriginal(); + for (const auto &iter : *commands) { stats.commands_stats[iter.first].calls = 0; stats.commands_stats[iter.first].latency = 0; -#ifdef ENABLE_HISTOGRAMS - // NB: Extra index for the last bucket (Inf) - for (std::size_t i{0}; i <= stats.bucket_boundaries.size(); ++i) { - auto bucket_ptr = std::shared_ptr>(new std::atomic(0)); + if (stats.bucket_boundaries.size() > 0) { + // NB: Extra index for the last bucket (Inf) + for (std::size_t i{0}; i <= stats.bucket_boundaries.size(); ++i) { + auto bucket_ptr = std::shared_ptr>(new std::atomic(0)); - stats.commands_histogram[iter.first].buckets.push_back(bucket_ptr); + stats.commands_histogram[iter.first].buckets.push_back(bucket_ptr); + } + stats.commands_histogram[iter.first].calls = 0; + stats.commands_histogram[iter.first].sum = 0; } - stats.commands_histogram[iter.first].calls = 0; - stats.commands_histogram[iter.first].sum = 0; -#endif } // init cursor_dict_ @@ -1180,7 +1178,6 @@ void Server::GetCommandsStatsInfo(std::string *info) { << ",usec_per_call=" << static_cast(latency / calls) << "\r\n"; } -#ifdef ENABLE_HISTOGRAMS for (const auto &cmd_hist : stats.commands_histogram) { auto command_name = cmd_hist.first; auto calls = stats.commands_histogram[command_name].calls.load(); @@ -1199,7 +1196,6 @@ void Server::GetCommandsStatsInfo(std::string *info) { } string_stream << "sum=" << sum << ",count=" << calls << "\r\n"; } -#endif *info = string_stream.str(); } diff --git a/src/stats/stats.cc b/src/stats/stats.cc index ffc34bb8ec6..b4de1701472 100644 --- a/src/stats/stats.cc +++ b/src/stats/stats.cc @@ -26,7 +26,6 @@ #include "fmt/format.h" #include "time_util.h" -#ifdef ENABLE_HISTOGRAMS Stats::Stats(Config *config) : config_(config) { for (int i = 0; i < STATS_METRIC_COUNT; i++) { InstMetric im; @@ -38,22 +37,10 @@ Stats::Stats(Config *config) : config_(config) { } inst_metrics.push_back(im); } - bucket_boundaries = config_->histogram_bucket_boundaries; -} -#else -Stats::Stats() { - for (int i = 0; i < STATS_METRIC_COUNT; i++) { - InstMetric im; - im.last_sample_time_ms = 0; - im.last_sample_count = 0; - im.idx = 0; - for (uint64_t &sample : im.samples) { - sample = 0; - } - inst_metrics.push_back(im); + if (config_->histogram_bucket_boundaries.size() > 0) { + bucket_boundaries = config_->histogram_bucket_boundaries; } } -#endif #if defined(__APPLE__) #include @@ -102,20 +89,22 @@ int64_t Stats::GetMemoryRSS() { void Stats::IncrCalls(const std::string &command_name) { total_calls.fetch_add(1, std::memory_order_relaxed); commands_stats[command_name].calls.fetch_add(1, std::memory_order_relaxed); -#ifdef ENABLE_HISTOGRAMS - commands_histogram[command_name].calls.fetch_add(1, std::memory_order_relaxed); -#endif + + if (bucket_boundaries.size() > 0) { + commands_histogram[command_name].calls.fetch_add(1, std::memory_order_relaxed); + } } void Stats::IncrLatency(uint64_t latency, const std::string &command_name) { commands_stats[command_name].latency.fetch_add(latency, std::memory_order_relaxed); -#ifdef ENABLE_HISTOGRAMS - commands_histogram[command_name].sum.fetch_add(latency, std::memory_order_relaxed); - const auto bucket_index = static_cast(std::distance( - bucket_boundaries.begin(), std::lower_bound(bucket_boundaries.begin(), bucket_boundaries.end(), latency))); - commands_histogram[command_name].buckets[bucket_index]->fetch_add(1, std::memory_order_relaxed); -#endif + if (bucket_boundaries.size() > 0) { + commands_histogram[command_name].sum.fetch_add(latency, std::memory_order_relaxed); + + const auto bucket_index = static_cast(std::distance( + bucket_boundaries.begin(), std::lower_bound(bucket_boundaries.begin(), bucket_boundaries.end(), latency))); + commands_histogram[command_name].buckets[bucket_index]->fetch_add(1, std::memory_order_relaxed); + } } void Stats::TrackInstantaneousMetric(int metric, uint64_t current_reading) { diff --git a/src/stats/stats.h b/src/stats/stats.h index b18c5ee5782..7d5bc02c4cd 100644 --- a/src/stats/stats.h +++ b/src/stats/stats.h @@ -22,16 +22,14 @@ #include +#include #include #include #include #include #include -#ifdef ENABLE_HISTOGRAMS -#include #include "config/config.h" -#endif enum StatsMetricFlags { STATS_METRIC_COMMAND = 0, // Number of commands executed @@ -48,14 +46,12 @@ enum StatsMetricFlags { constexpr int STATS_METRIC_SAMPLES = 16; // Number of samples per metric -#ifdef ENABLE_HISTOGRAMS // Experimental part to support histograms for cmd statistics struct CommandHistogram { std::vector>> buckets; std::atomic calls; std::atomic sum; }; -#endif struct CommandStat { std::atomic calls; @@ -71,14 +67,6 @@ struct InstMetric { class Stats { public: -#ifdef ENABLE_HISTOGRAMS - using BucketBoundaries = std::vector; - BucketBoundaries bucket_boundaries; - std::map commands_histogram; - - Config *config_ = nullptr; -#endif - std::atomic total_calls = {0}; std::atomic in_bytes = {0}; std::atomic out_bytes = {0}; @@ -91,11 +79,13 @@ class Stats { std::atomic psync_ok_count = {0}; std::map commands_stats; -#ifdef ENABLE_HISTOGRAMS - explicit Stats(Config *config); -#else - Stats(); -#endif + using BucketBoundaries = std::vector; + BucketBoundaries bucket_boundaries; + std::map commands_histogram; + + Config *config_ = nullptr; + + Stats(Config *config); void IncrCalls(const std::string &command_name); void IncrLatency(uint64_t latency, const std::string &command_name); From 93d5d4cf9e0f4e906adaca392308eee044b03305 Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Wed, 15 Jan 2025 11:21:20 -0800 Subject: [PATCH 05/18] revert unrelated change --- src/config/config.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/config/config.cc b/src/config/config.cc index 14576d12be6..53460a63076 100644 --- a/src/config/config.cc +++ b/src/config/config.cc @@ -131,8 +131,8 @@ Status SetRocksdbCompression(Server *srv, const rocksdb::CompressionType compres for (size_t i = compression_start_level; i < KVROCKS_MAX_LSM_LEVEL; i++) { compression_per_level_builder.emplace_back(compression_option); } - const std::string compression_per_level = - util::StringJoin(compression_per_level_builder, [](const auto &s) -> decltype(auto) { return s; }, ":"); + const std::string compression_per_level = util::StringJoin( + compression_per_level_builder, [](const auto &s) -> decltype(auto) { return s; }, ":"); return srv->storage->SetOptionForAllColumnFamilies("compression_per_level", compression_per_level); }; From 9f89ee22130fe25ad206d13a3e6eb5ab9afc430c Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Wed, 15 Jan 2025 11:22:24 -0800 Subject: [PATCH 06/18] remove tabs --- src/config/config.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config/config.cc b/src/config/config.cc index 53460a63076..ffc38215fe0 100644 --- a/src/config/config.cc +++ b/src/config/config.cc @@ -132,7 +132,7 @@ Status SetRocksdbCompression(Server *srv, const rocksdb::CompressionType compres compression_per_level_builder.emplace_back(compression_option); } const std::string compression_per_level = util::StringJoin( - compression_per_level_builder, [](const auto &s) -> decltype(auto) { return s; }, ":"); + compression_per_level_builder, [](const auto &s) -> decltype(auto) { return s; }, ":"); return srv->storage->SetOptionForAllColumnFamilies("compression_per_level", compression_per_level); }; From e1663b04cdcccee4a3660846c70ddb19a2eaa71a Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Wed, 15 Jan 2025 11:22:57 -0800 Subject: [PATCH 07/18] extra tabs --- src/config/config.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config/config.cc b/src/config/config.cc index ffc38215fe0..6e2dd5cc404 100644 --- a/src/config/config.cc +++ b/src/config/config.cc @@ -132,7 +132,7 @@ Status SetRocksdbCompression(Server *srv, const rocksdb::CompressionType compres compression_per_level_builder.emplace_back(compression_option); } const std::string compression_per_level = util::StringJoin( - compression_per_level_builder, [](const auto &s) -> decltype(auto) { return s; }, ":"); + compression_per_level_builder, [](const auto &s) -> decltype(auto) { return s; }, ":"); return srv->storage->SetOptionForAllColumnFamilies("compression_per_level", compression_per_level); }; From fbac462c1d37ea52eb4f015b39b961e57c89660e Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Wed, 15 Jan 2025 11:23:54 -0800 Subject: [PATCH 08/18] fix indentation --- src/config/config.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config/config.cc b/src/config/config.cc index 6e2dd5cc404..ae1ebb5f92f 100644 --- a/src/config/config.cc +++ b/src/config/config.cc @@ -132,7 +132,7 @@ Status SetRocksdbCompression(Server *srv, const rocksdb::CompressionType compres compression_per_level_builder.emplace_back(compression_option); } const std::string compression_per_level = util::StringJoin( - compression_per_level_builder, [](const auto &s) -> decltype(auto) { return s; }, ":"); + compression_per_level_builder, [](const auto &s) -> decltype(auto) { return s; }, ":"); return srv->storage->SetOptionForAllColumnFamilies("compression_per_level", compression_per_level); }; From c35bf88a4dbddd8936ed6d88b2c2d7e3df99d90a Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Wed, 15 Jan 2025 12:52:02 -0800 Subject: [PATCH 09/18] config update and fix formatting --- kvrocks.conf | 2 +- src/stats/stats.h | 7 ++++--- tests/cppunit/config_test.cc | 2 -- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/kvrocks.conf b/kvrocks.conf index 6f964f65629..461d077eb3f 100644 --- a/kvrocks.conf +++ b/kvrocks.conf @@ -383,7 +383,7 @@ txn-context-enabled no # NOTE: This is an experimental feature. There might be some performance overhead when using this # feature, please be aware. # Default: disabled -histogram-bucket-boundaries 10,20,40,60,80,100,150,250,350,500,750,1000,1500,2000,4000,8000 +# histogram-bucket-boundaries 10,20,40,60,80,100,150,250,350,500,750,1000,1500,2000,4000,8000 ################################## TLS ################################### diff --git a/src/stats/stats.h b/src/stats/stats.h index 7d5bc02c4cd..eea220eeb31 100644 --- a/src/stats/stats.h +++ b/src/stats/stats.h @@ -83,9 +83,7 @@ class Stats { BucketBoundaries bucket_boundaries; std::map commands_histogram; - Config *config_ = nullptr; - - Stats(Config *config); + explicit Stats(Config *config); void IncrCalls(const std::string &command_name); void IncrLatency(uint64_t latency, const std::string &command_name); @@ -97,4 +95,7 @@ class Stats { static int64_t GetMemoryRSS(); void TrackInstantaneousMetric(int metric, uint64_t current_reading); uint64_t GetInstantaneousMetric(int metric) const; + +private: + Config *config_ = nullptr; }; diff --git a/tests/cppunit/config_test.cc b/tests/cppunit/config_test.cc index d0687c333a3..2a6ac1b5608 100644 --- a/tests/cppunit/config_test.cc +++ b/tests/cppunit/config_test.cc @@ -130,9 +130,7 @@ TEST(Config, GetAndSet) { {"rocksdb.rate_limiter_auto_tuned", "yes"}, {"rocksdb.compression_level", "32767"}, {"rocksdb.wal_compression", "no"}, -#ifdef ENABLE_HISTOGRAMS {"histogram-bucket-boundaries", "10,100,1000,10000"}, -#endif }; for (const auto &iter : immutable_cases) { From 70c4f5e15e7f9cc1cc4b9b0e32a1fc836ad570f7 Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Wed, 15 Jan 2025 12:55:47 -0800 Subject: [PATCH 10/18] remove build parameters too --- CMakeLists.txt | 6 ------ 1 file changed, 6 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 11fe463f923..5f4f518d471 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -29,7 +29,6 @@ option(ENABLE_STATIC_LIBSTDCXX "link kvrocks with static library of libstd++ ins option(ENABLE_LUAJIT "enable use of luaJIT instead of lua" ON) option(ENABLE_OPENSSL "enable openssl to support tls connection" OFF) option(ENABLE_IPO "enable interprocedural optimization" ON) -option(ENABLE_HISTOGRAMS "enable histograms to view the operation latencies" OFF) set(SYMBOLIZE_BACKEND "" CACHE STRING "symbolization backend library for cpptrace (libbacktrace, libdwarf, or empty)") set(PORTABLE 0 CACHE STRING "build a portable binary (disable arch-specific optimizations)") # TODO: set ENABLE_NEW_ENCODING to ON when we are ready @@ -289,11 +288,6 @@ if(ENABLE_IPO) endif() endif() -if(ENABLE_HISTOGRAMS) - target_compile_definitions(kvrocks_objs PUBLIC ENABLE_HISTOGRAMS) -endif() - - # kvrocks main target add_executable(kvrocks src/cli/main.cc) target_link_libraries(kvrocks PRIVATE kvrocks_objs ${EXTERNAL_LIBS}) From 38323c793fe036fcacfd87f2bd6bb07efbde6f14 Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Wed, 15 Jan 2025 13:03:28 -0800 Subject: [PATCH 11/18] format fix --- src/stats/stats.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stats/stats.h b/src/stats/stats.h index eea220eeb31..452daf9561a 100644 --- a/src/stats/stats.h +++ b/src/stats/stats.h @@ -96,6 +96,6 @@ class Stats { void TrackInstantaneousMetric(int metric, uint64_t current_reading); uint64_t GetInstantaneousMetric(int metric) const; -private: + private: Config *config_ = nullptr; }; From e8fb0a23becd57389c998f5af89fe39c292d2b01 Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Thu, 16 Jan 2025 16:08:19 -0800 Subject: [PATCH 12/18] use make_shared --- src/server/server.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server/server.cc b/src/server/server.cc index 7442561fedc..fad074956fc 100644 --- a/src/server/server.cc +++ b/src/server/server.cc @@ -69,7 +69,7 @@ Server::Server(engine::Storage *storage, Config *config) if (stats.bucket_boundaries.size() > 0) { // NB: Extra index for the last bucket (Inf) for (std::size_t i{0}; i <= stats.bucket_boundaries.size(); ++i) { - auto bucket_ptr = std::shared_ptr>(new std::atomic(0)); + auto bucket_ptr = std::make_shared>(0); stats.commands_histogram[iter.first].buckets.push_back(bucket_ptr); } From f93973041d0dd79e2d8c23aecd3b7f476d8bf822 Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Thu, 16 Jan 2025 23:13:51 -0800 Subject: [PATCH 13/18] address comment --- src/config/config.cc | 13 +++++++------ src/server/server.cc | 2 +- src/stats/stats.cc | 5 +---- src/stats/stats.h | 5 +---- 4 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/config/config.cc b/src/config/config.cc index ae1ebb5f92f..3762178c537 100644 --- a/src/config/config.cc +++ b/src/config/config.cc @@ -763,14 +763,15 @@ void Config::initFieldCallback() { if (buckets.size() < 1) { return Status::OK(); } - std::transform(buckets.begin(), buckets.end(), std::back_inserter(histogram_bucket_boundaries), - [](const std::string &val) { return std::stod(val); }); - if (histogram_bucket_boundaries.size() != buckets.size()) { - return {Status::NotOK, "All values for the bucket must be double or integer values"}; + for (const auto &bucket_val : buckets) { + auto parse_result = ParseFloat(bucket_val); + if (!parse_result) { + return {Status::NotOK, "The values in the bucket list must be double or integer."}; + } + histogram_bucket_boundaries.push_back(*parse_result); } - if (!std::is_sorted(histogram_bucket_boundaries.begin(), histogram_bucket_boundaries.end())) { - return {Status::NotOK, "The values for the histogram must be sorted"}; + return {Status::NotOK, "The values for the histogram must be sorted."}; } return Status::OK(); }}, diff --git a/src/server/server.cc b/src/server/server.cc index fad074956fc..f8e7ee3a836 100644 --- a/src/server/server.cc +++ b/src/server/server.cc @@ -52,7 +52,7 @@ #include "worker.h" Server::Server(engine::Storage *storage, Config *config) - : stats(config), + : stats(config->histogram_bucket_boundaries), storage(storage), indexer(storage), index_mgr(&indexer, storage), diff --git a/src/stats/stats.cc b/src/stats/stats.cc index b4de1701472..5b49a4c5851 100644 --- a/src/stats/stats.cc +++ b/src/stats/stats.cc @@ -26,7 +26,7 @@ #include "fmt/format.h" #include "time_util.h" -Stats::Stats(Config *config) : config_(config) { +Stats::Stats(std::vector bucket_boundaries) : bucket_boundaries(bucket_boundaries) { for (int i = 0; i < STATS_METRIC_COUNT; i++) { InstMetric im; im.last_sample_time_ms = 0; @@ -37,9 +37,6 @@ Stats::Stats(Config *config) : config_(config) { } inst_metrics.push_back(im); } - if (config_->histogram_bucket_boundaries.size() > 0) { - bucket_boundaries = config_->histogram_bucket_boundaries; - } } #if defined(__APPLE__) diff --git a/src/stats/stats.h b/src/stats/stats.h index 452daf9561a..89fcd4a8c78 100644 --- a/src/stats/stats.h +++ b/src/stats/stats.h @@ -83,7 +83,7 @@ class Stats { BucketBoundaries bucket_boundaries; std::map commands_histogram; - explicit Stats(Config *config); + explicit Stats(std::vector histogram_bucket_boundaries); void IncrCalls(const std::string &command_name); void IncrLatency(uint64_t latency, const std::string &command_name); @@ -95,7 +95,4 @@ class Stats { static int64_t GetMemoryRSS(); void TrackInstantaneousMetric(int metric, uint64_t current_reading); uint64_t GetInstantaneousMetric(int metric) const; - - private: - Config *config_ = nullptr; }; From d695ba44a6455c0023f69ec5c996bf73c912be4c Mon Sep 17 00:00:00 2001 From: Rabun Kosar <54301319+rabunkosar-dd@users.noreply.github.com> Date: Thu, 16 Jan 2025 23:18:55 -0800 Subject: [PATCH 14/18] Update src/stats/stats.cc Co-authored-by: Twice --- src/stats/stats.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stats/stats.cc b/src/stats/stats.cc index 5b49a4c5851..5baea34d6f6 100644 --- a/src/stats/stats.cc +++ b/src/stats/stats.cc @@ -26,7 +26,7 @@ #include "fmt/format.h" #include "time_util.h" -Stats::Stats(std::vector bucket_boundaries) : bucket_boundaries(bucket_boundaries) { +Stats::Stats(std::vector bucket_boundaries) : bucket_boundaries(std::move(bucket_boundaries)) { for (int i = 0; i < STATS_METRIC_COUNT; i++) { InstMetric im; im.last_sample_time_ms = 0; From 9826973ad3ba3d8b6b8ff5d027302cfda8039125 Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Thu, 16 Jan 2025 23:27:55 -0800 Subject: [PATCH 15/18] format and remove unused header --- src/config/config.cc | 10 +++++----- src/stats/stats.h | 2 -- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/config/config.cc b/src/config/config.cc index 3762178c537..0102ee8820f 100644 --- a/src/config/config.cc +++ b/src/config/config.cc @@ -764,11 +764,11 @@ void Config::initFieldCallback() { return Status::OK(); } for (const auto &bucket_val : buckets) { - auto parse_result = ParseFloat(bucket_val); - if (!parse_result) { - return {Status::NotOK, "The values in the bucket list must be double or integer."}; - } - histogram_bucket_boundaries.push_back(*parse_result); + auto parse_result = ParseFloat(bucket_val); + if (!parse_result) { + return {Status::NotOK, "The values in the bucket list must be double or integer."}; + } + histogram_bucket_boundaries.push_back(*parse_result); } if (!std::is_sorted(histogram_bucket_boundaries.begin(), histogram_bucket_boundaries.end())) { return {Status::NotOK, "The values for the histogram must be sorted."}; diff --git a/src/stats/stats.h b/src/stats/stats.h index 89fcd4a8c78..4ae7c207c23 100644 --- a/src/stats/stats.h +++ b/src/stats/stats.h @@ -29,8 +29,6 @@ #include #include -#include "config/config.h" - enum StatsMetricFlags { STATS_METRIC_COMMAND = 0, // Number of commands executed STATS_METRIC_NET_INPUT, // Bytes read to network From 411e18db150540bd4a8d61cb60d93f661b08821c Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Sun, 19 Jan 2025 21:21:23 -0800 Subject: [PATCH 16/18] include memory --- src/stats/stats.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/stats/stats.h b/src/stats/stats.h index 4ae7c207c23..aee7f57c74e 100644 --- a/src/stats/stats.h +++ b/src/stats/stats.h @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include From 65cb29b59d50f279bc605935f4651732a346e5cf Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Tue, 21 Jan 2025 00:40:41 -0800 Subject: [PATCH 17/18] fix unit test --- tests/gocase/unit/info/info_test.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/tests/gocase/unit/info/info_test.go b/tests/gocase/unit/info/info_test.go index 6b706379a04..128e57748f3 100644 --- a/tests/gocase/unit/info/info_test.go +++ b/tests/gocase/unit/info/info_test.go @@ -27,9 +27,8 @@ import ( "testing" "time" - "github.com/redis/go-redis/v9" - "github.com/apache/kvrocks/tests/gocase/util" + "github.com/redis/go-redis/v9" "github.com/stretchr/testify/require" ) @@ -108,20 +107,17 @@ func TestInfo(t *testing.T) { }) t.Run("get command latencies via histogram INFO - histogram-bucket-boundaries", func(t *testing.T) { - output := util.FindInfoEntry(rdb0, "cmdstathist", "cmdstathist_info") - if len(output) == 0 { - t.SkipNow() - } + output := util.FindInfoEntry(rdb0, "cmdstathist_info", "commandstats") splitValues := strings.FieldsFunc(output, func(r rune) bool { return r == '=' || r == ',' }) // expected: 10=..,20=..,30=..,50=..,inf=..,sum=...,count=.. - require.GreaterOrEqual(t, len(splitValues), 15) + require.GreaterOrEqual(t, len(splitValues), 14) require.Contains(t, splitValues, "sum") require.Contains(t, splitValues, "count") - require.Contains(t, splitValues, "info") + require.Contains(t, splitValues, "inf") }) } From 76a8a964d8f32ff79d66619c7a6e97b8f39b753e Mon Sep 17 00:00:00 2001 From: rabunkosar-dd Date: Tue, 21 Jan 2025 10:45:41 -0800 Subject: [PATCH 18/18] use unique ptr --- src/server/server.cc | 4 +--- src/stats/stats.h | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/server/server.cc b/src/server/server.cc index f8e7ee3a836..e809444baa3 100644 --- a/src/server/server.cc +++ b/src/server/server.cc @@ -69,9 +69,7 @@ Server::Server(engine::Storage *storage, Config *config) if (stats.bucket_boundaries.size() > 0) { // NB: Extra index for the last bucket (Inf) for (std::size_t i{0}; i <= stats.bucket_boundaries.size(); ++i) { - auto bucket_ptr = std::make_shared>(0); - - stats.commands_histogram[iter.first].buckets.push_back(bucket_ptr); + stats.commands_histogram[iter.first].buckets.push_back(std::make_unique>(0)); } stats.commands_histogram[iter.first].calls = 0; stats.commands_histogram[iter.first].sum = 0; diff --git a/src/stats/stats.h b/src/stats/stats.h index aee7f57c74e..0bae042d640 100644 --- a/src/stats/stats.h +++ b/src/stats/stats.h @@ -47,7 +47,7 @@ constexpr int STATS_METRIC_SAMPLES = 16; // Number of samples per metric // Experimental part to support histograms for cmd statistics struct CommandHistogram { - std::vector>> buckets; + std::vector>> buckets; std::atomic calls; std::atomic sum; };