StatsAccessLogger: fixes connection gauge underflow crashes when decrementing metrics after Scope evictions.

source/common/stats/thread_local_store.cc

@@ -1130,15 +1130,22 @@ void ThreadLocalStoreImpl::evictUnused() {
         MetricBag metrics(scope->scope_id_);
         CentralCacheEntrySharedPtr& central_cache = scope->centralCacheMutableNoThreadAnalysis();
         auto filter_unused = []<typename T>(StatNameHashMap<T>& unused_metrics) {
-          return [&unused_metrics](std::pair<StatName, T> kv) {
+          return [&unused_metrics](const std::pair<const StatName, T>& kv) {
             const auto& [name, metric] = kv;
+            // Evictable scopes can contain counters, gauges, text-readouts, and histograms. For all
+            // the gauges we find in one, we treat them as up/down counters that become evictable
+            // when they hit zero.
+            if constexpr (std::is_same_v<T, GaugeSharedPtr>) {
+              if (metric->value() != 0) {
+                return false;
+              }
+            }
             if (metric->used()) {
               metric->markUnused();
               return false;
-            } else {
-              unused_metrics.try_emplace(name, metric);
-              return true;
             }
+            unused_metrics.try_emplace(name, metric);
+            return true;
           };
         };
         absl::erase_if(central_cache->counters_, filter_unused(metrics.counters_));

source/extensions/access_loggers/stats/BUILD

@@ -19,6 +19,7 @@ envoy_cc_library(
         "//source/common/access_log:access_log_lib",
         "//source/common/formatter:substitution_format_string_lib",
         "//source/common/matcher:matcher_lib",
+        "//source/common/stats:tag_utility_lib",
         "//source/extensions/access_loggers/common:access_log_base",
         "//source/extensions/matching/actions/transform_stat:transform_stat_lib",
         "@envoy_api//envoy/data/accesslog/v3:pkg_cc_proto",

source/extensions/access_loggers/stats/stats.cc

@@ -17,68 +17,6 @@ namespace {
 
 using Extensions::Matching::Actions::TransformStat::ActionContext;
 
-class AccessLogState : public StreamInfo::FilterState::Object {
-public:
-  AccessLogState(Stats::ScopeSharedPtr scope) : scope_(std::move(scope)) {}
-
-  // When the request is destroyed, we need to subtract the value from the gauge.
-  // We need to look up the gauge again in the scope because it might have been evicted.
-  // The gauge object itself is kept alive by the shared_ptr in the state, so we can access its
-  // name and tags to re-lookup/re-create it in the scope.
-  ~AccessLogState() override {
-    for (const auto& [gauge_ptr, state] : inflight_gauges_) {
-      // TODO(taoxuy):  make this as an accessor of the
-      // Stat class.
-      Stats::StatNameTagVector tag_names;
-      state.gauge_->iterateTagStatNames(
-          [&tag_names](Stats::StatName name, Stats::StatName value) -> bool {
-            tag_names.emplace_back(name, value);
-            return true;
-          });
-
-      // Using state.gauge_->statName() directly would be incorrect because it returns the fully
-      // qualified name (including tags). Passing this full name to scope_->gaugeFromStatName(...)
-      // would cause the scope to attempt tag extraction on the full name. Since the tags in
-      // AccessLogState are often dynamic and not configured in the global tag extractors, this
-      // extraction would likely fail to identify the tags correctly, resulting in a gauge with a
-      // different identity (the full name as the stat name and no tags).
-      auto& gauge = scope_->gaugeFromStatNameWithTags(
-          state.gauge_->tagExtractedStatName(), tag_names, Stats::Gauge::ImportMode::Accumulate);
-      gauge.sub(state.value_);
-    }
-  }
-
-  void addInflightGauge(Stats::Gauge* gauge, uint64_t value) {
-    inflight_gauges_.try_emplace(gauge, Stats::GaugeSharedPtr(gauge), value);
-  }
-
-  absl::optional<uint64_t> removeInflightGauge(Stats::Gauge* gauge) {
-    auto it = inflight_gauges_.find(gauge);
-    if (it == inflight_gauges_.end()) {
-      return absl::nullopt;
-    }
-    uint64_t value = it->second.value_;
-    inflight_gauges_.erase(it);
-    return value;
-  }
-
-  static constexpr absl::string_view key() { return "envoy.access_loggers.stats.access_log_state"; }
-
-private:
-  struct State {
-    State(Stats::GaugeSharedPtr gauge, uint64_t value) : gauge_(std::move(gauge)), value_(value) {}
-
-    Stats::GaugeSharedPtr gauge_;
-    uint64_t value_;
-  };
-
-  Stats::ScopeSharedPtr scope_;
-
-  // The map key holds a raw pointer to the gauge. The value holds a ref-counted pointer to ensure
-  // the gauge is not destroyed if it is evicted from the stats scope.
-  absl::flat_hash_map<Stats::Gauge*, State> inflight_gauges_;
-};
-
 Formatter::FormatterProviderPtr
 parseValueFormat(absl::string_view format,
                  const std::vector<Formatter::CommandParserPtr>& commands) {
@@ -138,6 +76,95 @@ class TagActionValidationVisitor
 
 } // namespace
 
+AccessLogState::~AccessLogState() {
+  for (const auto& p : inflight_gauges_) {
+    Stats::Gauge& gauge_stat = parent_->scope().gaugeFromStatNameWithTags(
+        p.first.statName(), p.first.tags(), p.second.import_mode_);
+    gauge_stat.sub(p.second.value_);
+  }
+}
+
+void AccessLogState::addInflightGauge(Stats::StatName stat_name,
+                                      Stats::StatNameTagVectorOptConstRef tags,
+                                      Stats::Gauge::ImportMode import_mode, uint64_t value,
+                                      std::vector<Stats::StatNameDynamicStorage> tags_storage) {
+  if (value == 0) {
+    return;
+  }
+
+  GaugeKey key{stat_name, tags};
+
+  auto it = inflight_gauges_.find(key);
+  if (it == inflight_gauges_.end()) {
+    key.makeOwned();
+    auto [new_it, inserted] = inflight_gauges_.emplace(
+        std::move(key), InflightGauge{std::move(tags_storage), 0, import_mode});
+    it = new_it;
+  }
+  it->second.value_ += value;
+  parent_->scope().gaugeFromStatNameWithTags(stat_name, tags, import_mode).add(value);
+}
+
+void AccessLogState::removeInflightGauge(Stats::StatName stat_name,
+                                         Stats::StatNameTagVectorOptConstRef tags,
+                                         Stats::Gauge::ImportMode import_mode, uint64_t value) {
+  if (value == 0) {
+    return;
+  }
+
+  GaugeKey key{stat_name, tags};
+
+  Stats::Gauge& gauge_stat =
+      parent_->scope().gaugeFromStatNameWithTags(stat_name, tags, import_mode);
+
+  auto it = inflight_gauges_.find(key);
+  const bool was_found = (it != inflight_gauges_.end());
+  if (was_found) {
+    ENVOY_BUG(it->second.value_ >= value, "Connection gauge underflow in removeInflightGauge");
+    it->second.value_ -= value;
+    gauge_stat.sub(value);
+    if (it->second.value_ == 0) {
+      inflight_gauges_.erase(it);
+    }
+  } else {
+    ENVOY_LOG_PERIODIC_MISC(error, std::chrono::seconds(10),
+                            "Stats access logger gauge paired subtract was skipped due to no "
+                            "corresponding add, possibly due to misconfigured events: {}",
+                            parent_->scope().symbolTable().toString(stat_name));
+  }
+}
+
+GaugeKey::GaugeKey(Stats::StatName stat_name, Stats::StatNameTagVectorOptConstRef borrowed_tags)
+    : stat_name_(stat_name), borrowed_tags_(borrowed_tags) {}
+
+void GaugeKey::makeOwned() {
+  ASSERT(!(borrowed_tags_.has_value() && owned_tags_.has_value()),
+         "Both borrowed and owned tags are present in GaugeKey::makeOwned");
+  if (borrowed_tags_.has_value() && !owned_tags_.has_value()) {
+    owned_tags_ = borrowed_tags_.value().get();
+    borrowed_tags_ = absl::nullopt;
+  }
+}
+
+Stats::StatNameTagVectorOptConstRef GaugeKey::tags() const {
+  if (owned_tags_.has_value()) {
+    return std::cref(owned_tags_.value());
+  }
+  return borrowed_tags_;
+}
+
+bool GaugeKey::operator==(const GaugeKey& rhs) const {
+  if (stat_name_ != rhs.stat_name_) {
+    return false;
+  }
+  Stats::StatNameTagVectorOptConstRef lhs_tags = tags();
+  Stats::StatNameTagVectorOptConstRef rhs_tags = rhs.tags();
+  if (lhs_tags.has_value() != rhs_tags.has_value()) {
+    return false;
+  }
+  return !lhs_tags.has_value() || lhs_tags.value().get() == rhs_tags.value().get();
+}
+
 StatsAccessLog::StatsAccessLog(const envoy::extensions::access_loggers::stats::v3::Config& config,
                                Server::Configuration::GenericFactoryContext& context,
                                AccessLog::FilterPtr&& filter,
@@ -417,31 +444,28 @@ void StatsAccessLog::emitLogForGauge(const Gauge& gauge, const Formatter::Contex
   Stats::Gauge::ImportMode import_mode = op == Gauge::OperationType::SET
                                              ? Stats::Gauge::ImportMode::NeverImport
                                              : Stats::Gauge::ImportMode::Accumulate;
-  auto& gauge_stat = scope_->gaugeFromStatNameWithTags(gauge.stat_.name_, tags, import_mode);
 
-  if (op == Gauge::OperationType::PAIRED_ADD || op == Gauge::OperationType::PAIRED_SUBTRACT) {
+  if (op == Gauge::OperationType::SET) {
+    Stats::Gauge& gauge_stat =
+        scope_->gaugeFromStatNameWithTags(gauge.stat_.name_, tags, import_mode);
+    gauge_stat.set(value);
+  } else if (op == Gauge::OperationType::PAIRED_ADD ||
+             op == Gauge::OperationType::PAIRED_SUBTRACT) {
     auto& filter_state = const_cast<StreamInfo::FilterState&>(stream_info.filterState());
     if (!filter_state.hasData<AccessLogState>(AccessLogState::key())) {
-      filter_state.setData(AccessLogState::key(), std::make_shared<AccessLogState>(scope_),
-                           StreamInfo::FilterState::StateType::Mutable,
-                           StreamInfo::FilterState::LifeSpan::Request);
+      // TODO(TAOXUY): Create a new PR that adds test coverage around any corner cases of which
+      // level should be used, and adds this comment or an updated version.
+      filter_state.setData(
+          AccessLogState::key(), std::make_shared<AccessLogState>(shared_from_this()),
+          StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::Request);
     }
     auto* state = filter_state.getDataMutable<AccessLogState>(AccessLogState::key());
 
     if (op == Gauge::OperationType::PAIRED_ADD) {
-      state->addInflightGauge(&gauge_stat, value);
-      gauge_stat.add(value);
+      state->addInflightGauge(gauge.stat_.name_, tags, import_mode, value, std::move(storage));
     } else {
-      absl::optional<uint64_t> added_value = state->removeInflightGauge(&gauge_stat);
-      if (added_value.has_value()) {
-        gauge_stat.sub(added_value.value());
-      }
+      state->removeInflightGauge(gauge.stat_.name_, tags, import_mode, value);
     }
-    return;
-  }
-
-  if (op == Gauge::OperationType::SET) {
-    gauge_stat.set(value);
   }
 }
 

source/extensions/access_loggers/stats/stats.h

@@ -4,18 +4,83 @@
 #include "envoy/extensions/access_loggers/stats/v3/stats.pb.h"
 #include "envoy/stats/stats.h"
 #include "envoy/stats/tag.h"
+#include "envoy/stream_info/filter_state.h"
 
 #include "source/common/matcher/matcher.h"
+#include "source/common/stats/symbol_table.h"
 #include "source/extensions/access_loggers/common/access_log_base.h"
 #include "source/extensions/matching/actions/transform_stat/transform_stat.h"
 
+#include "absl/container/node_hash_map.h"
+
 namespace Envoy {
 namespace Extensions {
 namespace AccessLoggers {
 namespace StatsAccessLog {
 
-class StatsAccessLog : public AccessLoggers::Common::ImplBase {
+// GaugeKey serves as a lock-free map key composed of exactly the configuration
+// properties that define a fully resolved gauge metric.
+//
+// It preserves the raw components (base name + tags) allowing us to safely
+// re-create the gauge from the scope if it gets evicted while the request is in-flight.
+//
+// To avoid heap-allocating a new std::vector on every map lookup (which happens
+// on every single gauge increment/decrement), this key acts as a lightweight
+// zero-allocation "view" using `borrowed_tags_` during map lookups.
+// When the key actually needs to be safely persisted into the map, `makeOwned()`
+// is explicitly called to allocate and copy the tags into `owned_tags_`.
+class GaugeKey {
+public:
+  GaugeKey(Stats::StatName stat_name, Stats::StatNameTagVectorOptConstRef borrowed_tags);
+
+  GaugeKey(const GaugeKey&) = delete;
+  GaugeKey& operator=(const GaugeKey&) = delete;
+
+  GaugeKey(GaugeKey&&) = default;
+  GaugeKey& operator=(GaugeKey&&) = default;
+
+  void makeOwned();
+
+  Stats::StatNameTagVectorOptConstRef tags() const;
+
+  Stats::StatName statName() const { return stat_name_; }
+
+  bool operator==(const GaugeKey& rhs) const;
+
+  template <typename H> friend H AbslHashValue(H h, const GaugeKey& key) {
+    // We hash the logical tag content to match operator== behavior, ignoring
+    // whether the tags are stored in owned_tags_ or borrowed_tags_. This ensures
+    // that two equal keys produce the same hash regardless of their storage representation.
+    Stats::StatNameTagVectorOptConstRef tags = key.tags();
+    if (tags.has_value()) {
+      h = H::combine(std::move(h), key.stat_name_, true);
+      for (const auto& tag : tags.value().get()) {
+        h = H::combine(std::move(h), tag.first, tag.second);
+      }
+      return h;
+    }
+    return H::combine(std::move(h), key.stat_name_, false);
+  }
+
+private:
+  // The backing store for `stat_name_` is the StatNamePool owned by the StatsAccessLog::Config,
+  // which has the same lifetime as the logger itself.
+  Stats::StatName stat_name_;
+
+  // The `StatName`s in `owned_tags_` (when present) represent dynamically generated tags.
+  // Their memory is backed by the Envoy stats store's SymbolTable. To ensure these dynamic tags
+  // are not freed prematurely and do not leak, their reference counts are kept alive by
+  // `StatNameDynamicStorage` instances stored alongside the gauge value in `InflightGauge`
+  // within the `AccessLogState`.
+  absl::optional<Stats::StatNameTagVector> owned_tags_;
+
+  Stats::StatNameTagVectorOptConstRef borrowed_tags_{absl::nullopt};
+};
+
+class StatsAccessLog : public AccessLoggers::Common::ImplBase,
+                       public std::enable_shared_from_this<StatsAccessLog> {
 public:
+  Stats::Scope& scope() const { return *scope_; }
   StatsAccessLog(const envoy::extensions::access_loggers::stats::v3::Config& config,
                  Server::Configuration::GenericFactoryContext& context,
                  AccessLog::FilterPtr&& filter,
@@ -102,7 +167,42 @@ class StatsAccessLog : public AccessLoggers::Common::ImplBase {
   const std::vector<Gauge> gauges_;
 };
 
+class AccessLogState : public StreamInfo::FilterState::Object {
+public:
+  AccessLogState(std::shared_ptr<const StatsAccessLog> parent) : parent_(std::move(parent)) {}
+
+  ~AccessLogState() override;
+
+  // Adds an incremental value to an existing gauge, or creates it if that gauge doesn't exist.
+  // Zero values are ignored. If the same value isn't removed with `removeInflightGauge`, the
+  // value is removed when the object is destroyed.
+  void addInflightGauge(Stats::StatName stat_name, Stats::StatNameTagVectorOptConstRef tags,
+                        Stats::Gauge::ImportMode import_mode, uint64_t value,
+                        std::vector<Stats::StatNameDynamicStorage> tags_storage);
+
+  // Removes an amount from an existing gauge, allowing the gauge to be evicted if the value reaches
+  // 0.
+  void removeInflightGauge(Stats::StatName stat_name, Stats::StatNameTagVectorOptConstRef tags,
+                           Stats::Gauge::ImportMode import_mode, uint64_t value);
+
+  static constexpr absl::string_view key() { return "envoy.access_loggers.stats.access_log_state"; }
+
+private:
+  // Hold a shared_ptr to the parent to ensure the parent and its members exist for the lifetime of
+  // AccessLogState.
+  std::shared_ptr<const StatsAccessLog> parent_;
+
+  struct InflightGauge {
+    std::vector<Stats::StatNameDynamicStorage> tags_storage_;
+    uint64_t value_;
+    Stats::Gauge::ImportMode import_mode_;
+  };
+
+  absl::node_hash_map<GaugeKey, InflightGauge> inflight_gauges_;
+};
+
 } // namespace StatsAccessLog
+
 } // namespace AccessLoggers
 } // namespace Extensions
 } // namespace Envoy