Skip to content

Stats: Remove the pure interface Stats::Allocator and rename the sole implementation from Stats::AllocatorImpl to Stats::Allocator #43968

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ggreenway merged 10 commits into from
Mar 17, 2026
Merged

Stats: Remove the pure interface Stats::Allocator and rename the sole implementation from Stats::AllocatorImpl to Stats::Allocator #43968

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d313f2f
Remove the pure interface Stats::Allocator and rename the sole implem…
jmarantz Mar 15, 2026
5b80d69
allocator_impl.h --> allocator.h
jmarantz Mar 15, 2026
7a70401
remove superfluous forward decl.
jmarantz Mar 15, 2026
20e15b2
restore allocator.h interface
jmarantz Mar 15, 2026
843235a
Add placeholder allocator_impl.h file.
jmarantz Mar 15, 2026
d20b02c
add AlloctorImpl placeholder nickname
jmarantz Mar 15, 2026
c9ca057
typo
jmarantz Mar 15, 2026
d08b9ed
remove superfluous include and forward declaration.
jmarantz Mar 16, 2026
9a7de83
includes hygiene
jmarantz Mar 16, 2026
d50f726
comments
jmarantz Mar 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion envoy/server/hot_restart.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

#include "envoy/common/pure.h"
#include "envoy/event/dispatcher.h"
#include "envoy/stats/allocator.h"
#include "envoy/stats/store.h"
#include "envoy/thread/thread.h"

Expand Down
105 changes: 6 additions & 99 deletions envoy/stats/allocator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,108 +1,15 @@
#pragma once

#include <chrono>
#include <cstdint>
#include <functional>
#include <list>
#include <memory>
#include <string>
#include <vector>

#include "envoy/common/pure.h"
#include "envoy/stats/stats.h"
#include "envoy/stats/tag.h"

#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.

@ggreenway ggreenway Mar 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jmarantz jmarantz Mar 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

//
// Please remove 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;
Comment on lines +3 to +12

@jmarantz jmarantz Mar 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


} // namespace Stats
} // namespace Envoy
7 changes: 5 additions & 2 deletions source/common/stats/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,11 @@ envoy_package()

