Stats: Remove the pure interface Stats::Allocator and rename the sole implementation from Stats::AllocatorImpl to Stats::Allocator#43968
Merged
ggreenway merged 10 commits intoenvoyproxy:mainfrom Mar 17, 2026
Conversation
…entation from Stats::AllocatorImpl to Stats::Allocator. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors Envoy’s stats implementation by removing the now-redundant Stats::Allocator pure interface and renaming the sole heap implementation from Stats::AllocatorImpl to Stats::Allocator, with corresponding include/BUILD/test updates across the codebase.
Changes:
- Replaces
AllocatorImplusage withAllocatorthroughoutsource/andtest/, including integration test custom allocator plumbing. - Renames/moves implementation files (
allocator_impl.{h,cc}→allocator.{h,cc}) and updates Bazel targets accordingly. - Removes the public header
envoy/stats/allocator.hfrom the public stats interface surface.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/server/server_stats_flush_benchmark_test.cc | Switches benchmark fixture to use Stats::Allocator name. |
| test/server/admin/stats_request_test.cc | Switches test fixture allocator type to Stats::Allocator. |
| test/server/admin/stats_render_test_base.h | Switches base test fixture allocator type to Stats::Allocator. |
| test/server/admin/stats_handler_test.cc | Updates allocator members to Stats::Allocator. |
| test/server/admin/prometheus_stats_test.cc | Updates allocator members to Stats::Allocator. |
| test/mocks/server/hot_restart.h | Updates mock member allocator type/name. |
| test/integration/server.h | Renames NotifyingAllocatorImpl → NotifyingAllocator and updates inheritance/calls. |
| test/integration/server.cc | Updates allocator instantiation to new type names. |
| test/extensions/access_loggers/stats/stats_test.cc | Updates include to source/common/stats/allocator.h. |
| test/common/stats/utility_test.cc | Updates allocator construction/type to Allocator. |
| test/common/stats/thread_local_store_test.cc | Updates allocator members/locals to Allocator. |
| test/common/stats/thread_local_store_speed_test.cc | Updates include and allocator member to Allocator. |
| test/common/stats/stat_merger_test.cc | Updates allocator member type to Allocator. |
| test/common/stats/real_thread_test_base.h | Updates allocator member type to Allocator. |
| test/common/stats/metric_impl_test.cc | Updates include and allocator member type to Allocator. |
| test/common/stats/deferred_creation_stats_test.cc | Updates allocator member type to Allocator. |
| test/common/stats/allocator_test.cc | Renames test fixture from AllocatorImplTest → AllocatorTest and updates symbols. |
| test/common/stats/BUILD | Renames test target/file from allocator_impl_test → allocator_test. |
| source/server/hot_restart_nop_impl.h | Updates include to source/common/stats/allocator.h. |
| source/server/hot_restart_impl.h | Updates include to source/common/stats/allocator.h. |
| source/exe/stripped_main_base.h | Updates member type to Stats::Allocator. |
| source/common/stats/thread_local_store.h | Updates include and comments referring to allocator file/name. |
| source/common/stats/thread_local_store.cc | Updates include and comment reference from allocator_impl.cc → allocator.cc. |
| source/common/stats/metric_impl.h | Updates comment referencing AllocatorImpl → Allocator. |
| source/common/stats/isolated_store_impl.h | Updates include and member type to Allocator. |
| source/common/stats/allocator.h | Introduces concrete Stats::Allocator implementation header (replacing AllocatorImpl). |
| source/common/stats/allocator.cc | Renamed implementation file; updates class/name references accordingly. |
| source/common/stats/BUILD | Updates allocator library to build allocator.{h,cc}. |
| envoy/stats/allocator.h | Removes the former public Stats::Allocator interface header. |
| envoy/stats/BUILD | Removes allocator.h from stats_interface public headers. |
| envoy/server/hot_restart.h | Removes now-unneeded include of envoy/stats/allocator.h. |
Comments suppressed due to low confidence (5)
source/common/stats/allocator.cc:407
- forEachSinkedCounter() unconditionally calls f_size(...) when sink_predicates_ is set. Callers (e.g. hot restart) pass nullptr for f_size, which will crash. Add a nullptr check before invoking f_size, consistent with forEachCounter().
source/common/stats/allocator.cc:419 - forEachSinkedGauge() unconditionally calls f_size(...) when sink_predicates_ is set. Store::forEachSinkedGauge() has callers that pass nullptr for f_size, which will crash. Guard the f_size call with a nullptr check.
source/common/stats/allocator.cc:435 - forEachSinkedTextReadout() unconditionally calls f_size(...) when sink_predicates_ is set. If callers pass nullptr for f_size (allowed elsewhere), this will crash. Add a nullptr check before calling f_size.
envoy/stats/BUILD:33 - Deleting envoy/stats/allocator.h (and removing it from stats_interface) is an API-breaking change for any out-of-tree Envoy extensions that included this public header. If this header is intentionally being removed from the public surface, consider leaving a deprecated shim header for at least one release cycle (or document the breaking change explicitly).
envoy_cc_library(
name = "stats_interface",
hdrs = [
"histogram.h",
"scope.h",
"stats.h",
"stats_matcher.h",
"store.h",
"tag_extractor.h",
"tag_producer.h",
],
test/common/stats/allocator_test.cc:437
- The sinked-iteration tests don’t cover the documented/used case where f_size is nullptr (e.g. hot restart callers pass nullptr). Add coverage that calls forEachSinkedCounter/Gauge/TextReadout with f_size == nullptr when sink predicates are enabled, to ensure it doesn’t crash and still iterates correctly.
Contributor
Author
|
/retest |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors Envoy’s stats allocator by removing the now-redundant pure Stats::Allocator interface and renaming the sole remaining implementation from Stats::AllocatorImpl to Stats::Allocator. This simplifies the stats subsystem internals and aligns with follow-up work to prevent direct allocator usage outside the stats system.
Changes:
- Replace
Stats::AllocatorImplusage withStats::Allocatoracross core code and tests. - Rename/move implementation files (
allocator_impl.{h,cc}->allocator.{h,cc}) and update Bazel targets accordingly. - Convert
envoy/stats/allocator.hinto a deprecation placeholder header.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/server/server_stats_flush_benchmark_test.cc | Switch benchmark fixture to Stats::Allocator. |
| test/server/admin/stats_request_test.cc | Update test fixture allocator type rename. |
| test/server/admin/stats_render_test_base.h | Update allocator member type rename. |
| test/server/admin/stats_handler_test.cc | Update allocator members to renamed type. |
| test/server/admin/prometheus_stats_test.cc | Update allocator members to renamed type. |
| test/mocks/server/hot_restart.h | Update mock hot-restart allocator member type. |
| test/integration/server.h | Rename NotifyingAllocatorImpl -> NotifyingAllocator and update base class/type usage. |
| test/integration/server.cc | Instantiate renamed allocator types. |
| test/extensions/access_loggers/stats/stats_test.cc | Update include to new allocator header. |
| test/common/stats/utility_test.cc | Update allocator unique_ptr type and construction. |
| test/common/stats/thread_local_store_test.cc | Update allocator member types and local constructions. |
| test/common/stats/thread_local_store_speed_test.cc | Update include and allocator member type. |
| test/common/stats/stat_merger_test.cc | Update allocator member initialization to renamed type. |
| test/common/stats/real_thread_test_base.h | Update allocator member type. |
| test/common/stats/metric_impl_test.cc | Update include and allocator member type. |
| test/common/stats/deferred_creation_stats_test.cc | Update allocator member type. |
| test/common/stats/allocator_test.cc | Rename test fixture and update syncpoint type reference. |
| test/common/stats/BUILD | Rename test target and source file to match new allocator name. |
| source/server/hot_restart_nop_impl.h | Update include to new allocator header. |
| source/server/hot_restart_impl.h | Update include to new allocator header. |
| source/exe/stripped_main_base.h | Update allocator member type rename. |
| source/common/stats/thread_local_store.h | Update include and comments referencing allocator file. |
| source/common/stats/thread_local_store.cc | Update include and comments referencing allocator file. |
| source/common/stats/metric_impl.h | Remove unused public allocator include; update comment wording. |
| source/common/stats/isolated_store_impl.h | Update include and allocator member type rename. |
| source/common/stats/allocator.h | Define the concrete allocator class (previously AllocatorImpl). |
| source/common/stats/allocator.cc | Move implementation to new filename and rename symbols. |
| source/common/stats/BUILD | Update allocator library to use allocator.{h,cc}. |
| envoy/stats/allocator.h | Convert to placeholder/deprecation header. |
| envoy/server/hot_restart.h | Remove include of allocator public header (no longer needed). |
Comments suppressed due to low confidence (3)
source/common/stats/allocator.cc:407
forEachSinkedCountercallsf_size(...)unconditionally whensink_predicates_is set. Callers do passnullptrforf_size(e.g. hot-restart exportStatsToChild), which would crash. Add a null check (as inforEachCounter) or require a non-null size functor and update all call sites accordingly.
source/common/stats/allocator.cc:419- Same issue as
forEachSinkedCounter:forEachSinkedGaugeunconditionally callsf_size(...)whensink_predicates_is set, butSizeFnis used as nullable elsewhere. Guardf_sizebefore invoking it.
source/common/stats/allocator.cc:435 - Same issue as
forEachSinkedCounter:forEachSinkedTextReadoutunconditionally callsf_size(...)whensink_predicates_is set. Guardf_sizebefore invoking it to match other forEach APIs and existing call sites that pass nullptr.
Comment on lines
+3
to
+12
| // This header is a placeholder which we will carry until June 1, 2026, | ||
| // as we have deprecated the pure interface and impl pattern. | ||
| // | ||
| // Please remoe references to this file and instead include | ||
| // source/common/stats/allocator.h. | ||
|
|
||
| namespace Envoy { | ||
| namespace Stats { | ||
|
|
||
| class Sink; | ||
| class SinkPredicates; | ||
|
|
||
| /** | ||
| * Abstract interface for allocating statistics. Implementations can | ||
| * be created utilizing a single fixed-size block suitable for | ||
| * shared-memory, or in the heap, allowing for pointers and sharing of | ||
| * substrings, with an opportunity for reduced memory consumption. | ||
| */ | ||
| class Allocator { | ||
| public: | ||
| virtual ~Allocator() = default; | ||
|
|
||
| /** | ||
| * @param name the full name of the stat. | ||
| * @param tag_extracted_name the name of the stat with tag-values stripped out. | ||
| * @param tags the tag values. | ||
| * @return CounterSharedPtr a counter. | ||
| */ | ||
| virtual CounterSharedPtr makeCounter(StatName name, StatName tag_extracted_name, | ||
| const StatNameTagVector& stat_name_tags) PURE; | ||
|
|
||
| /** | ||
| * @param name the full name of the stat. | ||
| * @param tag_extracted_name the name of the stat with tag-values stripped out. | ||
| * @param stat_name_tags the tag values. | ||
| * @return GaugeSharedPtr a gauge. | ||
| */ | ||
| virtual GaugeSharedPtr makeGauge(StatName name, StatName tag_extracted_name, | ||
| const StatNameTagVector& stat_name_tags, | ||
| Gauge::ImportMode import_mode) PURE; | ||
|
|
||
| /** | ||
| * @param name the full name of the stat. | ||
| * @param tag_extracted_name the name of the stat with tag-values stripped out. | ||
| * @param tags the tag values. | ||
| * @return TextReadoutSharedPtr a text readout. | ||
| */ | ||
| virtual TextReadoutSharedPtr makeTextReadout(StatName name, StatName tag_extracted_name, | ||
| const StatNameTagVector& stat_name_tags) PURE; | ||
| virtual const SymbolTable& constSymbolTable() const PURE; | ||
| virtual SymbolTable& symbolTable() PURE; | ||
|
|
||
| /** | ||
| * Mark rejected stats as deleted by moving them to a different vector, so they don't show up | ||
| * when iterating over stats, but prevent crashes when trying to access references to them. | ||
| * Note that allocating a stat with the same name after calling this will | ||
| * return a new stat. Hence callers should seek to avoid this situation, as is | ||
| * done in ThreadLocalStore. | ||
| */ | ||
| virtual void markCounterForDeletion(const CounterSharedPtr& counter) PURE; | ||
| virtual void markGaugeForDeletion(const GaugeSharedPtr& gauge) PURE; | ||
| virtual void markTextReadoutForDeletion(const TextReadoutSharedPtr& text_readout) PURE; | ||
|
|
||
| /** | ||
| * Iterate over all stats. Note, that implementations can potentially hold on to a mutex that | ||
| * will deadlock if the passed in functors try to create or delete a stat. | ||
| * @param f_size functor that is provided the current number of all stats. Note that this is | ||
| * called only once, prior to any calls to f_stat. | ||
| * @param f_stat functor that is provided one stat at a time from the stats container. | ||
| */ | ||
| virtual void forEachCounter(SizeFn f_size, StatFn<Counter> f_stat) const PURE; | ||
| virtual void forEachGauge(SizeFn f_size, StatFn<Gauge> f_stat) const PURE; | ||
| virtual void forEachTextReadout(SizeFn f_size, StatFn<TextReadout> f_stat) const PURE; | ||
|
|
||
| /** | ||
| * Iterate over all stats that need to be flushed to sinks. Note, that implementations can | ||
| * potentially hold on to a mutex that will deadlock if the passed in functors try to create | ||
| * or delete a stat. | ||
| * @param f_size functor that is provided the number of all stats that will be flushed to sinks. | ||
| * Note that this is called only once, prior to any calls to f_stat. | ||
| * @param f_stat functor that is provided one stat that will be flushed to sinks, at a time. | ||
| */ | ||
| virtual void forEachSinkedCounter(SizeFn f_size, StatFn<Counter> f_stat) const PURE; | ||
| virtual void forEachSinkedGauge(SizeFn f_size, StatFn<Gauge> f_stat) const PURE; | ||
| virtual void forEachSinkedTextReadout(SizeFn f_size, StatFn<TextReadout> f_stat) const PURE; | ||
|
|
||
| /** | ||
| * Set the predicates to filter stats for sink. | ||
| */ | ||
| virtual void setSinkPredicates(std::unique_ptr<SinkPredicates>&& sink_predicates) PURE; | ||
|
|
||
| // TODO(jmarantz): create a parallel mechanism to instantiate histograms. At | ||
| // the moment, histograms don't fit the same pattern of counters and gauges | ||
| // as they are not actually created in the context of a stats allocator. | ||
| }; | ||
| class Allocator; |
Contributor
Author
There was a problem hiding this comment.
This concern makes sense, but I think (a) the risk is small and (b) there is no good remedy.
The risk is small because Stats::Allocator was not a useful API; it was needed for any stats operations, except instantiating Stores. And to instantiate a Store you needed to allocate an Allocator you needed to include allocator_impl.h and reference an AllocatorImpl.
There is no good remedy because we do not want to have an interface in envoy/stats depend on an implementation in source/common/stats. The dependencies should only go the other way.
envoy/stats/allocator.h
Outdated
| // This header is a placeholder which we will carry until June 1, 2026, | ||
| // as we have deprecated the pure interface and impl pattern. | ||
| // | ||
| // Please remoe references to this file and instead include |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors Envoy’s stats allocation layer by removing the now-redundant Stats::Allocator pure interface and renaming the single remaining implementation from Stats::AllocatorImpl to Stats::Allocator, simplifying the stats internals and supporting upcoming snapshot/lifetime changes.
Changes:
- Introduces
source/common/stats/allocator.h|ccas the concrete allocator implementation and updates the stats library build target accordingly. - Replaces in-tree usages of
Stats::AllocatorImplwithStats::Allocatoracross server code and tests (including a test-only notifying allocator). - Renames the allocator unit test target/file and updates includes/comments to match the new filenames.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/server/server_stats_flush_benchmark_test.cc | Switches benchmark fixture member from AllocatorImpl to Allocator. |
| test/server/admin/stats_request_test.cc | Updates test fixture allocator type to Allocator. |
| test/server/admin/stats_render_test_base.h | Updates base test fixture allocator type to Allocator. |
| test/server/admin/stats_handler_test.cc | Updates allocator members in admin handler tests to Allocator. |
| test/server/admin/prometheus_stats_test.cc | Updates prometheus formatter tests to use Allocator. |
| test/mocks/server/hot_restart.h | Updates mock hot restart allocator member to Allocator. |
| test/integration/server.h | Renames NotifyingAllocatorImpl → NotifyingAllocator, updates base class and internal calls. |
| test/integration/server.cc | Updates allocator construction to Allocator / NotifyingAllocator. |
| test/extensions/access_loggers/stats/stats_test.cc | Updates include from allocator_impl.h to allocator.h. |
| test/common/stats/utility_test.cc | Updates test store setup to use Allocator. |
| test/common/stats/thread_local_store_test.cc | Updates multiple fixtures/locals from AllocatorImpl to Allocator. |
| test/common/stats/thread_local_store_speed_test.cc | Updates perf test allocator type and include. |
| test/common/stats/stat_merger_test.cc | Updates thread-local merger test allocator type. |
| test/common/stats/real_thread_test_base.h | Updates mixin allocator member to Allocator. |
| test/common/stats/metric_impl_test.cc | Updates include and fixture allocator type to Allocator. |
| test/common/stats/deferred_creation_stats_test.cc | Updates deferred creation test allocator type to Allocator. |
| test/common/stats/allocator_test.cc | Renames test fixture and updates sync-point references to Allocator. |
| test/common/stats/BUILD | Renames test target allocator_impl_test → allocator_test and updates source filename. |
| source/server/hot_restart_nop_impl.h | Updates include to new concrete allocator header. |
| source/server/hot_restart_impl.h | Updates include to new concrete allocator header. |
| source/exe/stripped_main_base.h | Updates member type from AllocatorImpl to Allocator. |
| source/common/stats/thread_local_store.h | Updates include and comments to refer to allocator.h/cc and Allocator. |
| source/common/stats/thread_local_store.cc | Switches from public interface include to internal concrete allocator include; updates comment reference. |
| source/common/stats/metric_impl.h | Removes dependency on the old public allocator interface header; updates comment wording. |
| source/common/stats/isolated_store_impl.h | Updates include and member from AllocatorImpl to Allocator. |
| source/common/stats/allocator_impl.h | Replaces prior implementation header with a deprecation shim typedefing AllocatorImpl to Allocator. |
| source/common/stats/allocator.h | Adds the concrete allocator class definition (moved from the old impl header). |
| source/common/stats/allocator.cc | Renames implementation file and updates symbols from AllocatorImpl::* to Allocator::*. |
| source/common/stats/BUILD | Updates allocator_lib to build allocator.cc and export allocator.h plus transitional allocator_impl.h. |
| envoy/stats/allocator.h | Converts public header to a deprecation placeholder with a forward declaration. |
| envoy/server/hot_restart.h | Removes now-unnecessary include of envoy/stats/allocator.h. |
source/common/stats/allocator_impl.h
Outdated
|
|
||
| #include "absl/container/flat_hash_set.h" | ||
| #include "absl/strings/string_view.h" | ||
| #include "source3/common/stats/allocator.h" |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors Envoy’s stats allocation implementation by removing the now-redundant Stats::Allocator pure interface and renaming the only remaining implementation (Stats::AllocatorImpl) to Stats::Allocator, while leaving temporary forwarding headers to ease migration.
Changes:
- Introduce
source/common/stats/allocator.{h,cc}as the concrete allocator implementation and migrate call sites fromAllocatorImpltoAllocator. - Convert
source/common/stats/allocator_impl.hinto a transitional alias header (using AllocatorImpl = Allocator) and reduceenvoy/stats/allocator.hto a placeholder forward declaration. - Update tests, integration harnesses, and Bazel targets/includes to match the new naming.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/server/server_stats_flush_benchmark_test.cc | Switch benchmark fixture member from AllocatorImpl to Allocator. |
| test/server/admin/stats_request_test.cc | Update test fixture allocator member type to Allocator. |
| test/server/admin/stats_render_test_base.h | Update allocator member type to Allocator. |
| test/server/admin/stats_handler_test.cc | Update allocator member types to Allocator in multiple fixtures. |
| test/server/admin/prometheus_stats_test.cc | Update allocator member types to Allocator in formatter/histogram tests. |
| test/mocks/server/hot_restart.h | Update mock’s allocator member type to Allocator. |
| test/integration/server.h | Rename NotifyingAllocatorImpl to NotifyingAllocator and base it on Stats::Allocator. |
| test/integration/server.cc | Update allocator construction to instantiate Allocator / NotifyingAllocator. |
| test/extensions/access_loggers/stats/stats_test.cc | Update include to the new allocator header. |
| test/common/stats/utility_test.cc | Update std::unique_ptr types and construction to Allocator. |
| test/common/stats/thread_local_store_test.cc | Update allocator members/locals to Allocator. |
| test/common/stats/thread_local_store_speed_test.cc | Update include and allocator member type to Allocator. |
| test/common/stats/stat_merger_test.cc | Update fixture allocator member type to Allocator. |
| test/common/stats/real_thread_test_base.h | Update allocator member type to Allocator. |
| test/common/stats/metric_impl_test.cc | Update include and allocator member type to Allocator. |
| test/common/stats/deferred_creation_stats_test.cc | Update allocator member type to Allocator. |
| test/common/stats/allocator_test.cc | Rename test fixture and update sync-point references to Allocator. |
| test/common/stats/BUILD | Rename Bazel test target/source from allocator_impl_test to allocator_test. |
| source/server/hot_restart_nop_impl.h | Update include to source/common/stats/allocator.h. |
| source/server/hot_restart_impl.h | Update include to source/common/stats/allocator.h. |
| source/exe/stripped_main_base.h | Update allocator member type to Allocator. |
| source/common/stats/thread_local_store.h | Update include and update comments referencing allocator file/class names. |
| source/common/stats/thread_local_store.cc | Update includes and references from allocator_impl to allocator. |
| source/common/stats/metric_impl.h | Remove dependency on public allocator header; update comment to Allocator. |
| source/common/stats/isolated_store_impl.h | Update include and member type to Allocator. |
| source/common/stats/allocator_impl.h | Convert to deprecation shim aliasing AllocatorImpl to Allocator. |
| source/common/stats/allocator.h | Add concrete allocator class definition (formerly the impl). |
| source/common/stats/allocator.cc | Rename implementation file and update class/method names accordingly. |
| source/common/stats/BUILD | Update allocator library to build allocator.cc and export new headers. |
| envoy/stats/allocator.h | Convert public header to placeholder forward-declaration for deprecation window. |
| envoy/server/hot_restart.h | Remove dependency on envoy/stats/allocator.h. |
Comments suppressed due to low confidence (4)
source/common/stats/allocator.cc:435
forEachSinkedTextReadoutcallsf_size(...)without anullptrcheck whensink_predicates_is set. This is inconsistent with the other iteration helpers and can crash if the caller passesnullptrfor the size functor; please add a guard in the sinked branch as well.
test/common/stats/allocator_test.cc:319- The new sinked-iteration paths in
Allocatorcan crash whenf_sizeisnullptrand sink predicates are enabled. Consider extending this test to coverforEachSinkedCounter/forEachSinkedGauge(and text readouts if applicable) withf_size == nullptr, including aftersetSinkPredicates()so the sinked branch is exercised.
source/common/stats/allocator.cc:407 forEachSinkedCountercallsf_size(...)without checking fornullptrwhensink_predicates_is set. There are call sites that passnullptrforf_size(e.g. hot restart stat export), so this will crash when sink predicates are enabled. Please guard thef_sizecall (or otherwise handle a null size callback) in the sinked branch, consistent withforEachCounter.
source/common/stats/allocator.cc:419forEachSinkedGaugeunconditionally invokesf_size(...)in thesink_predicates_ != nullptrbranch. Since callers may passnullptrfor the size callback, add a null check here (matching the non-sinked iterators) to avoid a null-function call when sink predicates are enabled.
source/common/stats/allocator.h
Outdated
|
|
||
| #include <vector> | ||
|
|
||
| #include "envoy/common/optref.h" |
source/common/stats/allocator.h
Outdated
| friend class CounterImpl; | ||
| friend class GaugeImpl; | ||
| friend class TextReadoutImpl; | ||
| friend class NotifyingAllocator; |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors Envoy’s stats allocator by removing the now-redundant Stats::Allocator pure interface and renaming the sole in-tree implementation from Stats::AllocatorImpl to Stats::Allocator, while keeping temporary shim headers to ease migration.
Changes:
- Introduces
source/common/stats/allocator.h+allocator.ccas the concrete allocator, and turnssource/common/stats/allocator_impl.hinto a compatibility shim. - Updates core code and tests to include/use
Stats::Allocatorinstead ofStats::AllocatorImpl. - Renames the allocator unit test Bazel target and file from
allocator_impl_testtoallocator_test.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/server/server_stats_flush_benchmark_test.cc | Updates benchmark fixture to use Stats::Allocator. |
| test/server/admin/stats_request_test.cc | Switches test allocator member to Stats::Allocator. |
| test/server/admin/stats_render_test_base.h | Switches test base allocator member to Stats::Allocator. |
| test/server/admin/stats_handler_test.cc | Replaces AllocatorImpl members with Allocator. |
| test/server/admin/prometheus_stats_test.cc | Replaces AllocatorImpl members with Allocator. |
| test/mocks/server/hot_restart.h | Updates mock to hold Stats::Allocator directly. |
| test/integration/server.h | Renames NotifyingAllocatorImpl to NotifyingAllocator and updates inheritance/casts/includes. |
| test/integration/server.cc | Constructs Stats::Allocator / Stats::NotifyingAllocator instead of old types. |
| test/extensions/access_loggers/stats/stats_test.cc | Updates include to source/common/stats/allocator.h. |
| test/common/stats/utility_test.cc | Updates allocator storage type to Allocator. |
| test/common/stats/thread_local_store_test.cc | Updates test fixtures to use Allocator. |
| test/common/stats/thread_local_store_speed_test.cc | Updates perf test to include/use Allocator. |
| test/common/stats/stat_merger_test.cc | Replaces AllocatorImpl fixture member with Allocator. |
| test/common/stats/real_thread_test_base.h | Replaces AllocatorImpl member with Allocator. |
| test/common/stats/metric_impl_test.cc | Updates include and fixture member to Allocator. |
| test/common/stats/deferred_creation_stats_test.cc | Replaces AllocatorImpl member with Allocator. |
| test/common/stats/allocator_test.cc | Renames test fixture from AllocatorImplTest to AllocatorTest and updates sync-point references. |
| test/common/stats/BUILD | Renames Bazel test target/src from allocator_impl_test to allocator_test. |
| source/server/hot_restart_nop_impl.h | Updates include to new concrete allocator header. |
| source/server/hot_restart_impl.h | Updates include to new concrete allocator header. |
| source/exe/stripped_main_base.h | Replaces AllocatorImpl member with Allocator. |
| source/common/stats/thread_local_store.h | Switches include to source/common/stats/allocator.h and updates comments. |
| source/common/stats/thread_local_store.cc | Switches include from envoy/stats/allocator.h to source/common/stats/allocator.h and updates comments. |
| source/common/stats/metric_impl.h | Updates comment to refer to Allocator (new name). |
| source/common/stats/isolated_store_impl.h | Switches include and member type from AllocatorImpl to Allocator. |
| source/common/stats/allocator_impl.h | Becomes a shim header that aliases AllocatorImpl to Allocator. |
| source/common/stats/allocator.h | Adds the new concrete allocator class declaration (moved from old impl header). |
| source/common/stats/allocator.cc | Implements the concrete allocator (renamed from old impl cc). |
| source/common/stats/BUILD | Updates allocator library to build allocator.cc and export new headers. |
| envoy/stats/allocator.h | Becomes a placeholder header with only a forward declaration. |
| envoy/server/hot_restart.h | Removes include of the old public allocator interface header. |
Comment on lines
+120
to
+124
| // free() operations are made from the destructors of the individual stat objects, which are not | ||
| // protected by locks. | ||
| mutable Thread::MutexBasicLockable mutex_; | ||
|
|
||
| StatSet<Counter> counters_ ABSL_GUARDED_BY(mutex_); |
source/common/stats/allocator.h
Outdated
| #include "source/common/stats/metric_impl.h" | ||
|
|
||
| #include "absl/container/flat_hash_set.h" | ||
| #include "absl/strings/string_view.h" |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Contributor
Author
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
The pull request refactors the AllocatorImpl class by renaming it to Allocator and consolidating its definition into source/common/stats/allocator.h and source/common/stats/allocator.cc. This involves updating numerous references throughout the codebase, including header includes, class instantiations, and test file names. Review comments suggest adding an explanation in source/common/stats/allocator.cc regarding the assertion of duplicate stats in the destructor, removing a redundant using statement in source/common/stats/allocator_impl.h, and updating an outdated file reference in a comment within source/common/stats/thread_local_store.cc.
Comment on lines
108
to
+109
| if (--ref_count_ == 0) { | ||
| alloc_.sync().syncPoint(AllocatorImpl::DecrementToZeroSyncPoint); | ||
| alloc_.sync().syncPoint(Allocator::DecrementToZeroSyncPoint); |
Contributor
There was a problem hiding this comment.
The Allocator::DecrementToZeroSyncPoint constant is now being used instead of AllocatorImpl::DecrementToZeroSyncPoint. This change is correct, but it's important to verify that this constant is indeed the correct one to use in this context.
Contributor
Author
There was a problem hiding this comment.
i don't think this comemnt adds anything. The code change highlighted is correct.
source/common/stats/allocator.cc
Outdated
Comment on lines
+478
to
+479
| ASSERT(counter.get() == *iter); | ||
| // Duplicates are ASSERTed in ~AllocatorImpl. | ||
| // Duplicates are ASSERTed in ~Allocator. |
Contributor
There was a problem hiding this comment.
The comment mentions that duplicates are asserted in ~Allocator. It would be helpful to add a brief explanation of why duplicates might occur and what implications they have for the system's behavior.
| std::vector<GaugeSharedPtr> deleted_gauges_ ABSL_GUARDED_BY(mutex_); | ||
| std::vector<TextReadoutSharedPtr> deleted_text_readouts_ ABSL_GUARDED_BY(mutex_); | ||
| }; | ||
| using AllocatorImpl = Allocator; |
Contributor
There was a problem hiding this comment.
This using statement is now redundant and can be removed.
Contributor
Author
There was a problem hiding this comment.
Added comment:
// This alias is provided for out-of-repository includes of allocator_impl.h, who will
// be expecting the concrete class to be called AllocatorImpl.
Comment on lines
+917
to
918
| // See also StatsSharedImpl::decRefCount() in allocator.cc, which has | ||
| // the same issue. |
Contributor
There was a problem hiding this comment.
Since allocator_impl.cc has been renamed to allocator.cc, this comment should be updated to reflect the change.
Suggested change
| // See also StatsSharedImpl::decRefCount() in allocator.cc, which has | |
| // the same issue. | |
| // | |
| // See also StatsSharedImpl::decRefCount() in allocator.cc, which has | |
| // the same issue. |
Contributor
Author
There was a problem hiding this comment.
the suggested change is not a change.
Contributor
Author
|
/retest |
Contributor
|
/assign @kyessenov |
ggreenway
approved these changes
Mar 17, 2026
Member
ggreenway
left a comment
ggreenway
left a comment
There was a problem hiding this comment.
This looks good, but I don't see the need to leave forwarding-headers. We never claim to avoid compile-time breakage with changes. WDYT?
|
|
||
| #include "absl/strings/string_view.h" | ||
| // This header is a placeholder which we will carry until June 1, 2026, | ||
| // as we have deprecated the pure interface and impl pattern. |
Member
There was a problem hiding this comment.
Why is this needed? If it's not a compile-time-breakage now, people probably won't even see this comment. I'd say just rip it out, maybe leave this comment in the commit message so that git blame will help find the solution, and be done with it.
Contributor
Author
There was a problem hiding this comment.
I will remove shortly; we have references to allocator_impl.h and AllocatorImpl in our internal repo that I need to remove first, otherwise that becomes tax on whoever does the import.
Member
|
/wait-any |
Contributor
Author
|
To clarify, what I meant was that I'd like to merge this as I have it, give some time for people with private repos to import an Allocator with a constructor and switch to that (I suppose an announcement to slack would be helpful). Per the comment I thought we could leave the forwarding header till June 1. |
Contributor
Author
|
Thanks Greg! |
fishcakez
pushed a commit
to fishcakez/envoy
that referenced
this pull request
Mar 25, 2026
… implementation from Stats::AllocatorImpl to Stats::Allocator (envoyproxy#43968) A long time ago, all Envoy stats were stored in a large shared memory block, significantly limiting how many you could get, and the lengths of their names. This facilitated keeping stat continuity across parent/child hot-restart. The fixed shared-mem block size was very limiting and a bit inefficient, so an alternative mechanism of keeping stats on the heap was added for when hot-restart was not needed. To switch between them, we used the Allocator interface, and kept two implementations of that. Then we removed the shared-memory version in favor of using message passing to update counters from parent to child during hot restart. Now it's time to remove that interface; it's making it a bit harder to reason about guaranteeing that stats users don't call the allocator directly; it's really an implementation detail of the stats system. So this is a pure refactor (no functional change) which can help that, and make it easier to reason about the safety of envoyproxy#43958 . As removing headers and class names can be a breaking change, temporary forwarding headers are left behind, temporarily; out-of-repo referencess should be changed by June. Signed-off-by: Joshua Marantz <jmarantz@google.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit Message: Removes no-longer-needed Stats::Allocator interface, and renames the sole implementation, Stats::AllocatorImpl to Stats::Allocator.
A long time ago, all Envoy stats were stored in a large shared memory block, significantly limiting how many you could get, and the lengths of their names. This facilitated keeping stat continuity across parent/child hot-restart. The fixed shared-mem block size was very limiting and a bit inefficient, so an alternative mechanism of keeping stats on the heap was added for when hot-restart was not needed. To switch between them, we used the Allocator interface, and kept two implementations of that.
Then we removed the shared-memory version in favor of using message passing to update counters from parent to child during hot restart.
Now it's time to remove that interface; it's making it a bit harder to reason about guaranteeing that stats users don't call the allocator directly; it's really an implementation detail of the stats system. So this is a pure refactor (no functional change) which can help that, and make it easier to reason about the safety of #43958 .
As removing headers and class names can be a breaking change, temporary forwarding headers are left behind, temporarily; out-of-repo referencess should be changed by June.
Additional Description:
Risk Level: low -- pure. refactor
Testing: CI
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: