Skip to content
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.

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

stats: add new TextReadout stat type #10639

Merged
merged 21 commits into from
Apr 17, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
9 changes: 9 additions & 0 deletions include/envoy/stats/allocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,15 @@ class Allocator {
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, or nullptr if allocation failed, in which case
* tag_extracted_name and tags are not moved.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think allocation can fail anymore (this is a holdover from the shared mem days), right? Can you fix this above also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hrm, good question, unfortunately I'm not familiar with previous shared mem behavior.

The TextReadout behavior is matching the Gauge, and looking at AllocatorImpl::makeTextReadout() it is possible to get nullptr if new TextReadoutImpl() call fails to allocate memory.

This failure isn't really handled as it still inserts nullptr into |text_readouts_| which seems wrong, but maybe is a common pattern?

I'll appreciate any guidance on proper behavior.

Copy link
Member

Choose a reason for hiding this comment

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

We crash on OOM so I think these comments can just be deleted. Thank you!

/wait

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the confirmation! Done.

*/
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;

Expand Down
33 changes: 33 additions & 0 deletions include/envoy/stats/scope.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,14 @@ namespace Stats {
class Counter;
class Gauge;
class Histogram;
class TextReadout;
efimki marked this conversation as resolved.
Show resolved Hide resolved
class Scope;
class NullGaugeImpl;

using CounterOptConstRef = absl::optional<std::reference_wrapper<const Counter>>;
using GaugeOptConstRef = absl::optional<std::reference_wrapper<const Gauge>>;
using HistogramOptConstRef = absl::optional<std::reference_wrapper<const Histogram>>;
using TextReadoutOptConstRef = absl::optional<std::reference_wrapper<const TextReadout>>;
using ScopePtr = std::unique_ptr<Scope>;
using ScopeSharedPtr = std::shared_ptr<Scope>;

Expand Down Expand Up @@ -136,6 +138,31 @@ class Scope {
*/
virtual Histogram& histogramFromString(const std::string& name, Histogram::Unit unit) PURE;

/**
* Creates a TextReadout from the stat name. Tag extraction will be performed on the name.
* @param name The name of the stat, obtained from the SymbolTable.
* @return a text readout within the scope's namespace.
*/
TextReadout& textReadoutFromStatName(const StatName& name) {
return textReadoutFromStatNameWithTags(name, absl::nullopt);
}
/**
efimki marked this conversation as resolved.
Show resolved Hide resolved
* Creates a TextReadout from the stat name and tags. If tags are not provided, tag extraction
* will be performed on the name.
* @param name The name of the stat, obtained from the SymbolTable.
* @param tags optionally specified tags.
* @return a text readout within the scope's namespace.
*/
virtual TextReadout& textReadoutFromStatNameWithTags(const StatName& name,
StatNameTagVectorOptConstRef tags) PURE;

/**
* TODO(#6667): this variant is deprecated: use textReadoutFromStatName.
efimki marked this conversation as resolved.
Show resolved Hide resolved
* @param name The name, expressed as a string.
* @return a text readout within the scope's namespace.
*/
virtual TextReadout& textReadoutFromString(const std::string& name) PURE;

/**
* @param The name of the stat, obtained from the SymbolTable.
* @return a reference to a counter within the scope's namespace, if it exists.
Expand All @@ -155,6 +182,12 @@ class Scope {
*/
virtual HistogramOptConstRef findHistogram(StatName name) const PURE;

/**
* @param The name of the stat, obtained from the SymbolTable.
* @return a reference to a text readout within the scope's namespace, if it exists.
*/
virtual TextReadoutOptConstRef findTextReadout(StatName name) const PURE;

/**
* @return a reference to the symbol table.
*/
Expand Down
13 changes: 13 additions & 0 deletions include/envoy/stats/stats.h
Original file line number Diff line number Diff line change
Expand Up @@ -156,5 +156,18 @@ class Gauge : public Metric {

using GaugeSharedPtr = RefcountPtr<Gauge>;

/**
* A string, possibly non-ASCII.
*/
class TextReadout : public virtual Metric {
public:
virtual ~TextReadout() {}

virtual void set(const std::string& value) PURE;
virtual std::string value() const PURE;
};

using TextReadoutSharedPtr = RefcountPtr<TextReadout>;

} // namespace Stats
} // namespace Envoy
3 changes: 3 additions & 0 deletions include/envoy/stats/stats_macros.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ namespace Envoy {
#define GENERATE_COUNTER_STRUCT(NAME) Envoy::Stats::Counter& NAME##_;
#define GENERATE_GAUGE_STRUCT(NAME, MODE) Envoy::Stats::Gauge& NAME##_;
#define GENERATE_HISTOGRAM_STRUCT(NAME, UNIT) Envoy::Stats::Histogram& NAME##_;
#define GENERATE_TEXT_READOUT_STRUCT(NAME) Envoy::Stats::TextReadout& NAME##_;

#define FINISH_STAT_DECL_(X) #X)),
#define FINISH_STAT_DECL_MODE_(X, MODE) #X), Envoy::Stats::Gauge::ImportMode::MODE),
Expand All @@ -57,10 +58,12 @@ static inline std::string statPrefixJoin(absl::string_view prefix, absl::string_
#define POOL_COUNTER_PREFIX(POOL, PREFIX) (POOL).counterFromString(Envoy::statPrefixJoin(PREFIX, FINISH_STAT_DECL_
#define POOL_GAUGE_PREFIX(POOL, PREFIX) (POOL).gaugeFromString(Envoy::statPrefixJoin(PREFIX, FINISH_STAT_DECL_MODE_
#define POOL_HISTOGRAM_PREFIX(POOL, PREFIX) (POOL).histogramFromString(Envoy::statPrefixJoin(PREFIX, FINISH_STAT_DECL_UNIT_
#define POOL_TEXT_READOUT_PREFIX(POOL, PREFIX) (POOL).textReadoutFromString(Envoy::statPrefixJoin(PREFIX, FINISH_STAT_DECL_

#define POOL_COUNTER(POOL) POOL_COUNTER_PREFIX(POOL, "")
#define POOL_GAUGE(POOL) POOL_GAUGE_PREFIX(POOL, "")
#define POOL_HISTOGRAM(POOL) POOL_HISTOGRAM_PREFIX(POOL, "")
#define POOL_TEXT_READOUT(POOL) POOL_TEXT_READOUT_PREFIX(POOL, "")

#define NULL_STAT_DECL_(X) std::string(#X)),
#define NULL_STAT_DECL_IGNORE_MODE_(X, MODE) std::string(#X)),
Expand Down
5 changes: 5 additions & 0 deletions include/envoy/stats/store.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ class Store : public Scope {
*/
virtual std::vector<GaugeSharedPtr> gauges() const PURE;

/**
* @return a list of all known text readouts.
*/
virtual std::vector<TextReadoutSharedPtr> textReadouts() const PURE;

/**
* @return a list of all known histograms.
*/
Expand Down
12 changes: 12 additions & 0 deletions source/common/stats/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ envoy_cc_library(
":histogram_lib",
":null_counter_lib",
":null_gauge_lib",
":null_text_readout_lib",
":scope_prefixer_lib",
":stats_lib",
":store_impl_lib",
Expand Down Expand Up @@ -100,6 +101,16 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "null_text_readout_lib",
hdrs = ["null_text_readout.h"],
deps = [
":metric_impl_lib",
":symbol_table_lib",
"//include/envoy/stats:stats_interface",
],
)

envoy_cc_library(
name = "recent_lookups_lib",
srcs = ["recent_lookups.cc"],
Expand Down Expand Up @@ -240,6 +251,7 @@ envoy_cc_library(
":allocator_lib",
":null_counter_lib",
":null_gauge_lib",
":null_text_readout_lib",
":scope_prefixer_lib",
":stats_lib",
":stats_matcher_lib",
Expand Down
49 changes: 44 additions & 5 deletions source/common/stats/allocator_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ void AllocatorImpl::debugPrint() {
}
#endif

// Counter and Gauge both inherit from from RefcountInterface and
// Counter, Gauge and TextReadout inherit from RefcountInterface and
// Metric. MetricImpl takes care of most of the Metric API, but we need to cover
// symbolTable() here, which we don't store directly, but get it via the alloc,
// which we need in order to clean up the counter and gauge maps in that class
Expand Down Expand Up @@ -101,10 +101,6 @@ template <class BaseClass> class StatsSharedImpl : public MetricImpl<BaseClass>
protected:
AllocatorImpl& alloc_;

// Holds backing store shared by both CounterImpl and GaugeImpl. CounterImpl
// adds another field, pending_increment_, that is not used in Gauge.
std::atomic<uint64_t> value_{0};

// 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
// already hold the lock. Increment also occurs when copying shared pointers,
Expand Down Expand Up @@ -144,6 +140,7 @@ class CounterImpl : public StatsSharedImpl<Counter> {
uint64_t value() const override { return value_; }

private:
std::atomic<uint64_t> value_{0};
std::atomic<uint64_t> pending_increment_{0};
};

Expand Down Expand Up @@ -226,6 +223,35 @@ class GaugeImpl : public StatsSharedImpl<Gauge> {
break;
}
}

private:
std::atomic<uint64_t> value_{0};
};

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

void removeFromSetLockHeld() EXCLUSIVE_LOCKS_REQUIRED(alloc_.mutex_) override {
const size_t count = alloc_.text_readouts_.erase(statName());
ASSERT(count == 1);
}

// Stats::TextReadout
void set(const std::string& value) override {
Thread::LockGuard lock(mutex_);
value_ = value;
}
std::string value() const override {
Thread::LockGuard lock(mutex_);
return value_;
}

private:
mutable Thread::MutexBasicLockable mutex_;
efimki marked this conversation as resolved.
Show resolved Hide resolved
std::string value_;
jmarantz marked this conversation as resolved.
Show resolved Hide resolved
efimki marked this conversation as resolved.
Show resolved Hide resolved
};

CounterSharedPtr AllocatorImpl::makeCounter(StatName name, StatName tag_extracted_name,
Expand Down Expand Up @@ -256,6 +282,19 @@ 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) {
Thread::LockGuard lock(mutex_);
ASSERT(gauges_.find(name) == gauges_.end());
efimki marked this conversation as resolved.
Show resolved Hide resolved
auto iter = text_readouts_.find(name);
if (iter != text_readouts_.end()) {
return TextReadoutSharedPtr(*iter);
}
auto text_readout =
TextReadoutSharedPtr(new TextReadoutImpl(name, *this, tag_extracted_name, stat_name_tags));
text_readouts_.insert(text_readout.get());
return text_readout;
}
bool AllocatorImpl::isMutexLockedForTest() {
bool locked = mutex_.tryLock();
if (locked) {
Expand Down
5 changes: 5 additions & 0 deletions source/common/stats/allocator_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ class AllocatorImpl : public Allocator {
GaugeSharedPtr makeGauge(StatName name, StatName tag_extracted_name,
const StatNameTagVector& stat_name_tags,
Gauge::ImportMode import_mode) override;
TextReadoutSharedPtr makeTextReadout(StatName name, StatName tag_extracted_name,
const StatNameTagVector& stat_name_tags) override;
SymbolTable& symbolTable() override { return symbol_table_; }
const SymbolTable& constSymbolTable() const override { return symbol_table_; }

Expand All @@ -49,6 +51,7 @@ class AllocatorImpl : public Allocator {
template <class BaseClass> friend class StatsSharedImpl;
friend class CounterImpl;
friend class GaugeImpl;
friend class TextReadoutImpl;

struct HeapStatHash {
using is_transparent = void; // NOLINT(readability-identifier-naming)
Expand All @@ -66,6 +69,7 @@ class AllocatorImpl : public Allocator {

void removeCounterFromSetLockHeld(Counter* counter) EXCLUSIVE_LOCKS_REQUIRED(mutex_);
void removeGaugeFromSetLockHeld(Gauge* gauge) EXCLUSIVE_LOCKS_REQUIRED(mutex_);
void removeTextReadoutFromSetLockHeld(Counter* counter) EXCLUSIVE_LOCKS_REQUIRED(mutex_);

// An unordered set of HeapStatData pointers which keys off the key()
// field in each object. This necessitates a custom comparator and hasher, which key off of the
Expand All @@ -74,6 +78,7 @@ class AllocatorImpl : public Allocator {
using StatSet = absl::flat_hash_set<StatType*, HeapStatHash, HeapStatCompare>;
StatSet<Counter> counters_ GUARDED_BY(mutex_);
StatSet<Gauge> gauges_ GUARDED_BY(mutex_);
StatSet<TextReadout> text_readouts_ GUARDED_BY(mutex_);

SymbolTable& symbol_table_;

Expand Down
3 changes: 3 additions & 0 deletions source/common/stats/isolated_store_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ IsolatedStoreImpl::IsolatedStoreImpl(SymbolTable& symbol_table)
histograms_([this](StatName name, Histogram::Unit unit) -> HistogramSharedPtr {
return HistogramSharedPtr(new HistogramImpl(name, unit, *this, name, StatNameTagVector{}));
}),
text_readouts_([this](StatName name, bool) -> TextReadoutSharedPtr {
return alloc_.makeTextReadout(name, name, StatNameTagVector{});
}),
null_counter_(new NullCounterImpl(symbol_table)),
null_gauge_(new NullGaugeImpl(symbol_table)) {}

Expand Down
32 changes: 32 additions & 0 deletions source/common/stats/isolated_store_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,14 @@ template <class Base> class IsolatedStatsCache {
using CounterAllocator = std::function<RefcountPtr<Base>(StatName name)>;
using GaugeAllocator = std::function<RefcountPtr<Base>(StatName, Gauge::ImportMode)>;
using HistogramAllocator = std::function<RefcountPtr<Base>(StatName, Histogram::Unit)>;
using TextReadoutAllocator =
std::function<RefcountPtr<Base>(StatName name, bool /* lowercase */)>;
using BaseOptConstRef = absl::optional<std::reference_wrapper<const Base>>;

IsolatedStatsCache(CounterAllocator alloc) : counter_alloc_(alloc) {}
IsolatedStatsCache(GaugeAllocator alloc) : gauge_alloc_(alloc) {}
IsolatedStatsCache(HistogramAllocator alloc) : histogram_alloc_(alloc) {}
IsolatedStatsCache(TextReadoutAllocator alloc) : text_readout_alloc_(alloc) {}

Base& get(StatName name) {
auto stat = stats_.find(name);
Expand Down Expand Up @@ -68,6 +71,17 @@ template <class Base> class IsolatedStatsCache {
return *new_stat;
}

Base& get(StatName name, bool b) {
efimki marked this conversation as resolved.
Show resolved Hide resolved
auto stat = stats_.find(name);
if (stat != stats_.end()) {
return *stat->second;
}

RefcountPtr<Base> new_stat = text_readout_alloc_(name, b);
stats_.emplace(new_stat->statName(), new_stat);
return *new_stat;
}

std::vector<RefcountPtr<Base>> toVector() const {
std::vector<RefcountPtr<Base>> vec;
vec.reserve(stats_.size());
Expand All @@ -93,6 +107,7 @@ template <class Base> class IsolatedStatsCache {
CounterAllocator counter_alloc_;
GaugeAllocator gauge_alloc_;
HistogramAllocator histogram_alloc_;
TextReadoutAllocator text_readout_alloc_;
};

class IsolatedStoreImpl : public StoreImpl {
Expand Down Expand Up @@ -124,11 +139,20 @@ class IsolatedStoreImpl : public StoreImpl {
Histogram& histogram = histograms_.get(joiner.nameWithTags(), unit);
return histogram;
}
TextReadout& textReadoutFromStatNameWithTags(const StatName& name,
StatNameTagVectorOptConstRef tags) override {
TagUtility::TagStatNameJoiner joiner(name, tags, symbolTable());
TextReadout& text_readout = text_readouts_.get(joiner.nameWithTags(), false);
return text_readout;
}
CounterOptConstRef findCounter(StatName name) const override { return counters_.find(name); }
GaugeOptConstRef findGauge(StatName name) const override { return gauges_.find(name); }
HistogramOptConstRef findHistogram(StatName name) const override {
return histograms_.find(name);
}
TextReadoutOptConstRef findTextReadout(StatName name) const override {
return text_readouts_.find(name);
}

// Stats::Store
std::vector<CounterSharedPtr> counters() const override { return counters_.toVector(); }
Expand All @@ -143,6 +167,9 @@ class IsolatedStoreImpl : public StoreImpl {
std::vector<ParentHistogramSharedPtr> histograms() const override {
return std::vector<ParentHistogramSharedPtr>{};
}
std::vector<TextReadoutSharedPtr> textReadouts() const override {
return text_readouts_.toVector();
}

Counter& counterFromString(const std::string& name) override {
StatNameManagedStorage storage(name, symbolTable());
Expand All @@ -156,6 +183,10 @@ class IsolatedStoreImpl : public StoreImpl {
StatNameManagedStorage storage(name, symbolTable());
return histogramFromStatName(storage.statName(), unit);
}
TextReadout& textReadoutFromString(const std::string& name) override {
StatNameManagedStorage storage(name, symbolTable());
return textReadoutFromStatName(storage.statName());
}

private:
IsolatedStoreImpl(std::unique_ptr<SymbolTable>&& symbol_table);
Expand All @@ -165,6 +196,7 @@ class IsolatedStoreImpl : public StoreImpl {
IsolatedStatsCache<Counter> counters_;
IsolatedStatsCache<Gauge> gauges_;
IsolatedStatsCache<Histogram> histograms_;
IsolatedStatsCache<TextReadout> text_readouts_;
RefcountPtr<NullCounterImpl> null_counter_;
RefcountPtr<NullGaugeImpl> null_gauge_;
};
Expand Down
Loading