From a9743a109afef71aa02cdbd6d9b02c971d7959c8 Mon Sep 17 00:00:00 2001 From: wbpcode/wangbaiping Date: Fri, 26 Jun 2026 08:59:17 +0000 Subject: [PATCH 1/6] stats: make it's possible to enable the new stats API Signed-off-by: wbpcode/wangbaiping --- .../stats__explicit-tags.rst | 8 + envoy/stats/scope.h | 2 +- envoy/stats/store.h | 8 + source/common/runtime/runtime_features.cc | 4 + source/common/stats/symbol_table.cc | 34 ++ source/common/stats/symbol_table.h | 34 ++ source/common/stats/tag_utility.h | 6 +- source/common/stats/thread_local_store.cc | 330 +++++++++--------- source/common/stats/thread_local_store.h | 84 ++--- source/server/server.cc | 16 + test/common/stats/tag_utility_test.cc | 2 +- test/common/stats/thread_local_store_test.cc | 86 +++-- test/integration/server.h | 1 + test/server/BUILD | 4 + test/server/server_test.cc | 62 ++++ 15 files changed, 436 insertions(+), 245 deletions(-) create mode 100644 changelogs/current/minor_behavior_changes/stats__explicit-tags.rst diff --git a/changelogs/current/minor_behavior_changes/stats__explicit-tags.rst b/changelogs/current/minor_behavior_changes/stats__explicit-tags.rst new file mode 100644 index 0000000000000..9a451213048fe --- /dev/null +++ b/changelogs/current/minor_behavior_changes/stats__explicit-tags.rst @@ -0,0 +1,8 @@ +Added the runtime guard ``envoy.reloadable_features.enable_stats_explicit_tags`` (default +``false``). When set to ``true`` and the stats configuration carries no custom tags (empty +:ref:`stats_tags ` and +:ref:`use_all_default_tags ` +left at its default of ``true``), the stats store uses the tags supplied by the calling code (the +explicit-tags logic) and propagates scope-level tags onto every stat, instead of re-parsing the +flat stat name. The guard is evaluated once at startup. There is no visible change to users while +the guard remains ``false``. diff --git a/envoy/stats/scope.h b/envoy/stats/scope.h index 7de2f3dc73d62..b556ef2088105 100644 --- a/envoy/stats/scope.h +++ b/envoy/stats/scope.h @@ -108,7 +108,7 @@ class Scope : public std::enable_shared_from_this { /** * Allocate a new scope, optionally supplying tags and a pre-built tagged name (with tag values * interleaved). NOTE: The behavior is implementation-defined for now because both legacy and - * tag-aware scopes are still supported. + * explicit-tags scopes are still supported. * * @param base_name supplies the scope's tag-extracted name. * @param name_tags tags to associate with (and propagate from) the scope, as string_view pairs. diff --git a/envoy/stats/store.h b/envoy/stats/store.h index 6dafcdaed3211..681cf58e9c511 100644 --- a/envoy/stats/store.h +++ b/envoy/stats/store.h @@ -251,6 +251,14 @@ class StoreRoot : public Store { */ virtual void setHistogramSettings(HistogramSettingsConstPtr&& histogram_settings) PURE; + /** + * Selects whether scopes created by this store use the explicit-tags logic, which propagates + * scope-level tags onto every stat. Must be called during single-threaded startup, before + * scopes other than the root scope are created and before initializeThreading(); flipping it + * afterwards is unsupported. + */ + virtual void setUseExplicitTags(bool use_explicit_tags) PURE; + /** * Initialize the store for threading. This will be called once after all worker threads have * been initialized. At this point the store can initialize itself for multi-threaded operation. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index eb87a178b5b71..1473c04acc07f 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -146,6 +146,10 @@ RUNTIME_GUARD(envoy_restart_features_worker_threads_watchdog_fix); // Sentinel and test flag. FALSE_RUNTIME_GUARD(envoy_reloadable_features_test_feature_false); +// When true (and the stats config carries no custom tags), the stats store uses the explicit-tags +// scope logic that propagates scope-level tags onto every stat. Evaluated once at startup. +// TODO: flip to true after sufficient testing. +FALSE_RUNTIME_GUARD(envoy_reloadable_features_enable_stats_explicit_tags); // TODO: Flip to true after sufficient testing to enable formatter support for rate limit action // descriptor_value fields by default. FALSE_RUNTIME_GUARD( diff --git a/source/common/stats/symbol_table.cc b/source/common/stats/symbol_table.cc index 4042c7ba9ca09..7598c7ce08217 100644 --- a/source/common/stats/symbol_table.cc +++ b/source/common/stats/symbol_table.cc @@ -718,6 +718,40 @@ void SymbolTable::populateList(const StatName* names, uint32_t num_names, StatNa list.moveStorageIntoList(mem_block.release()); } +void SymbolTable::populateList(StatName tagged_name, StatName base_name, StatNameTagSpan name_tags, + StatNameList& list) { + const size_t stat_name_count = name_tags.size() * 2 + 2; + + RELEASE_ASSERT(stat_name_count < 256, "Maximum number elements in a StatNameList exceeded"); + + // One byte for the number of names, plus the size of each name. + size_t total_size_bytes = 1; + total_size_bytes += tagged_name.size() + base_name.size(); + for (const auto& tag : name_tags) { + total_size_bytes += tag.first.size() + tag.second.size(); + } + + // Now allocate the exact number of bytes required and move the encodings into storage. + MemBlockBuilder mem_block(total_size_bytes); + mem_block.appendOne(stat_name_count); + Encoding::appendToMemBlock(tagged_name, mem_block); + Encoding::appendToMemBlock(base_name, mem_block); + incRefCount(tagged_name); + incRefCount(base_name); + for (const auto& tag : name_tags) { + Encoding::appendToMemBlock(tag.first, mem_block); + Encoding::appendToMemBlock(tag.second, mem_block); + incRefCount(tag.first); + incRefCount(tag.second); + } + + // This assertion double-checks the arithmetic where we computed total_size_bytes. After appending + // all the encoded data into the allocated byte array, we should have exhausted all the memory we + // thought we needed. + ASSERT(mem_block.capacityRemaining() == 0); + list.moveStorageIntoList(mem_block.release()); +} + StatNameList::~StatNameList() { ASSERT(!populated()); } void StatNameList::iterate(const std::function& f) const { diff --git a/source/common/stats/symbol_table.h b/source/common/stats/symbol_table.h index 2c3a3ea748d19..fa73b5b6696de 100644 --- a/source/common/stats/symbol_table.h +++ b/source/common/stats/symbol_table.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include #include #include @@ -28,6 +29,8 @@ using StatNameVec = absl::InlinedVector; class StatNameList; class StatNameSet; using StatNameSetPtr = std::unique_ptr; +using StatNameTag = std::pair; +using StatNameTagSpan = absl::Span; /** * Holds a range of indexes indicating which parts of a stat-name are @@ -314,6 +317,19 @@ class SymbolTable final { */ void populateList(const StatName* names, uint32_t num_names, StatNameList& list); + /** + * Populates a StatNameList from a name, base-name, and tags. This is not done at + * construction time to enable StatNameList to be instantiated directly in + * a class that doesn't have a live SymbolTable when it is constructed. + * + * @param tagged_name The tagged name of the stat. + * @param base_name The base name of the stat. + * @param name_tags The tags associated with the stat. + * @param list The StatNameList representing the stat names. + */ + void populateList(StatName tagged_name, StatName base_name, StatNameTagSpan name_tags, + StatNameList& list); + #ifndef ENVOY_CONFIG_COVERAGE void debugPrint() const; #endif @@ -906,6 +922,24 @@ class StatNameList { * @param f The function to call on each stat. */ void iterate(const std::function& f) const; + /** + * Iterates over each StatName in the list, calling f(StatName, index). f() + * should return true to keep iterating, or false to end the iteration. + * + * @param f The function to call on each stat. + */ + template void iterateWithIndex(const ConsumeStatNameCb& f) const { + ASSERT(populated()); + const uint8_t* p = storage_.get(); + const uint32_t num_elements = *p++; + for (uint32_t i = 0; i < num_elements; ++i) { + const StatName stat_name(p); + p += stat_name.size(); + if (!f(stat_name, i)) { + break; + } + } + } /** * Frees each StatName in the list. Failure to call this before destruction diff --git a/source/common/stats/tag_utility.h b/source/common/stats/tag_utility.h index 1d85c798c58ae..a41de7dad8717 100644 --- a/source/common/stats/tag_utility.h +++ b/source/common/stats/tag_utility.h @@ -27,7 +27,7 @@ class TagStatNameJoiner { /** * Combines a scope's tag-extracted prefix, prefix tags, and tagged (flat) prefix, together with * a stat's tag-extracted name, name tags and optional tagged (flat) name, into a single joined - * name. This is used by the tag-aware scope implementations to avoid needing to re-parse the + * name. This is used by the explicit-tags scope logic to avoid needing to re-parse the * final flat stat name in order to extract tags from it. * * @param prefix StatName the tag-extracted prefix for this scope (no tag values). @@ -61,7 +61,7 @@ class TagStatNameJoiner { * * Tags come from one of two mutually-exclusive sources depending on the constructor used: * the legacy constructor stores the caller-owned span directly (`stat_name_tags_`); the - * tag-aware constructor derives and owns them in `effective_tags_`. + * explicit-tags constructor derives and owns them in `effective_tags_`. */ absl::optional effectiveTags() const { if (stat_name_tags_.has_value()) { @@ -86,7 +86,7 @@ class TagStatNameJoiner { // lifetime. effectiveTags() returns this directly when set. absl::optional stat_name_tags_; - // Set only by the tag-aware constructor: the inherited (prefix) tags followed by this + // Set only by the explicit-tags constructor: the inherited (prefix) tags followed by this // element's own (name) tags, owned (copied) by this joiner. effectiveTags() returns a span over // this when it is non-empty. StatNameTagVec effective_tags_; diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index f1db94e223822..b6591e7145dfe 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -28,14 +28,13 @@ const char ThreadLocalStoreImpl::DeleteScopeSync[] = "delete-scope"; const char ThreadLocalStoreImpl::IterateScopeSync[] = "iterate-scope"; const char ThreadLocalStoreImpl::MainDispatcherCleanupSync[] = "main-dispatcher-cleanup"; -ThreadLocalStoreImpl::ThreadLocalStoreImpl(Allocator& alloc, bool use_tag_scope) +ThreadLocalStoreImpl::ThreadLocalStoreImpl(Allocator& alloc) : alloc_(alloc), tag_producer_(std::make_unique()), stats_matcher_(std::make_unique()), histogram_settings_(std::make_unique()), null_counter_(alloc.symbolTable()), null_gauge_(alloc.symbolTable()), null_histogram_(alloc.symbolTable()), null_text_readout_(alloc.symbolTable()), - well_known_tags_(alloc.symbolTable().makeSet("well_known_tags")), - use_tag_scope_(use_tag_scope) { + well_known_tags_(alloc.symbolTable().makeSet("well_known_tags")) { for (const auto& desc : Config::TagNames::get().descriptorVec()) { well_known_tags_->rememberBuiltin(desc.name_); } @@ -44,15 +43,12 @@ ThreadLocalStoreImpl::ThreadLocalStoreImpl(Allocator& alloc, bool use_tag_scope) // crash when StatNameStorage(StatName, SymbolTable&) calls src.size(). StatNameManagedStorage empty_storage("", alloc.symbolTable()); const StatName empty = empty_storage.statName(); - std::shared_ptr new_scope; - if (use_tag_scope_) { - auto pool = std::make_unique(alloc.symbolTable()); - const StatName interned_empty = pool->add(empty); - new_scope = std::make_shared(std::move(pool), interned_empty, StatNameTagSpan{}, - interned_empty, *this, false); - } else { - new_scope = std::make_shared(*this, empty, false); - } + // The root scope is always created explicit-tag-capable (via the explicit-tags constructor, which + // populates explicit_tag_data_) even though use_explicit_tags_ defaults to false. This lets + // setUseExplicitTags() enable the explicit-tags logic later (during startup) without recreating + // the root scope. For the empty-prefix, untagged root scope the explicit-tags and legacy code + // paths produce identical stat names, so this is behavior-neutral until the flag is flipped. + auto new_scope = std::make_shared(empty, StatNameTagSpan{}, empty, *this, false); addScope(new_scope); default_scope_ = new_scope; } @@ -173,11 +169,31 @@ std::vector ThreadLocalStoreImpl::counters() const { } ScopeSharedPtr ThreadLocalStoreImpl::ScopeImpl::createScopeWithTaggedName( - absl::string_view base_name, TagStringViewSpan, absl::string_view tagged_name, bool evictable, - const ScopeStatsLimitSettings& limits, StatsMatcherSharedPtr matcher) { - // The legacy scope implementation does not track scope-level tags. If a non-empty `tagged_name` - // is provided, it becomes the scope's flat name (the tag-extracted `base_name` and tags are - // dropped); otherwise `base_name` itself is used as the scope name. + absl::string_view base_name, TagStringViewSpan name_tags, absl::string_view tagged_name, + bool evictable, const ScopeStatsLimitSettings& limits, StatsMatcherSharedPtr matcher) { + if (useExplicitTags()) { + StatNamePool tag_pool(symbolTable()); + StatName stat_name = tag_pool.add(Utility::sanitizeStatsName(base_name)); + StatName stat_tagged_name; + if (!name_tags.empty()) { + // The tagged name is only meaningful when there are tags to interleave; otherwise it is + // ignored. + stat_tagged_name = + tagged_name.empty() ? StatName() : tag_pool.add(Utility::sanitizeStatsName(tagged_name)); + } + + StatNameTagVec stat_name_tags; + stat_name_tags.reserve(name_tags.size()); + for (const auto& [tag, value] : name_tags) { + stat_name_tags.emplace_back(tag_pool.add(tag), tag_pool.add(value)); + } + return scopeFromTaggedName(stat_name, stat_name_tags, stat_tagged_name, evictable, limits, + std::move(matcher)); + } + + // Legacy mode will ignore explicit tags of scope. If a non-empty `tagged_name` is provided, it + // becomes the scope's flat name (the tag-extracted `base_name` and tags are dropped); otherwise + // `base_name` itself is used as the scope name. if (!tagged_name.empty()) { base_name = tagged_name; } @@ -187,19 +203,34 @@ ScopeSharedPtr ThreadLocalStoreImpl::ScopeImpl::createScopeWithTaggedName( } ScopeSharedPtr ThreadLocalStoreImpl::ScopeImpl::scopeFromTaggedName( - StatName base_name, StatNameTagSpan, StatName tagged_name, bool evictable, + StatName base_name, StatNameTagSpan name_tags, StatName tagged_name, bool evictable, const ScopeStatsLimitSettings& limits, StatsMatcherSharedPtr matcher) { - // Same backward-compat behavior as createScopeWithTaggedName: if no explicit `tagged_name` is - // provided, fall back to using `base_name` as the scope name. The legacy scope does not retain - // scope-level tags. + // Use explicit matcher if provided; otherwise inherit scope_matcher_ (which may be null, + // meaning the store-level matcher is used). + StatsMatcherSharedPtr child_matcher = matcher ? std::move(matcher) : scope_matcher_; + + if (useExplicitTags()) { + // Combine this scope's prefix and tags with the new scope element to derive the child's + // tag-extracted prefix, tagged prefix and accumulated tags. The joiner's StatNames stay valid + // for the duration of the call, which is all the explicit-tags ScopeImpl constructor needs (it + // re-interns them into the child's own pool). + const TagUtility::TagStatNameJoiner joiner(base_prefix_, prefix_tags_, prefix_, base_name, + name_tags, tagged_name, symbolTable()); + std::shared_ptr new_scope = std::make_shared( + joiner.tagExtractedName(), joiner.effectiveTags().value_or(StatNameTagSpan{}), + joiner.nameWithTags(), parent_, evictable, limits, std::move(child_matcher)); + parent_.addScope(new_scope); + return new_scope; + } + + // Legacy mode will ignore explicit tags of scope. If a non-empty `tagged_name` is provided, it + // becomes the scope's flat name (the tag-extracted `base_name` and tags are dropped); otherwise + // `base_name` itself is used as the scope name. if (!tagged_name.empty()) { base_name = tagged_name; } - SymbolTable::StoragePtr joined = symbolTable().join({prefix_.statName(), base_name}); - // Use explicit matcher if provided; otherwise inherit scope_matcher_ (which may be null, - // meaning the store-level matcher is used). - StatsMatcherSharedPtr child_matcher = matcher ? std::move(matcher) : scope_matcher_; + SymbolTable::StoragePtr joined = symbolTable().join({prefix_, base_name}); auto new_scope = std::make_shared(parent_, StatName(joined.get()), evictable, limits, std::move(child_matcher)); parent_.addScope(new_scope); @@ -438,9 +469,48 @@ ThreadLocalStoreImpl::ScopeImpl::ScopeImpl(ThreadLocalStoreImpl& parent, StatNam bool evictable, const ScopeStatsLimitSettings& limits, StatsMatcherSharedPtr scope_matcher) : scope_id_(parent.next_scope_id_++), parent_(parent), evictable_(evictable), limits_(limits), - scope_matcher_(std::move(scope_matcher)), prefix_(prefix, parent.alloc_.symbolTable()), + scope_matcher_(std::move(scope_matcher)), central_cache_(new CentralCacheEntry(parent.alloc_.symbolTable())) { parent_.ensureOverflowStats(limits_); + + // Only single prefix for legacy mode. Both prefix_ and base_prefix_ are set to the same value, + // which is the flat (tagged) prefix. + parent_.alloc_.symbolTable().populateList(prefix, {}, {}, prefix_list_); + prefix_ = prefix_list_.at(0); + base_prefix_ = prefix_; +} + +ThreadLocalStoreImpl::ScopeImpl::ScopeImpl(StatName base_name, StatNameTagSpan name_tags, + StatName tagged_name, ThreadLocalStoreImpl& parent, + bool evictable, const ScopeStatsLimitSettings& limits, + StatsMatcherSharedPtr scope_matcher) + : scope_id_(parent.next_scope_id_++), parent_(parent), evictable_(evictable), limits_(limits), + scope_matcher_(std::move(scope_matcher)), + central_cache_(new CentralCacheEntry(parent.alloc_.symbolTable())) { + parent_.ensureOverflowStats(limits_); + + // Explicit-tags mode: prefix_ is the tag-extracted prefix, and base_prefix_ is the flat (tagged) + // prefix. The tag-extracted prefix is used for stat name interning and the flat prefix is used + // for stat name formatting. The tag-extracted prefix is always valid, but the flat prefix may be + // empty if the scope is created with an empty prefix and no tags. + parent_.alloc_.symbolTable().populateList(tagged_name, base_name, name_tags, prefix_list_); + ASSERT(prefix_list_.size() == 2 + name_tags.size() * 2); + prefix_tags_.resize(name_tags.size()); + prefix_list_.iterateWithIndex([this](StatName stat_name, size_t prefix_index) -> bool { + if (prefix_index == 0) { + prefix_ = stat_name; + } else if (prefix_index == 1) { + base_prefix_ = stat_name; + } else { + const size_t tag_index = (prefix_index - 2) / 2; + if (prefix_index % 2 == 0) { + prefix_tags_[tag_index].first = stat_name; + } else { + prefix_tags_[tag_index].second = stat_name; + } + } + return true; + }); } ThreadLocalStoreImpl::ScopeImpl::~ScopeImpl() { @@ -457,7 +527,7 @@ ThreadLocalStoreImpl::ScopeImpl::~ScopeImpl() { // releaseScopeCrossThread. For more details see the comment in // `ThreadLocalStoreImpl::iterHelper`, and the lock it takes prior to the loop. parent_.releaseScopeCrossThread(this); - prefix_.free(symbolTable()); + prefix_list_.clear(symbolTable()); } // Helper for managing the potential truncation of tags from the metric names and @@ -606,21 +676,28 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromTaggedName( StatName base_name, absl::optional stat_name_tags, StatName tagged_name) { + if (scopeRejectsAll()) { + return parent_.null_counter_; + } + + if (useExplicitTags()) { + const TagUtility::TagStatNameJoiner joiner(base_prefix_, prefix_tags_, prefix_, base_name, + stat_name_tags.value_or(StatNameTagSpan{}), + tagged_name, symbolTable()); + return getOrCreateCounterBase(joiner); + } + + // Legacy mode don't support explicit tagged name. If a non-empty `tagged_name` is provided, + // it becomes the counter's flat name (the tag-extracted `base_name` and tags are dropped). + // In otherwise, `base_name` and `stat_name_tags` are used to construct the counter's name. + // This keep the consistency with the previous behavior of the legacy mode. if (!tagged_name.empty()) { - // If a non-empty tagged_name (with tag values) is provided, it means the latest tag-aware API - // is being used. The legacy scope implementation does not retain scope-level tag metadata, so - // it uses tagged_name as both the tag-extracted name and the flat name and drops the tags. base_name = tagged_name; stat_name_tags = absl::nullopt; } - if (scopeRejectsAll()) { - return parent_.null_counter_; - } - // Determine the final name based on the prefix and the passed base_name. - const TagUtility::TagStatNameJoiner joiner(prefix_.statName(), base_name, stat_name_tags, - symbolTable()); + const TagUtility::TagStatNameJoiner joiner(prefix_, base_name, stat_name_tags, symbolTable()); return getOrCreateCounterBase(joiner); } @@ -670,22 +747,30 @@ void ThreadLocalStoreImpl::deliverHistogramToSinks(const Histogram& histogram, u Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromTaggedName( StatName base_name, absl::optional stat_name_tags, StatName tagged_name, Gauge::ImportMode import_mode) { + // If a gauge is "hidden" it should not be rejected as these are used for deferred stats. + if (scopeRejectsAll() && import_mode != Gauge::ImportMode::HiddenAccumulate) { + return parent_.null_gauge_; + } + + if (useExplicitTags()) { + const TagUtility::TagStatNameJoiner joiner(base_prefix_, prefix_tags_, prefix_, base_name, + stat_name_tags.value_or(StatNameTagSpan{}), + tagged_name, symbolTable()); + return getOrCreateGaugeBase(joiner, import_mode); + } + + // Legacy mode don't support explicit tagged name. If a non-empty `tagged_name` is provided, + // it becomes the counter's flat name (the tag-extracted `base_name` and tags are dropped). + // In otherwise, `base_name` and `stat_name_tags` are used to construct the counter's name. + // This keep the consistency with the previous behavior of the legacy mode. if (!tagged_name.empty()) { - // If a non-empty tagged_name (with tag values) is provided, it means the latest tag-aware API - // is being used. The legacy scope implementation does not retain scope-level tag metadata, so - // it uses tagged_name as both the tag-extracted name and the flat name and drops the tags. base_name = tagged_name; stat_name_tags = absl::nullopt; } - // If a gauge is "hidden" it should not be rejected as these are used for deferred stats. - if (scopeRejectsAll() && import_mode != Gauge::ImportMode::HiddenAccumulate) { - return parent_.null_gauge_; - } // See comments in counter(). There is no super clean way (via templates or otherwise) to // share this code so I'm leaving it largely duplicated for now. - const TagUtility::TagStatNameJoiner joiner(prefix_.statName(), base_name, stat_name_tags, - symbolTable()); + const TagUtility::TagStatNameJoiner joiner(prefix_, base_name, stat_name_tags, symbolTable()); return getOrCreateGaugeBase(joiner, import_mode); } @@ -730,22 +815,29 @@ ThreadLocalStoreImpl::ScopeImpl::getOrCreateGaugeBase(const TagUtility::TagStatN Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromTaggedName( StatName base_name, absl::optional stat_name_tags, StatName tagged_name, Histogram::Unit unit) { + if (scopeRejectsAll()) { + return parent_.null_histogram_; + } + + if (useExplicitTags()) { + const TagUtility::TagStatNameJoiner joiner(base_prefix_, prefix_tags_, prefix_, base_name, + stat_name_tags.value_or(StatNameTagSpan{}), + tagged_name, symbolTable()); + return getOrCreateHistogramBase(joiner, unit); + } + + // Legacy mode don't support explicit tagged name. If a non-empty `tagged_name` is provided, + // it becomes the counter's flat name (the tag-extracted `base_name` and tags are dropped). + // In otherwise, `base_name` and `stat_name_tags` are used to construct the counter's name. + // This keep the consistency with the previous behavior of the legacy mode. if (!tagged_name.empty()) { - // If a non-empty tagged_name (with tag values) is provided, it means the latest tag-aware API - // is being used. The legacy scope implementation does not retain scope-level tag metadata, so - // it uses tagged_name as both the tag-extracted name and the flat name and drops the tags. base_name = tagged_name; stat_name_tags = absl::nullopt; } - if (scopeRejectsAll()) { - return parent_.null_histogram_; - } - // See comments in counter(). There is no super clean way (via templates or otherwise) to // share this code so I'm leaving it largely duplicated for now. - TagUtility::TagStatNameJoiner joiner(prefix_.statName(), base_name, stat_name_tags, - symbolTable()); + TagUtility::TagStatNameJoiner joiner(prefix_, base_name, stat_name_tags, symbolTable()); return getOrCreateHistogramBase(joiner, unit); } @@ -830,21 +922,28 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::getOrCreateHistogramBase( TextReadout& ThreadLocalStoreImpl::ScopeImpl::textReadoutFromTaggedName( StatName base_name, absl::optional stat_name_tags, StatName tagged_name) { + if (scopeRejectsAll()) { + return parent_.null_text_readout_; + } + + if (useExplicitTags()) { + const TagUtility::TagStatNameJoiner joiner(base_prefix_, prefix_tags_, prefix_, base_name, + stat_name_tags.value_or(StatNameTagSpan{}), + tagged_name, symbolTable()); + return getOrCreateTextReadoutBase(joiner); + } + + // Legacy mode don't support explicit tagged name. If a non-empty `tagged_name` is provided, + // it becomes the counter's flat name (the tag-extracted `base_name` and tags are dropped). + // In otherwise, `base_name` and `stat_name_tags` are used to construct the counter's name. + // This keep the consistency with the previous behavior of the legacy mode. if (!tagged_name.empty()) { - // If a non-empty tagged_name (with tag values) is provided, it means the latest tag-aware API - // is being used. The legacy scope implementation does not retain scope-level tag metadata, so - // it uses tagged_name as both the tag-extracted name and the flat name and drops the tags. base_name = tagged_name; stat_name_tags = absl::nullopt; } - if (scopeRejectsAll()) { - return parent_.null_text_readout_; - } - // Determine the final name based on the prefix and the passed base_name. - TagUtility::TagStatNameJoiner joiner(prefix_.statName(), base_name, stat_name_tags, - symbolTable()); + TagUtility::TagStatNameJoiner joiner(prefix_, base_name, stat_name_tags, symbolTable()); return getOrCreateTextReadoutBase(joiner); } @@ -910,111 +1009,6 @@ TextReadoutOptConstRef ThreadLocalStoreImpl::ScopeImpl::findTextReadout(StatName return findStatLockHeld(name, central_cache_->text_readouts_); } -ThreadLocalStoreImpl::TagScopeImpl::TagScopeImpl(std::unique_ptr pool, StatName name, - StatNameTagSpan name_tags, StatName tagged_name, - ThreadLocalStoreImpl& store, bool evictable, - const ScopeStatsLimitSettings& limits, - StatsMatcherSharedPtr matcher) - : ScopeImpl(store, tagged_name, evictable, limits, std::move(matcher)), pool_(std::move(pool)), - tag_extracted_prefix_(name), prefix_tags_(name_tags.begin(), name_tags.end()) {} - -ScopeSharedPtr ThreadLocalStoreImpl::TagScopeImpl::createScopeWithTaggedName( - absl::string_view base_name, TagStringViewSpan name_tags, absl::string_view tagged_name, - bool evictable, const ScopeStatsLimitSettings& limits, StatsMatcherSharedPtr matcher) { - StatNamePool tag_pool(symbolTable()); - StatName stat_name = tag_pool.add(Utility::sanitizeStatsName(base_name)); - StatName stat_tagged_name; - if (!name_tags.empty()) { - // The tagged name is only meaningful when there are tags to interleave; otherwise it is - // ignored. - stat_tagged_name = - tagged_name.empty() ? StatName() : tag_pool.add(Utility::sanitizeStatsName(tagged_name)); - } - - StatNameTagVec stat_name_tags; - stat_name_tags.reserve(name_tags.size()); - for (const auto& [tag, value] : name_tags) { - stat_name_tags.emplace_back(tag_pool.add(tag), tag_pool.add(value)); - } - return scopeFromTaggedName(stat_name, stat_name_tags, stat_tagged_name, evictable, limits, - std::move(matcher)); -} - -ScopeSharedPtr ThreadLocalStoreImpl::TagScopeImpl::scopeFromTaggedName( - StatName base_name, StatNameTagSpan name_tags, StatName tagged_name, bool evictable, - const ScopeStatsLimitSettings& limits, StatsMatcherSharedPtr matcher) { - // Combine this scope's prefix and tags with the new scope element to derive the child's - // tag-extracted prefix, tagged prefix and accumulated tags. - const TagUtility::TagStatNameJoiner joiner(tag_extracted_prefix_, prefix_tags_, - prefix_.statName(), base_name, name_tags, tagged_name, - symbolTable()); - - auto child_pool = std::make_unique(symbolTable()); - const StatName child_tag_extracted_prefix = child_pool->add(joiner.tagExtractedName()); - StatNameTagVec child_prefix_tags; - if (const auto effective_tags = joiner.effectiveTags(); effective_tags.has_value()) { - child_prefix_tags.reserve(effective_tags->size()); - for (const auto& [tag, value] : *effective_tags) { - child_prefix_tags.emplace_back(child_pool->add(tag), child_pool->add(value)); - } - } - - // Use explicit matcher if provided; otherwise inherit scope_matcher_. - StatsMatcherSharedPtr child_matcher = matcher ? std::move(matcher) : scope_matcher_; - std::shared_ptr new_scope = std::make_shared( - std::move(child_pool), child_tag_extracted_prefix, std::move(child_prefix_tags), - joiner.nameWithTags(), parent_, evictable, limits, std::move(child_matcher)); - parent_.addScope(new_scope); - return new_scope; -} - -Counter& ThreadLocalStoreImpl::TagScopeImpl::counterFromTaggedName( - StatName base_name, absl::optional name_tags, StatName tagged_name) { - if (scopeRejectsAll()) { - return parent_.null_counter_; - } - const TagUtility::TagStatNameJoiner joiner(tag_extracted_prefix_, prefix_tags_, prefix(), - base_name, name_tags.value_or(StatNameTagSpan{}), - tagged_name, symbolTable()); - return getOrCreateCounterBase(joiner); -} - -Gauge& ThreadLocalStoreImpl::TagScopeImpl::gaugeFromTaggedName( - StatName base_name, absl::optional name_tags, StatName tagged_name, - Gauge::ImportMode import_mode) { - // If a gauge is "hidden" it should not be rejected as these are used for deferred stats. - if (scopeRejectsAll() && import_mode != Gauge::ImportMode::HiddenAccumulate) { - return parent_.null_gauge_; - } - const TagUtility::TagStatNameJoiner joiner(tag_extracted_prefix_, prefix_tags_, prefix(), - base_name, name_tags.value_or(StatNameTagSpan{}), - tagged_name, symbolTable()); - return getOrCreateGaugeBase(joiner, import_mode); -} - -Histogram& ThreadLocalStoreImpl::TagScopeImpl::histogramFromTaggedName( - StatName base_name, absl::optional name_tags, StatName tagged_name, - Histogram::Unit unit) { - if (scopeRejectsAll()) { - return parent_.null_histogram_; - } - const TagUtility::TagStatNameJoiner joiner(tag_extracted_prefix_, prefix_tags_, prefix(), - base_name, name_tags.value_or(StatNameTagSpan{}), - tagged_name, symbolTable()); - return getOrCreateHistogramBase(joiner, unit); -} - -TextReadout& ThreadLocalStoreImpl::TagScopeImpl::textReadoutFromTaggedName( - StatName base_name, absl::optional name_tags, StatName tagged_name) { - if (scopeRejectsAll()) { - return parent_.null_text_readout_; - } - const TagUtility::TagStatNameJoiner joiner(tag_extracted_prefix_, prefix_tags_, prefix(), - base_name, name_tags.value_or(StatNameTagSpan{}), - tagged_name, symbolTable()); - return getOrCreateTextReadoutBase(joiner); -} - Histogram& ThreadLocalStoreImpl::tlsHistogram(ParentHistogramImpl& parent, uint64_t id) { // tlsHistogram() is generally not called for a histogram that is rejected by // the matcher, so no further rejection-checking is needed at this level. diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 9b117059d429f..c30fd36019b9c 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -167,7 +167,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo static const char IterateScopeSync[]; static const char MainDispatcherCleanupSync[]; - ThreadLocalStoreImpl(Allocator& alloc, bool use_tag_scope = false); + explicit ThreadLocalStoreImpl(Allocator& alloc); ~ThreadLocalStoreImpl() override; // Stats::Store NullCounterImpl& nullCounter() override { return null_counter_; } @@ -202,6 +202,16 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo } void setStatsMatcher(StatsMatcherPtr&& stats_matcher) override; void setHistogramSettings(HistogramSettingsConstPtr&& histogram_settings) override; + // Enables/disables the explicit-tags logic store-wide. Safe to call during single-threaded + // startup even after some scopes exist: a scope created in legacy mode has no explicit_tag_data_, + // and the explicit-tags path falls back (via ScopeImpl::explicitTagPrefix()) to behavior + // identical to legacy mode for it. Must be set before threading is initialized. + void setUseExplicitTags(bool use_explicit_tags) override { + use_explicit_tags_ = use_explicit_tags; + } + // Whether the store is using the explicit-tags logic. Exposed primarily so tests can verify the + // value selected during server initialization. + bool useExplicitTags() const { return use_explicit_tags_; } void initializeThreading(Event::Dispatcher& main_thread_dispatcher, ThreadLocal::Instance& tls) override; void shutdownThreading() override; @@ -293,6 +303,12 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo ScopeImpl(ThreadLocalStoreImpl& parent, StatName prefix, bool evictable, const ScopeStatsLimitSettings& limits = {}, StatsMatcherSharedPtr scope_matcher = nullptr); + // Explicit-tags constructor. Used when the store is in explicit-tags mode + // (use_explicit_tags_ == true). + ScopeImpl(StatName base_name, StatNameTagSpan name_tags, StatName tagged_name, + ThreadLocalStoreImpl& parent, bool evictable, + const ScopeStatsLimitSettings& limits = {}, + StatsMatcherSharedPtr scope_matcher = nullptr); ~ScopeImpl() override; void setCleanupCallback(std::function callback) override { @@ -453,7 +469,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo return std::cref(*iter->second); } - StatName prefix() const override { return prefix_.statName(); } + StatName prefix() const override { return prefix_; } // Returns the central cache, asserting that the parent lock is held. // @@ -489,6 +505,14 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo return effectiveMatcher().fastRejects(name); } + // Whether this scope uses the explicit-tags logic (propagating scope-level tags) or the legacy + // logic (which drops scope-level tag metadata). This mirrors the store-level flag, which is the + // single source of truth. Note that explicit_tag_data_ may still be null when this is true: a + // scope created in legacy mode before setUseExplicitTags() enabled the flag has no + // explicit_tag_data_. Use explicitTagPrefix() (not explicit_tag_data_ directly) so that case is + // handled. + bool useExplicitTags() const { return parent_.use_explicit_tags_; } + const uint64_t scope_id_; ThreadLocalStoreImpl& parent_; const bool evictable_{}; @@ -496,53 +520,15 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo const ScopeStatsLimitSettings limits_; StatsMatcherSharedPtr scope_matcher_; - protected: - StatNameStorage prefix_; + StatNameList prefix_list_; + StatName prefix_; + StatName base_prefix_; + std::vector prefix_tags_; + mutable CentralCacheEntrySharedPtr central_cache_ ABSL_GUARDED_BY(parent_.lock_); std::function cleanup_callback_; }; - // Tag-aware scope for the thread-local store. It tracks a tag-extracted prefix separately from - // the prefix (the base ScopeImpl's prefix_, which may carry interspersed tag values) and a set - // of scope-level prefix tags propagated onto every stat it creates. The tag-aware Scope APIs are - // implemented here; see TagStatNameJoiner. - class TagScopeImpl : public ScopeImpl { - public: - // `name` is the scope's tag-extracted prefix; `tagged_name` is its flat prefix with tag - // values interleaved (stored in the base ScopeImpl's prefix_). - TagScopeImpl(std::unique_ptr pool, StatName name, StatNameTagSpan name_tags, - StatName tagged_name, ThreadLocalStoreImpl& store, bool evictable, - const ScopeStatsLimitSettings& limits = {}, - StatsMatcherSharedPtr matcher = nullptr); - - ScopeSharedPtr createScopeWithTaggedName(absl::string_view base_name, - TagStringViewSpan name_tags, - absl::string_view tagged_name, bool evictable, - const ScopeStatsLimitSettings& limits, - StatsMatcherSharedPtr matcher) override; - ScopeSharedPtr scopeFromTaggedName(StatName base_name, StatNameTagSpan name_tags, - StatName tagged_name, bool evictable, - const ScopeStatsLimitSettings& limits, - StatsMatcherSharedPtr matcher) override; - Counter& counterFromTaggedName(StatName base_name, absl::optional name_tags, - StatName tagged_name) override; - Gauge& gaugeFromTaggedName(StatName base_name, absl::optional name_tags, - StatName tagged_name, Gauge::ImportMode import_mode) override; - Histogram& histogramFromTaggedName(StatName base_name, - absl::optional name_tags, - StatName tagged_name, Histogram::Unit unit) override; - TextReadout& textReadoutFromTaggedName(StatName base_name, - absl::optional name_tags, - StatName tagged_name) override; - - private: - std::unique_ptr pool_; - // The scope's tag-extracted prefix. Paired with the base ScopeImpl's prefix_ - // (which holds the tagged/flat prefix) and prefix_tags_. - StatName tag_extracted_prefix_; - StatNameTagVec prefix_tags_; - }; - struct TlsCache : public ThreadLocal::ThreadLocalObject { TlsCacheEntry& insertScope(uint64_t scope_id); void eraseScopes(const std::vector& scope_ids); @@ -648,8 +634,12 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo uint64_t next_histogram_id_ ABSL_GUARDED_BY(hist_mutex_) = 0; StatNameSetPtr well_known_tags_; - // When true, the default scope is a TagScopeImpl, enabling the tag-aware Scope APIs. - const bool use_tag_scope_ = false; + // When true, scopes use the explicit-tags logic (ScopeImpl::useExplicitTags()), enabling the + // explicit-tags Scope APIs that propagate scope-level tags. When false, the legacy logic is used. + // Set once via setUseExplicitTags() during single-threaded startup (see the setter's contract); + // the root scope is always created explicit-tag-capable so this can be enabled without recreating + // it. + bool use_explicit_tags_ = false; mutable Thread::MutexBasicLockable hist_mutex_; StatSet histogram_set_ ABSL_GUARDED_BY(hist_mutex_); diff --git a/source/server/server.cc b/source/server/server.cc index 5a59706b1428c..b977c95dcc71d 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -482,6 +482,22 @@ absl::Status InstanceBase::initializeOrThrow(Network::Address::InstanceConstShar bootstrap_, options_, messageValidationContext().staticValidationVisitor(), *api_)); bootstrap_config_update_time_ = time_source_.systemTime(); + // Decide whether the stats store should use the explicit-tags logic. The explicit-tags logic + // use the tags specified by the caller when creating a stat and will ignore any tags extraction + // rules. + // To keep the backwards compatibility, we only enable the explicit-tags logic if the user + // has not specified any custom tags extraction rules and has not disabled the use of default + // tags. + { + const auto& stats_config = bootstrap_.stats_config(); + const bool use_all_default_tags = + !stats_config.has_use_all_default_tags() || stats_config.use_all_default_tags().value(); + if (stats_config.stats_tags().empty() && use_all_default_tags && + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.enable_stats_explicit_tags")) { + stats_store_.setUseExplicitTags(true); + } + } + if (bootstrap_.has_application_log_config()) { RETURN_IF_NOT_OK( Utility::assertExclusiveLogFormatMethod(options_, bootstrap_.application_log_config())); diff --git a/test/common/stats/tag_utility_test.cc b/test/common/stats/tag_utility_test.cc index 43b66fb886ebb..1b42f050f393c 100644 --- a/test/common/stats/tag_utility_test.cc +++ b/test/common/stats/tag_utility_test.cc @@ -34,7 +34,7 @@ TEST_F(TagUtilityTest, Dynamic) { EXPECT_EQ("prefix.name", symbol_table_.toString(joiner.tagExtractedName())); } -// With no tags and no explicit name, the tag-aware constructor behaves like a plain +// With no tags and no explicit name, the explicit-tags constructor behaves like a plain // tag_extracted_prefix + tag_extracted_name join. // Constructor arg order: // (tag_extracted_prefix, prefix_tags, prefix, tag_extracted_name, name_tags, name, symbol_table). diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 4df840578c6ee..4a1797a88f514 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -244,6 +244,21 @@ class HistogramTest : public testing::Test { h2_interval_values_; }; +// setUseExplicitTags() can be enabled at any time, even after scopes have been created in legacy +// mode. Such scopes have no explicit_tag_data_ and fall back (via ScopeImpl::explicitTagPrefix()) +// to behavior identical to legacy mode, so stats created in them still resolve correctly. +TEST_F(StatsThreadLocalStoreTest, SetUseExplicitTagsWithPreExistingLegacyScope) { + ScopeSharedPtr legacy_scope = store_->rootScope()->createScope("cluster.foo"); + + store_->setUseExplicitTags(true); + EXPECT_TRUE(store_->useExplicitTags()); + + // A stat created in the pre-existing (null explicit_tag_data_) scope resolves via the fallback, + // exactly as it would have in legacy mode. + Counter& c = legacy_scope->counterFromString("rq"); + EXPECT_EQ("cluster.foo.rq", c.name()); +} + TEST_F(StatsThreadLocalStoreTest, NoTls) { InSequence s; @@ -2699,15 +2714,15 @@ TEST_F(StatsThreadLocalStoreTest, SetSinkPredicates) { EXPECT_EQ(expected_sinked_stats, num_sinked_text_readouts); } -// Exercises the tag-aware scope (TagScopeImpl) of the thread-local store, enabled via the -// use_tag_scope constructor flag. -class ThreadLocalStoreTagScopeTest : public testing::Test { +// Exercises the explicit-tags logic of the thread-local store, enabled via setUseExplicitTags(). +class ThreadLocalStoreExplicitTagsTest : public testing::Test { public: - ThreadLocalStoreTagScopeTest() - : alloc_(symbol_table_), - store_(std::make_unique(alloc_, /*use_tag_scope=*/true)), - pool_(symbol_table_), scope_(*store_->rootScope()) {} - ~ThreadLocalStoreTagScopeTest() override { + ThreadLocalStoreExplicitTagsTest() + : alloc_(symbol_table_), store_(std::make_unique(alloc_)), + pool_(symbol_table_), scope_(*store_->rootScope()) { + store_->setUseExplicitTags(true); + } + ~ThreadLocalStoreExplicitTagsTest() override { tls_.shutdownGlobalThreading(); store_->shutdownThreading(); tls_.shutdownThread(); @@ -2725,7 +2740,7 @@ class ThreadLocalStoreTagScopeTest : public testing::Test { // The explicit `tagged_name` controls the flat stat name while name_tags are still recorded; // `name` yields the tag-extracted name. -TEST_F(ThreadLocalStoreTagScopeTest, CounterNameAndNameTags) { +TEST_F(ThreadLocalStoreExplicitTagsTest, CounterNameAndNameTags) { StatNameTagVector name_tags{{makeStatName("cluster_name"), makeStatName("foo")}}; Counter& c = scope_.counterFromTaggedName(makeStatName("cluster.upstream_rq"), StatNameTagSpan(name_tags), @@ -2744,7 +2759,7 @@ TEST_F(ThreadLocalStoreTagScopeTest, CounterNameAndNameTags) { // A child scope created with name_tags + an explicit tagged_name propagates the tag to child stats // and interleaves the tag value without double-counting. -TEST_F(ThreadLocalStoreTagScopeTest, ScopeTagsPropagate) { +TEST_F(ThreadLocalStoreExplicitTagsTest, ScopeTagsPropagate) { StatNameTagVector name_tags{{makeStatName("cluster_name"), makeStatName("foo")}}; ScopeSharedPtr cluster_scope = scope_.scopeFromTaggedName( makeStatName("cluster"), StatNameTagSpan(name_tags), makeStatName("cluster.foo")); @@ -2764,8 +2779,8 @@ TEST_F(ThreadLocalStoreTagScopeTest, ScopeTagsPropagate) { ASSERT_EQ(1, g.tags().size()); } -// The legacy createScope/counter APIs still work on a tag-aware scope. -TEST_F(ThreadLocalStoreTagScopeTest, LegacyScopeApiStillWorks) { +// The legacy createScope/counter APIs still work on a explicit-tags scope. +TEST_F(ThreadLocalStoreExplicitTagsTest, LegacyScopeApiStillWorks) { ScopeSharedPtr child = scope_.createScope("a.b"); Counter& c = child->counterFromString("c"); EXPECT_EQ("a.b.c", c.name()); @@ -2775,7 +2790,7 @@ TEST_F(ThreadLocalStoreTagScopeTest, LegacyScopeApiStillWorks) { // The string_view createScope interns its name_tags and tagged_name and propagates the tag to child // stats, exercising the TagStringViewSpan path. -TEST_F(ThreadLocalStoreTagScopeTest, CreateScopeWithTagStringViews) { +TEST_F(ThreadLocalStoreExplicitTagsTest, CreateScopeWithTagStringViews) { std::vector name_tags{{"cluster_name", "foo"}}; ScopeSharedPtr cluster_scope = scope_.createScopeWithTaggedName("cluster", name_tags, "cluster.foo"); @@ -2789,9 +2804,9 @@ TEST_F(ThreadLocalStoreTagScopeTest, CreateScopeWithTagStringViews) { EXPECT_EQ("foo", c.tags()[0].value_); } -// Covers TagScopeImpl::histogramFromStatName and textReadoutFromStatName when the scope carries -// inherited tags: the tag propagates as metadata and the prefix interleaves the value. -TEST_F(ThreadLocalStoreTagScopeTest, HistogramAndTextReadoutTagsPropagate) { +// Covers histogramFromStatName and textReadoutFromStatName in explicit-tags mode when the scope +// carries inherited tags: the tag propagates as metadata and the prefix interleaves the value. +TEST_F(ThreadLocalStoreExplicitTagsTest, HistogramAndTextReadoutTagsPropagate) { StatNameTagVector name_tags{{makeStatName("cluster_name"), makeStatName("foo")}}; ScopeSharedPtr cluster_scope = scope_.scopeFromTaggedName( makeStatName("cluster"), StatNameTagSpan(name_tags), makeStatName("cluster.foo")); @@ -2811,9 +2826,10 @@ TEST_F(ThreadLocalStoreTagScopeTest, HistogramAndTextReadoutTagsPropagate) { EXPECT_EQ("foo", t.tags()[0].value_); } -// Exercises the backward-compat branches in the legacy ScopeImpl (use_tag_scope=false): when the -// new tag-aware createScope / scopeFromStatName / *FromStatName APIs are called with non-empty -// `tagged_name`, the legacy path uses `tagged_name` as the scope/stat name and drops the tags. +// Exercises the backward-compat branches in the legacy ScopeImpl (use_explicit_tags=false): when +// the new explicit-tags createScope / scopeFromStatName / *FromStatName APIs are called with +// non-empty `tagged_name`, the legacy path uses `tagged_name` as the scope/stat name and drops the +// tags. TEST_F(StatsThreadLocalStoreTest, LegacyScopeBackwardCompatWithExplicitArgs) { StatNamePool pool(symbol_table_); @@ -2825,6 +2841,26 @@ TEST_F(StatsThreadLocalStoreTest, LegacyScopeBackwardCompatWithExplicitArgs) { // Tag extraction runs against the flat name on the legacy path; well-known tags may still match. EXPECT_EQ(0, c.tags().size()); + // Gauge/histogram/text-readout behave the same as counter on the legacy path: a non-empty + // tagged_name overrides `name` and the tags are dropped. Covers the backward-compat branch of + // each *FromTaggedName method. + Gauge& g = scope_.gaugeFromTaggedName(pool.add("active"), StatNameTagSpan(tags), + pool.add("active.cluster_name.foo"), + Gauge::ImportMode::Accumulate); + EXPECT_EQ("active.cluster_name.foo", g.name()); + EXPECT_EQ(0, g.tags().size()); + + Histogram& h = scope_.histogramFromTaggedName(pool.add("latency"), StatNameTagSpan(tags), + pool.add("latency.cluster_name.foo"), + Histogram::Unit::Milliseconds); + EXPECT_EQ("latency.cluster_name.foo", h.name()); + EXPECT_EQ(0, h.tags().size()); + + TextReadout& t = scope_.textReadoutFromTaggedName(pool.add("version"), StatNameTagSpan(tags), + pool.add("version.cluster_name.foo")); + EXPECT_EQ("version.cluster_name.foo", t.name()); + EXPECT_EQ(0, t.tags().size()); + // createScope: explicit (non-empty) tagged_name overrides `name`; tags are dropped. std::vector sv_tags{{"cluster_name", "foo"}}; ScopeSharedPtr child = scope_.createScopeWithTaggedName("cluster", sv_tags, "svc.foo"); @@ -2912,10 +2948,10 @@ TEST_F(StatsThreadLocalStoreTest, LegacyScopeCreationMatrix) { EXPECT_EQ("e.y.c", s4->counterFromString("c").name()); } -// TagScopeImpl stat-creation matrix on the root scope (no inherited tags). Per-stat name_tags are +// Explicit-tags stat-creation matrix on the root scope (no inherited tags). Per-stat name_tags are // honored. tagged_name, when name_tags are present, supplies the flat name verbatim; when // name_tags are empty, tagged_name is ignored and `name` is used. -TEST_F(ThreadLocalStoreTagScopeTest, TagStatCreationMatrixOnPlainScope) { +TEST_F(ThreadLocalStoreExplicitTagsTest, TagStatCreationMatrixOnPlainScope) { // No name_tags, no tagged_name. Counter& c1 = scope_.counterFromTaggedName(makeStatName("rq"), absl::nullopt, StatName()); EXPECT_EQ("rq", c1.name()); @@ -2948,9 +2984,9 @@ TEST_F(ThreadLocalStoreTagScopeTest, TagStatCreationMatrixOnPlainScope) { ASSERT_EQ(1, c4.tags().size()); } -// TagScopeImpl scope-creation matrix. For each variation of (name_tags, tagged_name) creating a +// Explicit-tags scope-creation matrix. For each variation of (name_tags, tagged_name) creating a // child scope, verify the child's prefix and the names/tag metadata of stats created in the child. -TEST_F(ThreadLocalStoreTagScopeTest, TagScopeCreationMatrix) { +TEST_F(ThreadLocalStoreExplicitTagsTest, ExplicitTagsScopeCreationMatrix) { // No name_tags, no tagged_name -> child has flat == canonical == "a". ScopeSharedPtr s1 = scope_.scopeFromTaggedName(makeStatName("a"), StatNameTagSpan{}, StatName()); EXPECT_EQ("a", symbol_table_.toString(s1->prefix())); @@ -2988,10 +3024,10 @@ TEST_F(ThreadLocalStoreTagScopeTest, TagScopeCreationMatrix) { EXPECT_EQ("v", s4c.tags()[0].value_); } -// TagScopeImpl stat-creation matrix on a scope that already carries inherited tags. The inherited +// Explicit-tags stat-creation matrix on a scope that already carries inherited tags. The inherited // scope tag must show up in every child stat regardless of whether the stat itself supplies tags // or a tagged_name. -TEST_F(ThreadLocalStoreTagScopeTest, TagStatCreationMatrixOnTaggedScope) { +TEST_F(ThreadLocalStoreExplicitTagsTest, TagStatCreationMatrixOnTaggedScope) { StatNameTagVector prefix_tags{{makeStatName("cluster_name"), makeStatName("foo")}}; ScopeSharedPtr cluster = scope_.scopeFromTaggedName( makeStatName("cluster"), StatNameTagSpan(prefix_tags), makeStatName("cluster.foo")); diff --git a/test/integration/server.h b/test/integration/server.h index 4248b59503d4d..a5df5150fec7d 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -399,6 +399,7 @@ class TestIsolatedStoreImpl : public StoreRoot { void setTagProducer(TagProducerPtr&&) override {} void setStatsMatcher(StatsMatcherPtr&&) override {} void setHistogramSettings(HistogramSettingsConstPtr&&) override {} + void setUseExplicitTags(bool) override {} void initializeThreading(Event::Dispatcher&, ThreadLocal::Instance&) override {} void shutdownThreading() override {} void mergeHistograms(PostMergeCb cb) override { merge_cb_ = cb; } diff --git a/test/server/BUILD b/test/server/BUILD index 985619bedb0ac..aee8afd867768 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -392,6 +392,10 @@ envoy_cc_test( rbe_pool = "6gig", deps = [ "//source/common/common:notification_lib", + "//source/common/runtime:runtime_features_lib", + "//source/common/stats:allocator_lib", + "//source/common/stats:symbol_table_lib", + "//source/common/stats:thread_local_store_lib", "//source/common/version:version_lib", "//source/extensions/access_loggers/file:config", "//source/extensions/clusters/dns:dns_cluster_lib", diff --git a/test/server/server_test.cc b/test/server/server_test.cc index f6ee0cf4dc516..9d8190bf0e7ac 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -14,6 +14,10 @@ #include "source/common/network/listen_socket_impl.h" #include "source/common/network/socket_option_impl.h" #include "source/common/protobuf/protobuf.h" +#include "source/common/runtime/runtime_features.h" +#include "source/common/stats/allocator_impl.h" +#include "source/common/stats/symbol_table.h" +#include "source/common/stats/thread_local_store.h" #include "source/common/thread_local/thread_local_impl.h" #include "source/common/version/version.h" #include "source/server/instance_impl.h" @@ -547,6 +551,64 @@ TEST_P(ServerInstanceImplTest, WithCustomInlineHeaders) { } } +// Boots a server backed by a real ThreadLocalStoreImpl (the production store type, unlike the base +// fixture's TestIsolatedStoreImpl) and verifies the explicit-tags decision made during +// initialization, observed via ThreadLocalStoreImpl::useExplicitTags(). These live in the +// ServerInstanceImplTest suite, after WithCustomInlineHeaders, because booting a server finalizes +// the process-global custom-inline-header registry that the inline-header tests must populate +// first. +// +// With the runtime guard enabled and a default stats config (no custom tags), server initialization +// turns on the explicit-tags logic on the real stats store. Guards against a regression where an +// early scope creation would silently cause setUseExplicitTags() to be ignored. +TEST_P(ServerInstanceImplTest, ExplicitTagsEnabledByRuntimeGuard) { + Runtime::maybeSetRuntimeGuard("envoy.reloadable_features.enable_stats_explicit_tags", true); + Stats::SymbolTableImpl symbol_table; + Stats::AllocatorImpl allocator(symbol_table); + Stats::ThreadLocalStoreImpl real_store(allocator); + + options_.config_path_ = TestEnvironment::temporaryFileSubstitute( + "test/server/test_data/server/empty_bootstrap.yaml", {}, version_); + thread_local_ = std::make_unique(); + init_manager_ = std::make_unique("Server"); + server_ = std::make_unique( + *init_manager_, options_, time_system_, hooks_, restart_, real_store, fakelock_, + std::make_unique>(), *thread_local_, + Thread::threadFactoryForTest(), Filesystem::fileSystemForTest(), nullptr); + server_->initialize(std::make_shared("127.0.0.1"), + component_factory_); + EXPECT_TRUE(real_store.useExplicitTags()); + + // Tear down the server (which shuts down threading on real_store) before real_store is destroyed, + // and restore the process-global runtime flag so it does not leak into other tests. + server_.reset(); + thread_local_.reset(); + Runtime::maybeSetRuntimeGuard("envoy.reloadable_features.enable_stats_explicit_tags", false); +} + +// With the runtime guard at its default (false), the explicit-tags logic stays off after init. +TEST_P(ServerInstanceImplTest, ExplicitTagsDisabledByDefault) { + Runtime::maybeSetRuntimeGuard("envoy.reloadable_features.enable_stats_explicit_tags", false); + Stats::SymbolTableImpl symbol_table; + Stats::AllocatorImpl allocator(symbol_table); + Stats::ThreadLocalStoreImpl real_store(allocator); + + options_.config_path_ = TestEnvironment::temporaryFileSubstitute( + "test/server/test_data/server/empty_bootstrap.yaml", {}, version_); + thread_local_ = std::make_unique(); + init_manager_ = std::make_unique("Server"); + server_ = std::make_unique( + *init_manager_, options_, time_system_, hooks_, restart_, real_store, fakelock_, + std::make_unique>(), *thread_local_, + Thread::threadFactoryForTest(), Filesystem::fileSystemForTest(), nullptr); + server_->initialize(std::make_shared("127.0.0.1"), + component_factory_); + EXPECT_FALSE(real_store.useExplicitTags()); + + server_.reset(); + thread_local_.reset(); +} + // Validates that server stats are flushed even when server is stuck with initialization. TEST_P(ServerInstanceImplTest, StatsFlushWhenServerIsStillInitializing) { CustomStatsSinkFactory factory; From 955394e4606ad9314b95a2263335dd9d14e6c0f8 Mon Sep 17 00:00:00 2001 From: wbpcode/wangbaiping Date: Fri, 26 Jun 2026 09:14:26 +0000 Subject: [PATCH 2/6] revolve conflict Signed-off-by: wbpcode/wangbaiping --- source/common/stats/thread_local_store.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 42b57d06ad9d3..e48a44265854c 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -675,7 +675,7 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( } Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromTaggedName( - StatName base_name, absl::optional stat_name_tags, StatName tagged_name) { + StatName base_name, std::optional stat_name_tags, StatName tagged_name) { if (scopeRejectsAll()) { return parent_.null_counter_; } @@ -921,7 +921,7 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::getOrCreateHistogramBase( } TextReadout& ThreadLocalStoreImpl::ScopeImpl::textReadoutFromTaggedName( - StatName base_name, absl::optional stat_name_tags, StatName tagged_name) { + StatName base_name, std::optional stat_name_tags, StatName tagged_name) { if (scopeRejectsAll()) { return parent_.null_text_readout_; } From db309812b0c54d4ebe8114d7ffce0de73d86139e Mon Sep 17 00:00:00 2001 From: wbpcode/wangbaiping Date: Mon, 29 Jun 2026 10:11:18 +0000 Subject: [PATCH 3/6] fix format Signed-off-by: wbpcode/wangbaiping --- source/common/stats/thread_local_store.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index e48a44265854c..8c87743d89871 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -693,7 +693,7 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromTaggedName( // This keep the consistency with the previous behavior of the legacy mode. if (!tagged_name.empty()) { base_name = tagged_name; - stat_name_tags = absl::nullopt; + stat_name_tags = std::nullopt; } // Determine the final name based on the prefix and the passed base_name. @@ -765,7 +765,7 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromTaggedName( // This keep the consistency with the previous behavior of the legacy mode. if (!tagged_name.empty()) { base_name = tagged_name; - stat_name_tags = absl::nullopt; + stat_name_tags = std::nullopt; } // See comments in counter(). There is no super clean way (via templates or otherwise) to @@ -832,7 +832,7 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromTaggedName( // This keep the consistency with the previous behavior of the legacy mode. if (!tagged_name.empty()) { base_name = tagged_name; - stat_name_tags = absl::nullopt; + stat_name_tags = std::nullopt; } // See comments in counter(). There is no super clean way (via templates or otherwise) to @@ -939,7 +939,7 @@ TextReadout& ThreadLocalStoreImpl::ScopeImpl::textReadoutFromTaggedName( // This keep the consistency with the previous behavior of the legacy mode. if (!tagged_name.empty()) { base_name = tagged_name; - stat_name_tags = absl::nullopt; + stat_name_tags = std::nullopt; } // Determine the final name based on the prefix and the passed base_name. From a506fc0e63b4c2b3832746f616574f159ebff696 Mon Sep 17 00:00:00 2001 From: wbpcode/wangbaiping Date: Mon, 29 Jun 2026 11:14:28 +0000 Subject: [PATCH 4/6] fix comments Signed-off-by: wbpcode/wangbaiping --- source/common/stats/thread_local_store.cc | 24 +++++++++++------------ 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 8c87743d89871..b49b3fc672035 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -216,7 +216,7 @@ ScopeSharedPtr ThreadLocalStoreImpl::ScopeImpl::scopeFromTaggedName( // re-interns them into the child's own pool). const TagUtility::TagStatNameJoiner joiner(base_prefix_, prefix_tags_, prefix_, base_name, name_tags, tagged_name, symbolTable()); - std::shared_ptr new_scope = std::make_shared( + auto new_scope = std::make_shared( joiner.tagExtractedName(), joiner.effectiveTags().value_or(StatNameTagSpan{}), joiner.nameWithTags(), parent_, evictable, limits, std::move(child_matcher)); parent_.addScope(new_scope); @@ -489,10 +489,8 @@ ThreadLocalStoreImpl::ScopeImpl::ScopeImpl(StatName base_name, StatNameTagSpan n central_cache_(new CentralCacheEntry(parent.alloc_.symbolTable())) { parent_.ensureOverflowStats(limits_); - // Explicit-tags mode: prefix_ is the tag-extracted prefix, and base_prefix_ is the flat (tagged) - // prefix. The tag-extracted prefix is used for stat name interning and the flat prefix is used - // for stat name formatting. The tag-extracted prefix is always valid, but the flat prefix may be - // empty if the scope is created with an empty prefix and no tags. + // Explicit-tags mode: ``prefix_`` is the flat (tagged) prefix and ``base_prefix_`` is the + // tag-extracted prefix. parent_.alloc_.symbolTable().populateList(tagged_name, base_name, name_tags, prefix_list_); ASSERT(prefix_list_.size() == 2 + name_tags.size() * 2); prefix_tags_.resize(name_tags.size()); @@ -688,8 +686,8 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromTaggedName( } // Legacy mode don't support explicit tagged name. If a non-empty `tagged_name` is provided, - // it becomes the counter's flat name (the tag-extracted `base_name` and tags are dropped). - // In otherwise, `base_name` and `stat_name_tags` are used to construct the counter's name. + // it becomes the stat's flat name (the tag-extracted `base_name` and tags are dropped). + // In otherwise, `base_name` and `stat_name_tags` are used to construct the stat's name. // This keep the consistency with the previous behavior of the legacy mode. if (!tagged_name.empty()) { base_name = tagged_name; @@ -760,8 +758,8 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromTaggedName( } // Legacy mode don't support explicit tagged name. If a non-empty `tagged_name` is provided, - // it becomes the counter's flat name (the tag-extracted `base_name` and tags are dropped). - // In otherwise, `base_name` and `stat_name_tags` are used to construct the counter's name. + // it becomes the stat's flat name (the tag-extracted `base_name` and tags are dropped). + // In otherwise, `base_name` and `stat_name_tags` are used to construct the stat's name. // This keep the consistency with the previous behavior of the legacy mode. if (!tagged_name.empty()) { base_name = tagged_name; @@ -827,8 +825,8 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromTaggedName( } // Legacy mode don't support explicit tagged name. If a non-empty `tagged_name` is provided, - // it becomes the counter's flat name (the tag-extracted `base_name` and tags are dropped). - // In otherwise, `base_name` and `stat_name_tags` are used to construct the counter's name. + // it becomes the stat's flat name (the tag-extracted `base_name` and tags are dropped). + // In otherwise, `base_name` and `stat_name_tags` are used to construct the stat's name. // This keep the consistency with the previous behavior of the legacy mode. if (!tagged_name.empty()) { base_name = tagged_name; @@ -934,8 +932,8 @@ TextReadout& ThreadLocalStoreImpl::ScopeImpl::textReadoutFromTaggedName( } // Legacy mode don't support explicit tagged name. If a non-empty `tagged_name` is provided, - // it becomes the counter's flat name (the tag-extracted `base_name` and tags are dropped). - // In otherwise, `base_name` and `stat_name_tags` are used to construct the counter's name. + // it becomes the stat's flat name (the tag-extracted `base_name` and tags are dropped). + // In otherwise, `base_name` and `stat_name_tags` are used to construct the stat's name. // This keep the consistency with the previous behavior of the legacy mode. if (!tagged_name.empty()) { base_name = tagged_name; From 216f167afcf73874271eae1c3077462aa61134bd Mon Sep 17 00:00:00 2001 From: wbpcode/wangbaiping Date: Mon, 29 Jun 2026 11:43:58 +0000 Subject: [PATCH 5/6] try make msan happy Signed-off-by: wbpcode/wangbaiping --- test/server/server_test.cc | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 9d8190bf0e7ac..8e6ae6c473657 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -565,23 +565,24 @@ TEST_P(ServerInstanceImplTest, ExplicitTagsEnabledByRuntimeGuard) { Runtime::maybeSetRuntimeGuard("envoy.reloadable_features.enable_stats_explicit_tags", true); Stats::SymbolTableImpl symbol_table; Stats::AllocatorImpl allocator(symbol_table); - Stats::ThreadLocalStoreImpl real_store(allocator); + auto real_store = std::make_unique(allocator); options_.config_path_ = TestEnvironment::temporaryFileSubstitute( "test/server/test_data/server/empty_bootstrap.yaml", {}, version_); thread_local_ = std::make_unique(); init_manager_ = std::make_unique("Server"); server_ = std::make_unique( - *init_manager_, options_, time_system_, hooks_, restart_, real_store, fakelock_, + *init_manager_, options_, time_system_, hooks_, restart_, *real_store, fakelock_, std::make_unique>(), *thread_local_, Thread::threadFactoryForTest(), Filesystem::fileSystemForTest(), nullptr); server_->initialize(std::make_shared("127.0.0.1"), component_factory_); - EXPECT_TRUE(real_store.useExplicitTags()); + EXPECT_TRUE(real_store->useExplicitTags()); // Tear down the server (which shuts down threading on real_store) before real_store is destroyed, // and restore the process-global runtime flag so it does not leak into other tests. server_.reset(); + real_store.reset(); thread_local_.reset(); Runtime::maybeSetRuntimeGuard("envoy.reloadable_features.enable_stats_explicit_tags", false); } @@ -591,21 +592,22 @@ TEST_P(ServerInstanceImplTest, ExplicitTagsDisabledByDefault) { Runtime::maybeSetRuntimeGuard("envoy.reloadable_features.enable_stats_explicit_tags", false); Stats::SymbolTableImpl symbol_table; Stats::AllocatorImpl allocator(symbol_table); - Stats::ThreadLocalStoreImpl real_store(allocator); + auto real_store = std::make_unique(allocator); options_.config_path_ = TestEnvironment::temporaryFileSubstitute( "test/server/test_data/server/empty_bootstrap.yaml", {}, version_); thread_local_ = std::make_unique(); init_manager_ = std::make_unique("Server"); server_ = std::make_unique( - *init_manager_, options_, time_system_, hooks_, restart_, real_store, fakelock_, + *init_manager_, options_, time_system_, hooks_, restart_, *real_store, fakelock_, std::make_unique>(), *thread_local_, Thread::threadFactoryForTest(), Filesystem::fileSystemForTest(), nullptr); server_->initialize(std::make_shared("127.0.0.1"), component_factory_); - EXPECT_FALSE(real_store.useExplicitTags()); + EXPECT_FALSE(real_store->useExplicitTags()); server_.reset(); + real_store.reset(); thread_local_.reset(); } From 2121dbcc86321df7176c0576e0535c17dd93957b Mon Sep 17 00:00:00 2001 From: wbpcode Date: Fri, 3 Jul 2026 03:00:37 +0000 Subject: [PATCH 6/6] ensure all input string be sanitized like the previous legacy mode Signed-off-by: wbpcode --- source/common/stats/isolated_store_impl.cc | 11 +++++++---- source/common/stats/thread_local_store.cc | 10 +++++++--- source/common/stats/utility.cc | 21 +++++++++++++++++++++ source/common/stats/utility.h | 11 +++++++++++ 4 files changed, 46 insertions(+), 7 deletions(-) diff --git a/source/common/stats/isolated_store_impl.cc b/source/common/stats/isolated_store_impl.cc index e23199d5f2c6e..da4af663aa92c 100644 --- a/source/common/stats/isolated_store_impl.cc +++ b/source/common/stats/isolated_store_impl.cc @@ -66,20 +66,23 @@ ScopeSharedPtr IsolatedScopeImpl::createScopeWithTaggedName( bool evictable, const ScopeStatsLimitSettings& limits, StatsMatcherSharedPtr matcher) { // Intern the string-based tag-extracted name, name_tags and (optional) tagged name into a // temporary pool, then delegate to the StatName-based variant. + std::string sanitize_buffer; StatNamePool tag_pool(symbolTable()); - StatName stat_name = tag_pool.add(Utility::sanitizeStatsName(base_name)); + StatName stat_name = tag_pool.add(Utility::sanitizeStatsName(base_name, sanitize_buffer)); StatName stat_tagged_name; if (!name_tags.empty()) { // The tagged name is only meaningful when there are tags to interleave; otherwise it is // ignored and the TagStatNameJoiner will use the tag-extracted name as the flat tagged name. - stat_tagged_name = - tagged_name.empty() ? StatName() : tag_pool.add(Utility::sanitizeStatsName(tagged_name)); + stat_tagged_name = tagged_name.empty() + ? StatName() + : tag_pool.add(Utility::sanitizeStatsName(tagged_name, sanitize_buffer)); } StatNameTagVec stat_name_tags; stat_name_tags.reserve(name_tags.size()); for (const auto& [tag, value] : name_tags) { - stat_name_tags.emplace_back(tag_pool.add(tag), tag_pool.add(value)); + stat_name_tags.emplace_back(tag_pool.add(Utility::sanitizeStatsName(tag, sanitize_buffer)), + tag_pool.add(Utility::sanitizeStatsName(value, sanitize_buffer))); } return scopeFromTaggedName(stat_name, stat_name_tags, stat_tagged_name, evictable, limits, std::move(matcher)); diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index b49b3fc672035..9428b0e942cae 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -172,20 +172,24 @@ ScopeSharedPtr ThreadLocalStoreImpl::ScopeImpl::createScopeWithTaggedName( absl::string_view base_name, TagStringViewSpan name_tags, absl::string_view tagged_name, bool evictable, const ScopeStatsLimitSettings& limits, StatsMatcherSharedPtr matcher) { if (useExplicitTags()) { + std::string sanitize_buffer; StatNamePool tag_pool(symbolTable()); - StatName stat_name = tag_pool.add(Utility::sanitizeStatsName(base_name)); + StatName stat_name = tag_pool.add(Utility::sanitizeStatsName(base_name, sanitize_buffer)); StatName stat_tagged_name; if (!name_tags.empty()) { // The tagged name is only meaningful when there are tags to interleave; otherwise it is // ignored. stat_tagged_name = - tagged_name.empty() ? StatName() : tag_pool.add(Utility::sanitizeStatsName(tagged_name)); + tagged_name.empty() + ? StatName() + : tag_pool.add(Utility::sanitizeStatsName(tagged_name, sanitize_buffer)); } StatNameTagVec stat_name_tags; stat_name_tags.reserve(name_tags.size()); for (const auto& [tag, value] : name_tags) { - stat_name_tags.emplace_back(tag_pool.add(tag), tag_pool.add(value)); + stat_name_tags.emplace_back(tag_pool.add(Utility::sanitizeStatsName(tag, sanitize_buffer)), + tag_pool.add(Utility::sanitizeStatsName(value, sanitize_buffer))); } return scopeFromTaggedName(stat_name, stat_name_tags, stat_tagged_name, evictable, limits, std::move(matcher)); diff --git a/source/common/stats/utility.cc b/source/common/stats/utility.cc index 585e74da9abce..5e7f2bb892323 100644 --- a/source/common/stats/utility.cc +++ b/source/common/stats/utility.cc @@ -26,6 +26,27 @@ std::string Utility::sanitizeStatsName(absl::string_view name) { }); } +absl::string_view Utility::sanitizeStatsName(absl::string_view name, std::string& buffer) { + if (absl::EndsWith(name, ".")) { + name.remove_suffix(1); + } + if (absl::StartsWith(name, ".")) { + name.remove_prefix(1); + } + + // Check if the name needs sanitization. + if (name.find(':') != absl::string_view::npos || name.find('\0') != absl::string_view::npos) { + buffer = absl::StrReplaceAll(name, { + {"://", "_"}, + {":/", "_"}, + {":", "_"}, + {absl::string_view("\0", 1), "_"}, + }); + return buffer; + } + return name; +} + std::optional Utility::findTag(const Metric& metric, StatName find_tag_name) { std::optional value; metric.iterateTagStatNames( diff --git a/source/common/stats/utility.h b/source/common/stats/utility.h index 4b1aa994b9b19..3adfc53102ca6 100644 --- a/source/common/stats/utility.h +++ b/source/common/stats/utility.h @@ -72,6 +72,17 @@ namespace Utility { */ std::string sanitizeStatsName(absl::string_view name); +/** + * Sanitizes a stat name and writes it to the provided buffer. The buffer is + * used to hold the sanitized name, and the returned string_view points to the + * buffer. The buffer is not modified if the name does not need sanitization. + * @param name the stat name to sanitize. + * @param buffer the buffer to write the sanitized name to. + * @return a string_view pointing to the sanitized name in the buffer, or the + * original name if no sanitization was needed. + */ +absl::string_view sanitizeStatsName(absl::string_view name, std::string& buffer); + /** * Finds a metric tag with the specified name. *