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 all 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
14 changes: 10 additions & 4 deletions include/envoy/stats/allocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@ class Allocator {
* @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, or nullptr if allocation failed, in which case
* tag_extracted_name and tags are not moved.
* @return CounterSharedPtr a counter.
*/
virtual CounterSharedPtr makeCounter(StatName name, StatName tag_extracted_name,
const StatNameTagVector& stat_name_tags) PURE;
Expand All @@ -42,13 +41,20 @@ class Allocator {
* @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, or nullptr if allocation failed, in which case
* tag_extracted_name and tags are not moved.
* @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;

Expand Down
36 changes: 35 additions & 1 deletion 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 Scope;
class NullGaugeImpl;
class Scope;
class TextReadout;

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,32 @@ 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 +183,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
1 change: 1 addition & 0 deletions include/envoy/stats/sink.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class MetricSnapshot {
* @return a snapshot of all histograms.
*/
virtual const std::vector<std::reference_wrapper<const ParentHistogram>>& histograms() PURE;
// TODO(efimki): Add support of text readouts stats.
};

/**
Expand Down
27 changes: 27 additions & 0 deletions include/envoy/stats/stats.h
Original file line number Diff line number Diff line change
Expand Up @@ -156,5 +156,32 @@ class Gauge : public Metric {

using GaugeSharedPtr = RefcountPtr<Gauge>;

/**
* A string, possibly non-ASCII.
*/
class TextReadout : public virtual Metric {
public:
// Text readout type is used internally to disambiguate isolated store
// constructors. In the future we can extend it to specify text encoding or
// some such.
enum class Type {
Default, // No particular meaning.
};

~TextReadout() override = default;

/**
* Sets the value of this TextReadout by moving the input |value| to minimize
* buffer copies under the lock.
*/
virtual void set(std::string&& value) PURE;
efimki marked this conversation as resolved.
Show resolved Hide resolved
/**
* @return the copy of this TextReadout value.
*/
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
53 changes: 48 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,12 +223,42 @@ 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(std::string&& value) override {
absl::MutexLock lock(&mutex_);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if this will be contentious. WDYT of taking the 'value' param as an && param and swapping it or moving it with value_, so we don't have to copy bytes while holding the lock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion! Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually I just thought of another idea; may be this would be more intuitive?

Leave this API as is, but change the implementation to copy the input param to a local without taking the lock, and then std::move it into the structure after taking the lock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that having set() method move input value is quite surprising, and I was going to suggest to keep const std::string& and add swap(std::string& value) method, but I did find quite a few methods that take &&, so I've scraped that. :)

To be honest I would argue that lock contention is not an issue as I don't expect to have text readout values changed racily from multiple threads.
I might be coming from a very narrow use case point of view, but in that use case the value of the text readout is unlikely to change ever.
What use case are you thinking about?

value_ = std::move(value);
}
std::string value() const override {
absl::MutexLock lock(&mutex_);
return value_;
}

private:
mutable absl::Mutex mutex_;
std::string value_ ABSL_GUARDED_BY(mutex_);
};

CounterSharedPtr AllocatorImpl::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());
auto iter = counters_.find(name);
if (iter != counters_.end()) {
return CounterSharedPtr(*iter);
Expand All @@ -246,6 +273,7 @@ GaugeSharedPtr AllocatorImpl::makeGauge(StatName name, StatName tag_extracted_na
Gauge::ImportMode import_mode) {
Thread::LockGuard lock(mutex_);
ASSERT(counters_.find(name) == counters_.end());
ASSERT(text_readouts_.find(name) == text_readouts_.end());
auto iter = gauges_.find(name);
if (iter != gauges_.end()) {
return GaugeSharedPtr(*iter);
Expand All @@ -256,6 +284,21 @@ 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(counters_.find(name) == counters_.end());
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, TextReadout::Type) -> TextReadoutSharedPtr {
return alloc_.makeTextReadout(name, name, StatNameTagVector{});
}),
null_counter_(new NullCounterImpl(symbol_table)),
null_gauge_(new NullGaugeImpl(symbol_table)) {}

Expand Down
Loading