stats: use tag iterator rather than constructing the tag-array and searching that.

source/common/stats/utility.cc

@@ -21,5 +21,18 @@ std::string Utility::sanitizeStatsName(absl::string_view name) {
   return stats_name;
 }
 
+absl::optional<StatName> Utility::findTag(Metric& metric, StatName find_tag_name) {
+  absl::optional<StatName> 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

source/common/stats/utility.h

@@ -2,7 +2,12 @@
 
 #include <string>
 
+#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<StatName> findTag(Metric& metric, StatName find_tag_name);
 };
 
 } // namespace Stats

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",
     ],
 )

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<std::string, QuantileLatencyMap> 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<Stats::StatName> 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;

source/extensions/stat_sinks/hystrix/hystrix.h

@@ -158,8 +158,10 @@ class HystrixSink : public Stats::Sink, public Logger::Loggable<Logger::Id::hyst
   // Map from cluster names to a struct of all of that cluster's stat windows.
   std::unordered_map<std::string, ClusterStatsCachePtr> 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<HystrixSink> HystrixSinkPtr;

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",
     ],
 )

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

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<NiceMock<Stats::MockGauge>>();
   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<Stats::MockHistogram> 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<NiceMock<MockWriter>>(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<NiceMock<MockWriter>>(writer_ptr),
@@ -210,7 +210,7 @@ TEST(UdpStatsdSinkWithTagsTest, CheckActualStats) {
 
   NiceMock<Stats::MockHistogram> timer;
   timer.name_ = "test_timer";
-  timer.tags_ = tags;
+  timer.setTags(tags);
   EXPECT_CALL(*std::dynamic_pointer_cast<NiceMock<MockWriter>>(writer_ptr),
               write("envoy.test_timer:5|ms|#key1:value1,key2:value2"));
   sink.onHistogramComplete(timer, 5);

test/extensions/stats_sinks/hystrix/hystrix_test.cc

@@ -463,7 +463,7 @@ TEST_F(HystrixSinkTest, HistogramTest) {
   auto histogram = std::make_shared<NiceMock<Stats::MockParentHistogram>>();
   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.

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<StatNameManagedStorage>(tagExtractedName(), *symbol_table_);
 }
 
+void MockMetric::setTags(const std::vector<Tag>& 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&>(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;
     }
   }

test/mocks/stats/mocks.h

@@ -69,10 +69,15 @@ class MockMetric : public virtual Metric {
 
   Test::Global<FakeSymbolTableImpl> symbol_table_; // Must outlive name_.
   MetricName name_;
-  std::vector<Tag> tags_;
+
+  void setTags(const std::vector<Tag>& tags);
+  void addTag(const Tag& tag);
 
 private:
+  std::vector<Tag> tags_;
+  std::vector<StatName> tag_names_and_values_;
   std::string tag_extracted_name_;
+  StatNamePool tag_pool_;
   std::unique_ptr<StatNameManagedStorage> tag_extracted_stat_name_;
 };
 

test/server/http/admin_test.cc

@@ -1464,7 +1464,7 @@ TEST_F(PrometheusStatsFormatterTest, OutputWithAllMetricTypes) {
   auto histogram1 = std::make_shared<NiceMock<Stats::MockParentHistogram>>();
   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<NiceMock<Stats::MockParentHistogram>>();
   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<NiceMock<Stats::MockParentHistogram>>();
   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);
 
   {