envoy_cc_library(
name = "allocator_lib",
srcs = ["allocator_impl.cc"],
hdrs = ["allocator_impl.h"],
srcs = ["allocator.cc"],
hdrs = [
"allocator.h",
"allocator_impl.h",
],
deps = [
":metric_impl_lib",
":stat_merger_lib",
Expand Down
66 changes: 33 additions & 33 deletions source/common/stats/allocator_impl.cc → source/common/stats/allocator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#include "source/common/stats/allocator_impl.h"
#include "source/common/stats/allocator.h"

#include <algorithm>
#include <cstdint>
Expand All @@ -21,9 +21,9 @@
namespace Envoy {
namespace Stats {

const char AllocatorImpl::DecrementToZeroSyncPoint[] = "decrement-zero";
const char Allocator::DecrementToZeroSyncPoint[] = "decrement-zero";

AllocatorImpl::~AllocatorImpl() {
Allocator::~Allocator() {
ASSERT(counters_.empty());
ASSERT(gauges_.empty());

Expand All @@ -48,7 +48,7 @@ AllocatorImpl::~AllocatorImpl() {
}

#ifndef ENVOY_CONFIG_COVERAGE
void AllocatorImpl::debugPrint() {
void Allocator::debugPrint() {
Thread::LockGuard lock(mutex_);
for (Counter* counter : counters_) {
ENVOY_LOG_MISC(info, "counter: {}", symbolTable().toString(counter->statName()));
Expand All @@ -69,7 +69,7 @@ void AllocatorImpl::debugPrint() {
// shared_ptr.
template <class BaseClass> class StatsSharedImpl : public MetricImpl<BaseClass> {
public:
StatsSharedImpl(StatName name, AllocatorImpl& alloc, StatName tag_extracted_name,
StatsSharedImpl(StatName name, Allocator& alloc, StatName tag_extracted_name,
const StatNameTagVector& stat_name_tags)
: MetricImpl<BaseClass>(name, tag_extracted_name, stat_name_tags, alloc.symbolTable()),
alloc_(alloc) {}
Expand Down Expand Up @@ -105,7 +105,7 @@ template <class BaseClass> class StatsSharedImpl : public MetricImpl<BaseClass>
Thread::LockGuard lock(alloc_.mutex_);
ASSERT(ref_count_ >= 1);
if (--ref_count_ == 0) {
alloc_.sync().syncPoint(AllocatorImpl::DecrementToZeroSyncPoint);
alloc_.sync().syncPoint(Allocator::DecrementToZeroSyncPoint);
Comment on lines 108 to +109

@gemini-code-assist gemini-code-assist Bot Mar 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

@jmarantz jmarantz Mar 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think this comemnt adds anything. The code change highlighted is correct.

removeFromSetLockHeld();
return true;
}
Expand All @@ -121,7 +121,7 @@ template <class BaseClass> class StatsSharedImpl : public MetricImpl<BaseClass>
virtual void removeFromSetLockHeld() ABSL_EXCLUSIVE_LOCKS_REQUIRED(alloc_.mutex_) PURE;

protected:
AllocatorImpl& alloc_;
Allocator& alloc_;

// ref_count_ can be incremented as an atomic, without taking a new lock, as
// the critical 0->1 transition occurs in makeCounter and makeGauge, which
Expand All @@ -139,7 +139,7 @@ template <class BaseClass> class StatsSharedImpl : public MetricImpl<BaseClass>

class CounterImpl : public StatsSharedImpl<Counter> {
public:
CounterImpl(StatName name, AllocatorImpl& alloc, StatName tag_extracted_name,
CounterImpl(StatName name, Allocator& alloc, StatName tag_extracted_name,
const StatNameTagVector& stat_name_tags)
: StatsSharedImpl(name, alloc, tag_extracted_name, stat_name_tags) {}

Expand Down Expand Up @@ -169,7 +169,7 @@ class CounterImpl : public StatsSharedImpl<Counter> {

class GaugeImpl : public StatsSharedImpl<Gauge> {
public:
GaugeImpl(StatName name, AllocatorImpl& alloc, StatName tag_extracted_name,
GaugeImpl(StatName name, Allocator& alloc, StatName tag_extracted_name,
const StatNameTagVector& stat_name_tags, ImportMode import_mode)
: StatsSharedImpl(name, alloc, tag_extracted_name, stat_name_tags) {
switch (import_mode) {
Expand Down Expand Up @@ -270,7 +270,7 @@ class GaugeImpl : public StatsSharedImpl<Gauge> {

class TextReadoutImpl : public StatsSharedImpl<TextReadout> {
public:
TextReadoutImpl(StatName name, AllocatorImpl& alloc, StatName tag_extracted_name,
TextReadoutImpl(StatName name, Allocator& alloc, StatName tag_extracted_name,
const StatNameTagVector& stat_name_tags)
: StatsSharedImpl(name, alloc, tag_extracted_name, stat_name_tags) {}

Expand All @@ -297,8 +297,8 @@ class TextReadoutImpl : public StatsSharedImpl<TextReadout> {
std::string value_ ABSL_GUARDED_BY(mutex_);
};

CounterSharedPtr AllocatorImpl::makeCounter(StatName name, StatName tag_extracted_name,
const StatNameTagVector& stat_name_tags) {
CounterSharedPtr Allocator::makeCounter(StatName name, StatName tag_extracted_name,
const StatNameTagVector& stat_name_tags) {
Thread::LockGuard lock(mutex_);
ASSERT(gauges_.find(name) == gauges_.end());
ASSERT(text_readouts_.find(name) == text_readouts_.end());
Expand All @@ -316,9 +316,9 @@ CounterSharedPtr AllocatorImpl::makeCounter(StatName name, StatName tag_extracte
return counter;
}

GaugeSharedPtr AllocatorImpl::makeGauge(StatName name, StatName tag_extracted_name,
const StatNameTagVector& stat_name_tags,
Gauge::ImportMode import_mode) {
GaugeSharedPtr Allocator::makeGauge(StatName name, StatName tag_extracted_name,
const StatNameTagVector& stat_name_tags,
Gauge::ImportMode import_mode) {
Thread::LockGuard lock(mutex_);
ASSERT(counters_.find(name) == counters_.end());
ASSERT(text_readouts_.find(name) == text_readouts_.end());
Expand All @@ -337,8 +337,8 @@ GaugeSharedPtr AllocatorImpl::makeGauge(StatName name, StatName tag_extracted_na
return gauge;
}

TextReadoutSharedPtr AllocatorImpl::makeTextReadout(StatName name, StatName tag_extracted_name,
const StatNameTagVector& stat_name_tags) {
TextReadoutSharedPtr Allocator::makeTextReadout(StatName name, StatName tag_extracted_name,
const StatNameTagVector& stat_name_tags) {
Thread::LockGuard lock(mutex_);
ASSERT(counters_.find(name) == counters_.end());
ASSERT(gauges_.find(name) == gauges_.end());
Expand All @@ -357,20 +357,20 @@ TextReadoutSharedPtr AllocatorImpl::makeTextReadout(StatName name, StatName tag_
return text_readout;
}

bool AllocatorImpl::isMutexLockedForTest() {
bool Allocator::isMutexLockedForTest() {
bool locked = mutex_.tryLock();
if (locked) {
mutex_.unlock();
}
return !locked;
}

Counter* AllocatorImpl::makeCounterInternal(StatName name, StatName tag_extracted_name,
const StatNameTagVector& stat_name_tags) {
Counter* Allocator::makeCounterInternal(StatName name, StatName tag_extracted_name,
const StatNameTagVector& stat_name_tags) {
return new CounterImpl(name, *this, tag_extracted_name, stat_name_tags);
}

void AllocatorImpl::forEachCounter(SizeFn f_size, StatFn<Counter> f_stat) const {
void Allocator::forEachCounter(SizeFn f_size, StatFn<Counter> f_stat) const {
Thread::LockGuard lock(mutex_);
if (f_size != nullptr) {
f_size(counters_.size());
Expand All @@ -380,7 +380,7 @@ void AllocatorImpl::forEachCounter(SizeFn f_size, StatFn<Counter> f_stat) const
}
}

void AllocatorImpl::forEachGauge(SizeFn f_size, StatFn<Gauge> f_stat) const {
void Allocator::forEachGauge(SizeFn f_size, StatFn<Gauge> f_stat) const {
Thread::LockGuard lock(mutex_);
if (f_size != nullptr) {
f_size(gauges_.size());
Expand All @@ -390,7 +390,7 @@ void AllocatorImpl::forEachGauge(SizeFn f_size, StatFn<Gauge> f_stat) const {
}
}

void AllocatorImpl::forEachTextReadout(SizeFn f_size, StatFn<TextReadout> f_stat) const {
void Allocator::forEachTextReadout(SizeFn f_size, StatFn<TextReadout> f_stat) const {
Thread::LockGuard lock(mutex_);
if (f_size != nullptr) {
f_size(text_readouts_.size());
Expand All @@ -400,7 +400,7 @@ void AllocatorImpl::forEachTextReadout(SizeFn f_size, StatFn<TextReadout> f_stat
}
}

void AllocatorImpl::forEachSinkedCounter(SizeFn f_size, StatFn<Counter> f_stat) const {
void Allocator::forEachSinkedCounter(SizeFn f_size, StatFn<Counter> f_stat) const {
if (sink_predicates_ != nullptr) {
Thread::LockGuard lock(mutex_);
f_size(sinked_counters_.size());
Expand All @@ -412,7 +412,7 @@ void AllocatorImpl::forEachSinkedCounter(SizeFn f_size, StatFn<Counter> f_stat)
}
}

void AllocatorImpl::forEachSinkedGauge(SizeFn f_size, StatFn<Gauge> f_stat) const {
void Allocator::forEachSinkedGauge(SizeFn f_size, StatFn<Gauge> f_stat) const {
if (sink_predicates_ != nullptr) {
Thread::LockGuard lock(mutex_);
f_size(sinked_gauges_.size());
Expand All @@ -428,7 +428,7 @@ void AllocatorImpl::forEachSinkedGauge(SizeFn f_size, StatFn<Gauge> f_stat) cons
}
}

void AllocatorImpl::forEachSinkedTextReadout(SizeFn f_size, StatFn<TextReadout> f_stat) const {
void Allocator::forEachSinkedTextReadout(SizeFn f_size, StatFn<TextReadout> f_stat) const {
if (sink_predicates_ != nullptr) {
Thread::LockGuard lock(mutex_);
f_size(sinked_text_readouts_.size());
Expand All @@ -440,7 +440,7 @@ void AllocatorImpl::forEachSinkedTextReadout(SizeFn f_size, StatFn<TextReadout>
}
}

void AllocatorImpl::setSinkPredicates(std::unique_ptr<SinkPredicates>&& sink_predicates) {
void Allocator::setSinkPredicates(std::unique_ptr<SinkPredicates>&& sink_predicates) {
Thread::LockGuard lock(mutex_);
ASSERT(sink_predicates_ == nullptr);
sink_predicates_ = std::move(sink_predicates);
Expand All @@ -467,43 +467,43 @@ void AllocatorImpl::setSinkPredicates(std::unique_ptr<SinkPredicates>&& sink_pre
}
}

void AllocatorImpl::markCounterForDeletion(const CounterSharedPtr& counter) {
void Allocator::markCounterForDeletion(const CounterSharedPtr& counter) {
Thread::LockGuard lock(mutex_);
auto iter = counters_.find(counter->statName());
if (iter == counters_.end()) {
// This has already been marked for deletion.
return;
}
ASSERT(counter.get() == *iter);
// Duplicates are ASSERTed in ~AllocatorImpl.
// Duplicates are ASSERTed in ~Allocator.

@gemini-code-assist gemini-code-assist Bot Mar 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

@jmarantz jmarantz Mar 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

deleted_counters_.emplace_back(*iter);
counters_.erase(iter);
sinked_counters_.erase(counter.get());
}

void AllocatorImpl::markGaugeForDeletion(const GaugeSharedPtr& gauge) {
void Allocator::markGaugeForDeletion(const GaugeSharedPtr& gauge) {
Thread::LockGuard lock(mutex_);
auto iter = gauges_.find(gauge->statName());
if (iter == gauges_.end()) {
// This has already been marked for deletion.
return;
}
ASSERT(gauge.get() == *iter);
// Duplicates are ASSERTed in ~AllocatorImpl.
// Duplicates are ASSERTed in ~Allocator.
deleted_gauges_.emplace_back(*iter);
gauges_.erase(iter);
sinked_gauges_.erase(gauge.get());
}

void AllocatorImpl::markTextReadoutForDeletion(const TextReadoutSharedPtr& text_readout) {
void Allocator::markTextReadoutForDeletion(const TextReadoutSharedPtr& text_readout) {
Thread::LockGuard lock(mutex_);
auto iter = text_readouts_.find(text_readout->statName());
if (iter == text_readouts_.end()) {
// This has already been marked for deletion.
return;
}
ASSERT(text_readout.get() == *iter);
// Duplicates are ASSERTed in ~AllocatorImpl.
// Duplicates are ASSERTed in ~Allocator.
deleted_text_readouts_.emplace_back(*iter);
text_readouts_.erase(iter);
sinked_text_readouts_.erase(text_readout.get());
Expand Down
Loading
Loading