diff --git a/source/common/stats/utility.cc b/source/common/stats/utility.cc index 7d4cd4cb2d37f..5ec6d919c78d2 100644 --- a/source/common/stats/utility.cc +++ b/source/common/stats/utility.cc @@ -21,5 +21,18 @@ std::string Utility::sanitizeStatsName(absl::string_view name) { return stats_name; } +absl::optional Utility::findTag(Metric& metric, StatName find_tag_name) { + absl::optional value; + metric.iterateTagStatNames( + [&value, &find_tag_name](Stats::StatName tag_name, Stats::StatName tag_value) -> bool { + if (tag_name == find_tag_name) { + value = tag_value; + return false; + } + return true; + }); + return value; +} + } // namespace Stats } // namespace Envoy diff --git a/source/common/stats/utility.h b/source/common/stats/utility.h index f1cf255e47d73..c5100f8ff9eb5 100644 --- a/source/common/stats/utility.h +++ b/source/common/stats/utility.h @@ -2,7 +2,12 @@ #include +#include "envoy/stats/stats.h" + +#include "common/stats/symbol_table_impl.h" + #include "absl/strings/string_view.h" +#include "absl/types/optional.h" namespace Envoy { namespace Stats { @@ -12,9 +17,23 @@ namespace Stats { */ class Utility { public: - // ':' is a reserved char in statsd. Do a character replacement to avoid costly inline - // translations later. + /** + * ':' is a reserved char in statsd. Do a character replacement to avoid + * costly inline translations later. + * + * @param name the stat name to sanitize. + * @return the sanitized stat name. + */ static std::string sanitizeStatsName(absl::string_view name); + + /** + * Finds a metric tag with the specified name. + * + * @param metric The metric in which the tag is expected to exist. + * @param find_tag_name The name of the tag to search for. + * @return The value of the tag, if found. + */ + static absl::optional findTag(Metric& metric, StatName find_tag_name); }; } // namespace Stats diff --git a/source/extensions/stat_sinks/hystrix/BUILD b/source/extensions/stat_sinks/hystrix/BUILD index 388ad3dde8bca..df82734226759 100644 --- a/source/extensions/stat_sinks/hystrix/BUILD +++ b/source/extensions/stat_sinks/hystrix/BUILD @@ -38,5 +38,6 @@ envoy_cc_library( "//source/common/config:well_known_names", "//source/common/http:headers_lib", "//source/common/stats:symbol_table_lib", + "//source/common/stats:utility_lib", ], ) diff --git a/source/extensions/stat_sinks/hystrix/hystrix.cc b/source/extensions/stat_sinks/hystrix/hystrix.cc index f8442b2c9d52e..ede5d6021c070 100644 --- a/source/extensions/stat_sinks/hystrix/hystrix.cc +++ b/source/extensions/stat_sinks/hystrix/hystrix.cc @@ -11,6 +11,7 @@ #include "common/common/logger.h" #include "common/config/well_known_names.h" #include "common/http/headers.h" +#include "common/stats/utility.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_split.h" @@ -267,8 +268,10 @@ const std::string HystrixSink::printRollingWindows() { HystrixSink::HystrixSink(Server::Instance& server, const uint64_t num_buckets) : server_(server), current_index_(num_buckets > 0 ? num_buckets : DEFAULT_NUM_BUCKETS), - window_size_(current_index_ + 1), - cluster_upstream_rq_time_("cluster.upstream_rq_time", server.stats().symbolTable()) { + window_size_(current_index_ + 1), stat_name_pool_(server.stats().symbolTable()), + cluster_name_(stat_name_pool_.add(Config::TagNames::get().CLUSTER_NAME)), + cluster_upstream_rq_time_(stat_name_pool_.add("cluster.upstream_rq_time")) { + Server::Admin& admin = server_.admin(); ENVOY_LOG(debug, "adding hystrix_event_stream endpoint to enable connection to hystrix dashboard"); @@ -328,16 +331,12 @@ void HystrixSink::flush(Stats::Source& source) { // Save a map of the relevant histograms per cluster in a convenient format. std::unordered_map time_histograms; for (const Stats::ParentHistogramSharedPtr& histogram : source.cachedHistograms()) { - if (histogram->tagExtractedStatName() == cluster_upstream_rq_time_.statName()) { - // TODO(mrice32): add an Envoy utility function to look up and return a tag for a metric. - auto tags = histogram->tags(); - auto it = std::find_if(tags.begin(), tags.end(), [](const Stats::Tag& tag) { - return (tag.name_ == Config::TagNames::get().CLUSTER_NAME); - }); - + if (histogram->tagExtractedStatName() == cluster_upstream_rq_time_) { + absl::optional value = Stats::Utility::findTag(*histogram, cluster_name_); // Make sure we found the cluster name tag - ASSERT(it != histogram->tags().end()); - auto it_bool_pair = time_histograms.emplace(std::make_pair(it->value_, QuantileLatencyMap())); + ASSERT(value); + std::string value_str = server_.stats().symbolTable().toString(*value); + auto it_bool_pair = time_histograms.emplace(std::make_pair(value_str, QuantileLatencyMap())); // Make sure histogram with this name was not already added ASSERT(it_bool_pair.second); QuantileLatencyMap& hist_map = it_bool_pair.first->second; diff --git a/source/extensions/stat_sinks/hystrix/hystrix.h b/source/extensions/stat_sinks/hystrix/hystrix.h index c37a98db15c6e..f3801cb612e78 100644 --- a/source/extensions/stat_sinks/hystrix/hystrix.h +++ b/source/extensions/stat_sinks/hystrix/hystrix.h @@ -158,8 +158,10 @@ class HystrixSink : public Stats::Sink, public Logger::Loggable cluster_stats_cache_map_; - // Saved StatName for "cluster.upstream_rq_time" for fast comparisons in loop. - Stats::StatNameManagedStorage cluster_upstream_rq_time_; + // Saved StatNames for fast comparisons in loop. + Stats::StatNamePool stat_name_pool_; + Stats::StatName cluster_name_; + Stats::StatName cluster_upstream_rq_time_; }; typedef std::unique_ptr HystrixSinkPtr; diff --git a/test/common/stats/BUILD b/test/common/stats/BUILD index 1e1e7626c863f..f3ee1e0660e9f 100644 --- a/test/common/stats/BUILD +++ b/test/common/stats/BUILD @@ -34,6 +34,7 @@ envoy_cc_test( deps = [ "//source/common/stats:fake_symbol_table_lib", "//source/common/stats:heap_stat_data_lib", + "//source/common/stats:utility_lib", "//test/test_common:logging_lib", ], ) diff --git a/test/common/stats/metric_impl_test.cc b/test/common/stats/metric_impl_test.cc index d105047a7193d..0500a94c7b542 100644 --- a/test/common/stats/metric_impl_test.cc +++ b/test/common/stats/metric_impl_test.cc @@ -2,6 +2,7 @@ #include "common/stats/fake_symbol_table_impl.h" #include "common/stats/heap_stat_data.h" +#include "common/stats/utility.h" #include "test/test_common/logging.h" @@ -60,6 +61,14 @@ TEST_F(MetricImplTest, TwoTagsIterOnce) { EXPECT_EQ(1, count); } +TEST_F(MetricImplTest, FindTag) { + CounterSharedPtr counter = alloc_.makeCounter(makeStat("counter.name.value"), "counter", + {{"name1", "value1"}, {"name2", "value2"}}); + EXPECT_EQ(makeStat("value1"), Utility::findTag(*counter, makeStat("name1"))); + EXPECT_EQ(makeStat("value2"), Utility::findTag(*counter, makeStat("name2"))); + EXPECT_FALSE(Utility::findTag(*counter, makeStat("name3"))); +} + } // namespace } // namespace Stats } // namespace Envoy diff --git a/test/extensions/stats_sinks/common/statsd/udp_statsd_test.cc b/test/extensions/stats_sinks/common/statsd/udp_statsd_test.cc index b4263b010e58a..d48999fc258ab 100644 --- a/test/extensions/stats_sinks/common/statsd/udp_statsd_test.cc +++ b/test/extensions/stats_sinks/common/statsd/udp_statsd_test.cc @@ -95,21 +95,21 @@ TEST_P(UdpStatsdSinkWithTagsTest, InitWithIpAddress) { counter->name_ = "test_counter"; counter->used_ = true; counter->latch_ = 1; - counter->tags_ = tags; + counter->setTags(tags); source.counters_.push_back(counter); auto gauge = std::make_shared>(); gauge->name_ = "test_gauge"; gauge->value_ = 1; gauge->used_ = true; - gauge->tags_ = tags; + gauge->setTags(tags); source.gauges_.push_back(gauge); sink.flush(source); NiceMock timer; timer.name_ = "test_timer"; - timer.tags_ = tags; + timer.setTags(tags); sink.onHistogramComplete(timer, 5); EXPECT_EQ(fd, sink.getFdForTests()); @@ -189,7 +189,7 @@ TEST(UdpStatsdSinkWithTagsTest, CheckActualStats) { counter->name_ = "test_counter"; counter->used_ = true; counter->latch_ = 1; - counter->tags_ = tags; + counter->setTags(tags); source.counters_.push_back(counter); EXPECT_CALL(*std::dynamic_pointer_cast>(writer_ptr), @@ -201,7 +201,7 @@ TEST(UdpStatsdSinkWithTagsTest, CheckActualStats) { gauge->name_ = "test_gauge"; gauge->value_ = 1; gauge->used_ = true; - gauge->tags_ = tags; + gauge->setTags(tags); source.gauges_.push_back(gauge); EXPECT_CALL(*std::dynamic_pointer_cast>(writer_ptr), @@ -210,7 +210,7 @@ TEST(UdpStatsdSinkWithTagsTest, CheckActualStats) { NiceMock timer; timer.name_ = "test_timer"; - timer.tags_ = tags; + timer.setTags(tags); EXPECT_CALL(*std::dynamic_pointer_cast>(writer_ptr), write("envoy.test_timer:5|ms|#key1:value1,key2:value2")); sink.onHistogramComplete(timer, 5); diff --git a/test/extensions/stats_sinks/hystrix/hystrix_test.cc b/test/extensions/stats_sinks/hystrix/hystrix_test.cc index 2f52df159d198..0e4a140035d2c 100644 --- a/test/extensions/stats_sinks/hystrix/hystrix_test.cc +++ b/test/extensions/stats_sinks/hystrix/hystrix_test.cc @@ -463,7 +463,7 @@ TEST_F(HystrixSinkTest, HistogramTest) { auto histogram = std::make_shared>(); histogram->name_ = "cluster." + cluster1_name_ + ".upstream_rq_time"; histogram->setTagExtractedName("cluster.upstream_rq_time"); - histogram->tags_.emplace_back(Stats::Tag{Config::TagNames::get().CLUSTER_NAME, cluster1_name_}); + histogram->addTag(Stats::Tag{Config::TagNames::get().CLUSTER_NAME, cluster1_name_}); histogram->used_ = true; // Init with data such that the quantile value is equal to the quantile. diff --git a/test/mocks/stats/mocks.cc b/test/mocks/stats/mocks.cc index 51717099e01db..18c7e1a08bc0f 100644 --- a/test/mocks/stats/mocks.cc +++ b/test/mocks/stats/mocks.cc @@ -17,7 +17,7 @@ using testing::ReturnRef; namespace Envoy { namespace Stats { -MockMetric::MockMetric() : name_(*this) {} +MockMetric::MockMetric() : name_(*this), tag_pool_(*symbol_table_) {} MockMetric::~MockMetric() {} MockMetric::MetricName::~MetricName() { @@ -32,6 +32,20 @@ void MockMetric::setTagExtractedName(absl::string_view name) { std::make_unique(tagExtractedName(), *symbol_table_); } +void MockMetric::setTags(const std::vector& tags) { + tag_pool_.clear(); + tags_ = tags; + for (const Tag& tag : tags) { + tag_names_and_values_.push_back(tag_pool_.add(tag.name_)); + tag_names_and_values_.push_back(tag_pool_.add(tag.value_)); + } +} +void MockMetric::addTag(const Tag& tag) { + tags_.emplace_back(tag); + tag_names_and_values_.push_back(tag_pool_.add(tag.name_)); + tag_names_and_values_.push_back(tag_pool_.add(tag.value_)); +} + void MockMetric::iterateTags(const TagIterFn& fn) const { for (const Tag& tag : tags_) { if (!fn(tag)) { @@ -41,11 +55,9 @@ void MockMetric::iterateTags(const TagIterFn& fn) const { } void MockMetric::iterateTagStatNames(const TagStatNameIterFn& fn) const { - SymbolTable& symbol_table = const_cast(symbolTable()); - for (const Tag& tag : tags_) { - StatNameManagedStorage name(tag.name_, symbol_table); - StatNameManagedStorage value(tag.value_, symbol_table); - if (!fn(name.statName(), value.statName())) { + ASSERT((tag_names_and_values_.size() % 2) == 0); + for (size_t i = 0; i < tag_names_and_values_.size(); i += 2) { + if (!fn(tag_names_and_values_[i], tag_names_and_values_[i + 1])) { return; } } diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index 48c76e01424aa..cd7407e0c95e3 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -69,10 +69,15 @@ class MockMetric : public virtual Metric { Test::Global symbol_table_; // Must outlive name_. MetricName name_; - std::vector tags_; + + void setTags(const std::vector& tags); + void addTag(const Tag& tag); private: + std::vector tags_; + std::vector tag_names_and_values_; std::string tag_extracted_name_; + StatNamePool tag_pool_; std::unique_ptr tag_extracted_stat_name_; }; diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 98a2620c5331a..6268023b474e0 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -1464,7 +1464,7 @@ TEST_F(PrometheusStatsFormatterTest, OutputWithAllMetricTypes) { auto histogram1 = std::make_shared>(); histogram1->name_ = "cluster.test_1.upstream_rq_time"; histogram1->used_ = true; - histogram1->tags_ = {Stats::Tag{"key1", "value1"}, Stats::Tag{"key2", "value2"}}; + histogram1->setTags({Stats::Tag{"key1", "value1"}, Stats::Tag{"key2", "value2"}}); addHistogram(histogram1); EXPECT_CALL(*histogram1, cumulativeStatistics()) .WillOnce(testing::ReturnRef(h1_cumulative_statistics)); @@ -1524,7 +1524,7 @@ TEST_F(PrometheusStatsFormatterTest, OutputWithUsedOnly) { auto histogram1 = std::make_shared>(); histogram1->name_ = "cluster.test_1.upstream_rq_time"; histogram1->used_ = true; - histogram1->tags_ = {Stats::Tag{"key1", "value1"}, Stats::Tag{"key2", "value2"}}; + histogram1->setTags({Stats::Tag{"key1", "value1"}, Stats::Tag{"key2", "value2"}}); addHistogram(histogram1); EXPECT_CALL(*histogram1, cumulativeStatistics()) .WillOnce(testing::ReturnRef(h1_cumulative_statistics)); @@ -1571,7 +1571,7 @@ TEST_F(PrometheusStatsFormatterTest, OutputWithUsedOnlyHistogram) { auto histogram1 = std::make_shared>(); histogram1->name_ = "cluster.test_1.upstream_rq_time"; histogram1->used_ = false; - histogram1->tags_ = {Stats::Tag{"key1", "value1"}, Stats::Tag{"key2", "value2"}}; + histogram1->setTags({Stats::Tag{"key1", "value1"}, Stats::Tag{"key2", "value2"}}); addHistogram(histogram1); {