diff --git a/source/common/memory/BUILD b/source/common/memory/BUILD index 2b22e25f9f28d..83a46fa4fcc20 100644 --- a/source/common/memory/BUILD +++ b/source/common/memory/BUILD @@ -34,5 +34,6 @@ envoy_cc_library( "//include/envoy/event:dispatcher_interface", "//include/envoy/server:overload_manager_interface", "//include/envoy/stats:stats_interface", + "//source/common/stats:symbol_table_lib", ], ) diff --git a/source/common/memory/heap_shrinker.cc b/source/common/memory/heap_shrinker.cc index 2582cfe76c84f..da0413bbfce3a 100644 --- a/source/common/memory/heap_shrinker.cc +++ b/source/common/memory/heap_shrinker.cc @@ -1,6 +1,7 @@ #include "common/memory/heap_shrinker.h" #include "common/memory/utils.h" +#include "common/stats/symbol_table_impl.h" #include "absl/strings/str_cat.h" @@ -18,7 +19,9 @@ HeapShrinker::HeapShrinker(Event::Dispatcher& dispatcher, Server::OverloadManage [this](Server::OverloadActionState state) { active_ = (state == Server::OverloadActionState::Active); })) { - shrink_counter_ = &stats.counter(absl::StrCat("overload.", action_name, ".shrink_count")); + Envoy::Stats::StatNameManagedStorage stat_name( + absl::StrCat("overload.", action_name, ".shrink_count"), stats.symbolTable()); + shrink_counter_ = &stats.counterFromStatName(stat_name.statName()); timer_ = dispatcher.createTimer([this] { shrinkHeap(); timer_->enableTimer(kTimerInterval); diff --git a/source/extensions/filters/http/dynamo/BUILD b/source/extensions/filters/http/dynamo/BUILD index e1ba977284758..048be93233952 100644 --- a/source/extensions/filters/http/dynamo/BUILD +++ b/source/extensions/filters/http/dynamo/BUILD @@ -17,7 +17,7 @@ envoy_cc_library( hdrs = ["dynamo_filter.h"], deps = [ ":dynamo_request_parser_lib", - ":dynamo_utility_lib", + ":dynamo_stats_lib", "//include/envoy/http:filter_interface", "//include/envoy/runtime:runtime_interface", "//source/common/buffer:buffer_lib", @@ -37,16 +37,6 @@ envoy_cc_library( ], ) -envoy_cc_library( - name = "dynamo_utility_lib", - srcs = ["dynamo_utility.cc"], - hdrs = ["dynamo_utility.h"], - deps = [ - "//include/envoy/stats:stats_interface", - "//source/common/stats:stats_lib", - ], -) - envoy_cc_library( name = "config", srcs = ["config.cc"], @@ -59,3 +49,14 @@ envoy_cc_library( "//source/extensions/filters/http/common:empty_http_filter_config_lib", ], ) + +envoy_cc_library( + name = "dynamo_stats_lib", + srcs = ["dynamo_stats.cc"], + hdrs = ["dynamo_stats.h"], + deps = [ + ":dynamo_request_parser_lib", + "//include/envoy/stats:stats_interface", + "//source/common/stats:symbol_table_lib", + ], +) diff --git a/source/extensions/filters/http/dynamo/config.cc b/source/extensions/filters/http/dynamo/config.cc index 7355e46760f63..77dde48be63cd 100644 --- a/source/extensions/filters/http/dynamo/config.cc +++ b/source/extensions/filters/http/dynamo/config.cc @@ -5,6 +5,7 @@ #include "envoy/registry/registry.h" #include "extensions/filters/http/dynamo/dynamo_filter.h" +#include "extensions/filters/http/dynamo/dynamo_stats.h" namespace Envoy { namespace Extensions { @@ -14,9 +15,10 @@ namespace Dynamo { Http::FilterFactoryCb DynamoFilterConfig::createFilter(const std::string& stat_prefix, Server::Configuration::FactoryContext& context) { - return [&context, stat_prefix](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamFilter(Http::StreamFilterSharedPtr{new Dynamo::DynamoFilter( - context.runtime(), stat_prefix, context.scope(), context.dispatcher().timeSource())}); + auto stats = std::make_shared(context.scope(), stat_prefix); + return [&context, stat_prefix, stats](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamFilter(std::make_shared( + context.runtime(), stats, context.dispatcher().timeSource())); }; } diff --git a/source/extensions/filters/http/dynamo/dynamo_filter.cc b/source/extensions/filters/http/dynamo/dynamo_filter.cc index cb7ee4c357025..c9072aaf5b1ac 100644 --- a/source/extensions/filters/http/dynamo/dynamo_filter.cc +++ b/source/extensions/filters/http/dynamo/dynamo_filter.cc @@ -15,7 +15,7 @@ #include "common/json/json_loader.h" #include "extensions/filters/http/dynamo/dynamo_request_parser.h" -#include "extensions/filters/http/dynamo/dynamo_utility.h" +#include "extensions/filters/http/dynamo/dynamo_stats.h" namespace Envoy { namespace Extensions { @@ -62,7 +62,7 @@ void DynamoFilter::onDecodeComplete(const Buffer::Instance& data) { table_descriptor_ = RequestParser::parseTable(operation_, *json_body); } catch (const Json::Exception& jsonEx) { // Body parsing failed. This should not happen, just put a stat for that. - scope_.counter(fmt::format("{}invalid_req_body", stat_prefix_)).inc(); + stats_->counter({stats_->invalid_req_body_}).inc(); } } } @@ -89,7 +89,7 @@ void DynamoFilter::onEncodeComplete(const Buffer::Instance& data) { } } catch (const Json::Exception&) { // Body parsing failed. This should not happen, just put a stat for that. - scope_.counter(fmt::format("{}invalid_resp_body", stat_prefix_)).inc(); + stats_->counter({stats_->invalid_resp_body_}).inc(); } } } @@ -158,15 +158,15 @@ void DynamoFilter::chargeBasicStats(uint64_t status) { if (!operation_.empty()) { chargeStatsPerEntity(operation_, "operation", status); } else { - scope_.counter(fmt::format("{}operation_missing", stat_prefix_)).inc(); + stats_->counter({stats_->operation_missing_}).inc(); } if (!table_descriptor_.table_name.empty()) { chargeStatsPerEntity(table_descriptor_.table_name, "table", status); } else if (table_descriptor_.is_single_table) { - scope_.counter(fmt::format("{}table_missing", stat_prefix_)).inc(); + stats_->counter({stats_->table_missing_}).inc(); } else { - scope_.counter(fmt::format("{}multiple_tables", stat_prefix_)).inc(); + stats_->counter({stats_->multiple_tables_}).inc(); } } @@ -175,29 +175,23 @@ void DynamoFilter::chargeStatsPerEntity(const std::string& entity, const std::st std::chrono::milliseconds latency = std::chrono::duration_cast( time_source_.monotonicTime() - start_decode_); - std::string group_string = - Http::CodeUtility::groupStringForResponseCode(static_cast(status)); + size_t group_index = DynamoStats::groupIndex(status); + const Stats::StatName entity_type_name = stats_->getStatName(entity_type); + const Stats::StatName entity_name = stats_->getStatName(entity); + const Stats::StatName total_name = + stats_->getStatName(absl::StrCat("upstream_rq_total_", status)); + const Stats::StatName time_name = stats_->getStatName(absl::StrCat("upstream_rq_time_", status)); - scope_.counter(fmt::format("{}{}.{}.upstream_rq_total", stat_prefix_, entity_type, entity)).inc(); - scope_ - .counter(fmt::format("{}{}.{}.upstream_rq_total_{}", stat_prefix_, entity_type, entity, - group_string)) - .inc(); - scope_ - .counter(fmt::format("{}{}.{}.upstream_rq_total_{}", stat_prefix_, entity_type, entity, - std::to_string(status))) - .inc(); + stats_->counter({entity_type_name, entity_name, stats_->upstream_rq_total_}).inc(); + const Stats::StatName total_group = stats_->upstream_rq_total_groups_[group_index]; + stats_->counter({entity_type_name, entity_name, total_group}).inc(); + stats_->counter({entity_type_name, entity_name, total_name}).inc(); - scope_.histogram(fmt::format("{}{}.{}.upstream_rq_time", stat_prefix_, entity_type, entity)) - .recordValue(latency.count()); - scope_ - .histogram(fmt::format("{}{}.{}.upstream_rq_time_{}", stat_prefix_, entity_type, entity, - group_string)) - .recordValue(latency.count()); - scope_ - .histogram(fmt::format("{}{}.{}.upstream_rq_time_{}", stat_prefix_, entity_type, entity, - std::to_string(status))) + stats_->histogram({entity_type_name, entity_name, stats_->upstream_rq_time_}) .recordValue(latency.count()); + const Stats::StatName time_group = stats_->upstream_rq_time_groups_[group_index]; + stats_->histogram({entity_type_name, entity_name, time_group}).recordValue(latency.count()); + stats_->histogram({entity_type_name, entity_name, time_name}).recordValue(latency.count()); } void DynamoFilter::chargeUnProcessedKeysStats(const Json::Object& json_body) { @@ -205,9 +199,9 @@ void DynamoFilter::chargeUnProcessedKeysStats(const Json::Object& json_body) { // complete apart of the batch operation. Only the table names will be logged for errors. std::vector unprocessed_tables = RequestParser::parseBatchUnProcessedKeys(json_body); for (const std::string& unprocessed_table : unprocessed_tables) { - scope_ - .counter( - fmt::format("{}error.{}.BatchFailureUnprocessedKeys", stat_prefix_, unprocessed_table)) + stats_ + ->counter({stats_->error_, stats_->getStatName(unprocessed_table), + stats_->batch_failure_unprocessed_keys_}) .inc(); } } @@ -217,15 +211,15 @@ void DynamoFilter::chargeFailureSpecificStats(const Json::Object& json_body) { if (!error_type.empty()) { if (table_descriptor_.table_name.empty()) { - scope_.counter(fmt::format("{}error.no_table.{}", stat_prefix_, error_type)).inc(); + stats_->counter({stats_->error_, stats_->no_table_, stats_->getStatName(error_type)}).inc(); } else { - scope_ - .counter( - fmt::format("{}error.{}.{}", stat_prefix_, table_descriptor_.table_name, error_type)) + stats_ + ->counter({stats_->error_, stats_->getStatName(table_descriptor_.table_name), + stats_->getStatName(error_type)}) .inc(); } } else { - scope_.counter(fmt::format("{}empty_response_body", stat_prefix_)).inc(); + stats_->counter({stats_->empty_response_body_}).inc(); } } @@ -237,9 +231,10 @@ void DynamoFilter::chargeTablePartitionIdStats(const Json::Object& json_body) { std::vector partitions = RequestParser::parsePartitions(json_body); for (const RequestParser::PartitionDescriptor& partition : partitions) { - std::string scope_string = Utility::buildPartitionStatString( - stat_prefix_, table_descriptor_.table_name, operation_, partition.partition_id_); - scope_.counter(scope_string).add(partition.capacity_); + stats_ + ->buildPartitionStatCounter(table_descriptor_.table_name, operation_, + partition.partition_id_) + .add(partition.capacity_); } } diff --git a/source/extensions/filters/http/dynamo/dynamo_filter.h b/source/extensions/filters/http/dynamo/dynamo_filter.h index 3de6e378db1fc..8702444a11508 100644 --- a/source/extensions/filters/http/dynamo/dynamo_filter.h +++ b/source/extensions/filters/http/dynamo/dynamo_filter.h @@ -10,6 +10,7 @@ #include "common/json/json_loader.h" #include "extensions/filters/http/dynamo/dynamo_request_parser.h" +#include "extensions/filters/http/dynamo/dynamo_stats.h" namespace Envoy { namespace Extensions { @@ -24,10 +25,8 @@ namespace Dynamo { */ class DynamoFilter : public Http::StreamFilter { public: - DynamoFilter(Runtime::Loader& runtime, const std::string& stat_prefix, Stats::Scope& scope, - TimeSource& time_system) - : runtime_(runtime), stat_prefix_(stat_prefix + "dynamodb."), scope_(scope), - time_source_(time_system) { + DynamoFilter(Runtime::Loader& runtime, const DynamoStatsSharedPtr& stats, TimeSource& time_source) + : runtime_(runtime), stats_(stats), time_source_(time_source) { enabled_ = runtime_.snapshot().featureEnabled("dynamodb.filter_enabled", 100); } @@ -68,8 +67,7 @@ class DynamoFilter : public Http::StreamFilter { void chargeTablePartitionIdStats(const Json::Object& json_body); Runtime::Loader& runtime_; - std::string stat_prefix_; - Stats::Scope& scope_; + const DynamoStatsSharedPtr stats_; bool enabled_{}; std::string operation_{}; diff --git a/source/extensions/filters/http/dynamo/dynamo_request_parser.cc b/source/extensions/filters/http/dynamo/dynamo_request_parser.cc index 3ec282c784283..357357f067eba 100644 --- a/source/extensions/filters/http/dynamo/dynamo_request_parser.cc +++ b/source/extensions/filters/http/dynamo/dynamo_request_parser.cc @@ -186,6 +186,24 @@ RequestParser::parsePartitions(const Json::Object& json_data) { return partition_descriptors; } +void RequestParser::forEachStatString(const StringFn& fn) { + for (const std::string& str : SINGLE_TABLE_OPERATIONS) { + fn(str); + } + for (const std::string& str : SUPPORTED_ERROR_TYPES) { + fn(str); + } + for (const std::string& str : BATCH_OPERATIONS) { + fn(str); + } + for (const std::string& str : TRANSACT_OPERATIONS) { + fn(str); + } + for (const std::string& str : TRANSACT_ITEM_OPERATIONS) { + fn(str); + } +} + } // namespace Dynamo } // namespace HttpFilters } // namespace Extensions diff --git a/source/extensions/filters/http/dynamo/dynamo_request_parser.h b/source/extensions/filters/http/dynamo/dynamo_request_parser.h index c7bd669b0bec8..8c63b32fd6413 100644 --- a/source/extensions/filters/http/dynamo/dynamo_request_parser.h +++ b/source/extensions/filters/http/dynamo/dynamo_request_parser.h @@ -97,6 +97,17 @@ class RequestParser { */ static std::vector parsePartitions(const Json::Object& json_data); + using StringFn = std::function; + + /** + * Calls a function for every string that is likely to be included as a token + * in a stat. This is not functionally necessary, but can reduce potentially + * contented access to create entries in the symbol table in the hot path. + * + * @param fn the function to call for every potential stat name. + */ + static void forEachStatString(const StringFn& fn); + private: static const Http::LowerCaseString X_AMZ_TARGET; static const std::vector SINGLE_TABLE_OPERATIONS; diff --git a/source/extensions/filters/http/dynamo/dynamo_stats.cc b/source/extensions/filters/http/dynamo/dynamo_stats.cc new file mode 100644 index 0000000000000..51e12d96d452a --- /dev/null +++ b/source/extensions/filters/http/dynamo/dynamo_stats.cc @@ -0,0 +1,109 @@ +#include "extensions/filters/http/dynamo/dynamo_stats.h" + +#include +#include +#include + +#include "envoy/stats/scope.h" + +#include "common/stats/symbol_table_impl.h" + +#include "extensions/filters/http/dynamo/dynamo_request_parser.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace Dynamo { + +DynamoStats::DynamoStats(Stats::Scope& scope, const std::string& prefix) + : scope_(scope), pool_(scope.symbolTable()), prefix_(pool_.add(prefix + "dynamodb")), + batch_failure_unprocessed_keys_(pool_.add("BatchFailureUnprocessedKeys")), + capacity_(pool_.add("capacity")), empty_response_body_(pool_.add("empty_response_body")), + error_(pool_.add("error")), invalid_req_body_(pool_.add("invalid_req_body")), + invalid_resp_body_(pool_.add("invalid_resp_body")), + multiple_tables_(pool_.add("multiple_tables")), no_table_(pool_.add("no_table")), + operation_missing_(pool_.add("operation_missing")), table_(pool_.add("table")), + table_missing_(pool_.add("table_missing")), upstream_rq_time_(pool_.add("upstream_rq_time")), + upstream_rq_total_(pool_.add("upstream_rq_total")) { + upstream_rq_total_groups_[0] = pool_.add("upstream_rq_total_unknown"); + upstream_rq_time_groups_[0] = pool_.add("upstream_rq_time_unknown"); + for (size_t i = 1; i < DynamoStats::NumGroupEntries; ++i) { + upstream_rq_total_groups_[i] = pool_.add(fmt::format("upstream_rq_total_{}xx", i)); + upstream_rq_time_groups_[i] = pool_.add(fmt::format("upstream_rq_time_{}xx", i)); + } + RequestParser::forEachStatString([this](const std::string& str) { + // Thread annotation does not realize this function is only called from the + // constructor, so we need to lock the mutex. It's easier to just do that + // and it's no real penalty. + absl::MutexLock lock(&mutex_); + builtin_stat_names_[str] = pool_.add(str); + }); + builtin_stat_names_[""] = Stats::StatName(); +} + +Stats::SymbolTable::StoragePtr DynamoStats::addPrefix(const std::vector& names) { + std::vector names_with_prefix{prefix_}; + names_with_prefix.reserve(names.end() - names.begin()); + names_with_prefix.insert(names_with_prefix.end(), names.begin(), names.end()); + return scope_.symbolTable().join(names_with_prefix); +} + +Stats::Counter& DynamoStats::counter(const std::vector& names) { + const Stats::SymbolTable::StoragePtr stat_name_storage = addPrefix(names); + return scope_.counterFromStatName(Stats::StatName(stat_name_storage.get())); +} + +Stats::Histogram& DynamoStats::histogram(const std::vector& names) { + const Stats::SymbolTable::StoragePtr stat_name_storage = addPrefix(names); + return scope_.histogramFromStatName(Stats::StatName(stat_name_storage.get())); +} + +Stats::Counter& DynamoStats::buildPartitionStatCounter(const std::string& table_name, + const std::string& operation, + const std::string& partition_id) { + // Use the last 7 characters of the partition id. + absl::string_view id_last_7 = absl::string_view(partition_id).substr(partition_id.size() - 7); + const Stats::SymbolTable::StoragePtr stat_name_storage = + addPrefix({table_, getStatName(table_name), capacity_, getStatName(operation), + getStatName(absl::StrCat("__partition_id=", id_last_7))}); + return scope_.counterFromStatName(Stats::StatName(stat_name_storage.get())); +} + +size_t DynamoStats::groupIndex(uint64_t status) { + size_t index = status / 100; + if (index >= NumGroupEntries) { + index = 0; // status-code 600 or higher is unknown. + } + return index; +} + +Stats::StatName DynamoStats::getStatName(const std::string& token) { + // The Dynamo system has a few well-known tokens that we have stored during + // construction, and we can access StatNames for those without a lock. Note + // that we only mutate builtin_stat_names_ during construction. + auto iter = builtin_stat_names_.find(token); + if (iter != builtin_stat_names_.end()) { + return iter->second; + } + + // However, some of the tokens come from json data received during requests, + // so we need to store these in a mutex-protected map. Once we hold the mutex, + // we can check dynamic_stat_names_. If it's missing we'll have to add it + // to the symbol table, whose mutex is more likely to be contended, and then + // store it in dynamic_stat_names. + // + // TODO(jmarantz): Potential perf issue here with contention, both on this + // mutex and also the SymbolTable mutex which must be taken during + // StatNamePool::add(). + absl::MutexLock lock(&mutex_); + Stats::StatName& stat_name = dynamic_stat_names_[token]; + if (stat_name.empty()) { // Note that builtin_stat_names_ already has one for "". + stat_name = pool_.add(token); + } + return stat_name; +} + +} // namespace Dynamo +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/dynamo/dynamo_stats.h b/source/extensions/filters/http/dynamo/dynamo_stats.h new file mode 100644 index 0000000000000..57671514e5812 --- /dev/null +++ b/source/extensions/filters/http/dynamo/dynamo_stats.h @@ -0,0 +1,73 @@ +#pragma once + +#include +#include + +#include "envoy/stats/scope.h" + +#include "common/stats/symbol_table_impl.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace Dynamo { + +class DynamoStats { +public: + DynamoStats(Stats::Scope& scope, const std::string& prefix); + + Stats::Counter& counter(const std::vector& names); + Stats::Histogram& histogram(const std::vector& names); + + /** + * Creates the partition id stats string. The stats format is + * "table..capacity..__partition_id=". + * Partition ids and dynamodb table names can be long. To satisfy the string + * length, we truncate, taking only the last 7 characters of the partition id. + */ + Stats::Counter& buildPartitionStatCounter(const std::string& table_name, + const std::string& operation, + const std::string& partition_id); + + static size_t groupIndex(uint64_t status); + + Stats::StatName getStatName(const std::string& str); + +private: + Stats::SymbolTable::StoragePtr addPrefix(const std::vector& names); + + Stats::Scope& scope_; + Stats::StatNamePool pool_ GUARDED_BY(mutex_); + const Stats::StatName prefix_; + absl::Mutex mutex_; + using StringStatNameMap = absl::flat_hash_map; + StringStatNameMap builtin_stat_names_; + StringStatNameMap dynamic_stat_names_ GUARDED_BY(mutex_); + +public: + const Stats::StatName batch_failure_unprocessed_keys_; + const Stats::StatName capacity_; + const Stats::StatName empty_response_body_; + const Stats::StatName error_; + const Stats::StatName invalid_req_body_; + const Stats::StatName invalid_resp_body_; + const Stats::StatName multiple_tables_; + const Stats::StatName no_table_; + const Stats::StatName operation_missing_; + const Stats::StatName table_; + const Stats::StatName table_missing_; + const Stats::StatName upstream_rq_time_; + const Stats::StatName upstream_rq_total_; + const Stats::StatName upstream_rq_unknown_; + + // Keep group codes for HTTP status codes through the 500s. + static constexpr size_t NumGroupEntries = 6; + Stats::StatName upstream_rq_total_groups_[NumGroupEntries]; + Stats::StatName upstream_rq_time_groups_[NumGroupEntries]; +}; +using DynamoStatsSharedPtr = std::shared_ptr; + +} // namespace Dynamo +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/dynamo/dynamo_utility.cc b/source/extensions/filters/http/dynamo/dynamo_utility.cc deleted file mode 100644 index a71408439fc31..0000000000000 --- a/source/extensions/filters/http/dynamo/dynamo_utility.cc +++ /dev/null @@ -1,27 +0,0 @@ -#include "extensions/filters/http/dynamo/dynamo_utility.h" - -#include - -#include "common/common/fmt.h" - -namespace Envoy { -namespace Extensions { -namespace HttpFilters { -namespace Dynamo { - -std::string Utility::buildPartitionStatString(const std::string& stat_prefix, - const std::string& table_name, - const std::string& operation, - const std::string& partition_id) { - // Use the last 7 characters of the partition id. - std::string stats_partition_postfix = - fmt::format(".capacity.{}.__partition_id={}", operation, - partition_id.substr(partition_id.size() - 7, partition_id.size())); - std::string stats_table_prefix = fmt::format("{}table.{}", stat_prefix, table_name); - return fmt::format("{}{}", stats_table_prefix, stats_partition_postfix); -} - -} // namespace Dynamo -} // namespace HttpFilters -} // namespace Extensions -} // namespace Envoy diff --git a/source/extensions/filters/http/dynamo/dynamo_utility.h b/source/extensions/filters/http/dynamo/dynamo_utility.h deleted file mode 100644 index 87b4bd0bc119f..0000000000000 --- a/source/extensions/filters/http/dynamo/dynamo_utility.h +++ /dev/null @@ -1,32 +0,0 @@ -#pragma once - -#include - -namespace Envoy { -namespace Extensions { -namespace HttpFilters { -namespace Dynamo { - -class Utility { -public: - /** - * Creates the partition id stats string. - * The stats format is - * "table..capacity..__partition_id=". - * Partition ids and dynamodb table names can be long. To satisfy the string length, - * we truncate in two ways: - * 1. We only take the last 7 characters of the partition id. - * 2. If the stats string with is longer than the stats MAX_NAME_SIZE, we will - * truncate the table name to - * fit the size requirements. - */ - static std::string buildPartitionStatString(const std::string& stat_prefix, - const std::string& table_name, - const std::string& operation, - const std::string& partition_id); -}; - -} // namespace Dynamo -} // namespace HttpFilters -} // namespace Extensions -} // namespace Envoy diff --git a/test/extensions/filters/http/dynamo/BUILD b/test/extensions/filters/http/dynamo/BUILD index 01797af74456c..9d87a0531b405 100644 --- a/test/extensions/filters/http/dynamo/BUILD +++ b/test/extensions/filters/http/dynamo/BUILD @@ -40,12 +40,12 @@ envoy_extension_cc_test( ) envoy_extension_cc_test( - name = "dynamo_utility_test", - srcs = ["dynamo_utility_test.cc"], + name = "dynamo_stats_test", + srcs = ["dynamo_stats_test.cc"], extension_name = "envoy.filters.http.dynamo", deps = [ "//source/common/stats:stats_lib", - "//source/extensions/filters/http/dynamo:dynamo_utility_lib", + "//source/extensions/filters/http/dynamo:dynamo_stats_lib", "//test/mocks/stats:stats_mocks", ], ) diff --git a/test/extensions/filters/http/dynamo/dynamo_filter_test.cc b/test/extensions/filters/http/dynamo/dynamo_filter_test.cc index a21e4734963fa..eb93be0c670c4 100644 --- a/test/extensions/filters/http/dynamo/dynamo_filter_test.cc +++ b/test/extensions/filters/http/dynamo/dynamo_filter_test.cc @@ -34,7 +34,8 @@ class DynamoFilterTest : public testing::Test { .WillByDefault(Return(enabled)); EXPECT_CALL(loader_.snapshot_, featureEnabled("dynamodb.filter_enabled", 100)); - filter_ = std::make_unique(loader_, stat_prefix_, stats_, + auto stats = std::make_shared(stats_, "prefix."); + filter_ = std::make_unique(loader_, stats, decoder_callbacks_.dispatcher().timeSource()); filter_->setDecoderFilterCallbacks(decoder_callbacks_); @@ -43,10 +44,10 @@ class DynamoFilterTest : public testing::Test { ~DynamoFilterTest() override { filter_->onDestroy(); } + NiceMock stats_; std::unique_ptr filter_; NiceMock loader_; std::string stat_prefix_{"prefix."}; - NiceMock stats_; NiceMock decoder_callbacks_; NiceMock encoder_callbacks_; }; diff --git a/test/extensions/filters/http/dynamo/dynamo_utility_test.cc b/test/extensions/filters/http/dynamo/dynamo_stats_test.cc similarity index 63% rename from test/extensions/filters/http/dynamo/dynamo_utility_test.cc rename to test/extensions/filters/http/dynamo/dynamo_stats_test.cc index b00fd773e036d..0458c5dc3879d 100644 --- a/test/extensions/filters/http/dynamo/dynamo_utility_test.cc +++ b/test/extensions/filters/http/dynamo/dynamo_stats_test.cc @@ -1,6 +1,6 @@ #include -#include "extensions/filters/http/dynamo/dynamo_utility.h" +#include "extensions/filters/http/dynamo/dynamo_stats.h" #include "test/mocks/stats/mocks.h" @@ -17,40 +17,49 @@ namespace HttpFilters { namespace Dynamo { namespace { -TEST(DynamoUtility, PartitionIdStatString) { +TEST(DynamoStats, PartitionIdStatString) { + Stats::IsolatedStoreImpl store; + auto build_partition_string = + [&store](const std::string& stat_prefix, const std::string& table_name, + const std::string& operation, const std::string& partition_id) -> std::string { + DynamoStats stats(store, stat_prefix); + Stats::Counter& counter = stats.buildPartitionStatCounter(table_name, operation, partition_id); + return counter.name(); + }; + { - std::string stat_prefix = "stat.prefix."; + std::string stats_prefix = "prefix."; std::string table_name = "locations"; std::string operation = "GetItem"; std::string partition_id = "6235c781-1d0d-47a3-a4ea-eec04c5883ca"; std::string partition_stat_string = - Utility::buildPartitionStatString(stat_prefix, table_name, operation, partition_id); + build_partition_string(stats_prefix, table_name, operation, partition_id); std::string expected_stat_string = - "stat.prefix.table.locations.capacity.GetItem.__partition_id=c5883ca"; + "prefix.dynamodb.table.locations.capacity.GetItem.__partition_id=c5883ca"; EXPECT_EQ(expected_stat_string, partition_stat_string); } { - std::string stat_prefix = "http.egress_dynamodb_iad.dynamodb."; + std::string stats_prefix = "http.egress_dynamodb_iad."; std::string table_name = "locations-sandbox-partition-test-iad-mytest-really-long-name"; std::string operation = "GetItem"; std::string partition_id = "6235c781-1d0d-47a3-a4ea-eec04c5883ca"; std::string partition_stat_string = - Utility::buildPartitionStatString(stat_prefix, table_name, operation, partition_id); + build_partition_string(stats_prefix, table_name, operation, partition_id); std::string expected_stat_string = "http.egress_dynamodb_iad.dynamodb.table.locations-sandbox-partition-test-iad-mytest-" "really-long-name.capacity.GetItem.__partition_id=c5883ca"; EXPECT_EQ(expected_stat_string, partition_stat_string); } { - std::string stat_prefix = "http.egress_dynamodb_iad.dynamodb."; + std::string stats_prefix = "http.egress_dynamodb_iad."; std::string table_name = "locations-sandbox-partition-test-iad-mytest-rea"; std::string operation = "GetItem"; std::string partition_id = "6235c781-1d0d-47a3-a4ea-eec04c5883ca"; std::string partition_stat_string = - Utility::buildPartitionStatString(stat_prefix, table_name, operation, partition_id); + build_partition_string(stats_prefix, table_name, operation, partition_id); std::string expected_stat_string = "http.egress_dynamodb_iad.dynamodb.table.locations-sandbox-" "partition-test-iad-mytest-rea.capacity.GetItem.__partition_" "id=c5883ca"; diff --git a/tools/check_format.py b/tools/check_format.py index 8a9bece5c4c01..8f5759c5aa968 100755 --- a/tools/check_format.py +++ b/tools/check_format.py @@ -48,9 +48,7 @@ # https://github.com/envoyproxy/envoy/pull/7573 and others. # # TODO(#4196): Eliminate this list completely and then merge #4980. -STAT_FROM_STRING_WHITELIST = ("./source/common/memory/heap_shrinker.cc", - "./source/extensions/filters/http/dynamo/dynamo_filter.cc", - "./source/extensions/filters/http/ext_authz/ext_authz.cc", +STAT_FROM_STRING_WHITELIST = ("./source/extensions/filters/http/ext_authz/ext_authz.cc", "./source/extensions/filters/http/fault/fault_filter.cc", "./source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc", "./source/extensions/filters/network/mongo_proxy/proxy.cc", @@ -584,7 +582,7 @@ def checkSourceLine(line, file_path, reportError): if isInSubdir(file_path, 'source') and file_path.endswith('.cc') and \ not whitelistedForStatFromString(file_path) and \ ('.counter(' in line or '.gauge(' in line or '.histogram(' in line): - reportError("Don't lookup stats by name at runtime; used StatName saved during construction") + reportError("Don't lookup stats by name at runtime; use StatName saved during construction") def checkBuildLine(line, file_path, reportError): diff --git a/tools/check_format_test_helper.py b/tools/check_format_test_helper.py index 2641112ea7224..7309a4bcaff15 100755 --- a/tools/check_format_test_helper.py +++ b/tools/check_format_test_helper.py @@ -210,13 +210,13 @@ def checkFileExpectingOK(filename): "check_format.py") errors += checkUnfixableError( "counter_from_string.cc", - "Don't lookup stats by name at runtime; used StatName saved during construction") + "Don't lookup stats by name at runtime; use StatName saved during construction") errors += checkUnfixableError( "gauge_from_string.cc", - "Don't lookup stats by name at runtime; used StatName saved during construction") + "Don't lookup stats by name at runtime; use StatName saved during construction") errors += checkUnfixableError( "histogram_from_string.cc", - "Don't lookup stats by name at runtime; used StatName saved during construction") + "Don't lookup stats by name at runtime; use StatName saved during construction") errors += fixFileExpectingFailure( "api/missing_package.proto",