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

Closed
wants to merge 3 commits into from
Closed
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
5 changes: 5 additions & 0 deletions include/envoy/stats/scope.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ class Scope {
*/
virtual Gauge& gauge(const std::string& name) PURE;

/**
* @return a text stat within the scope's namespace.
*/
virtual TextReadout& textReadout(const std::string& name) PURE;

/**
* @return a histogram within the scope's namespace with a particular value type.
*/
Expand Down
8 changes: 8 additions & 0 deletions include/envoy/stats/source.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ class Source {
*/
virtual const std::vector<GaugeSharedPtr>& cachedGauges() PURE;

/**
* Returns all known readouts. Will use cached values if already accessed and clearCache() hasn't
* been called since.
* @return std::vector<TextReadoutSharedPtr>& all known readouts. Note: reference may not be
* valid after clearCache() is called.
*/
virtual const std::vector<TextReadoutSharedPtr>& cachedTextReadouts() PURE;

/**
* Returns all known parent histograms. Will use cached values if already accessed and
* clearCache() hasn't been called since.
Expand Down
11 changes: 11 additions & 0 deletions include/envoy/stats/stat_data_allocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,17 @@ class StatDataAllocator {
virtual GaugeSharedPtr makeGauge(absl::string_view name, std::string&& tag_extracted_name,
std::vector<Tag>&& 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 tags the extracted tag values.
* @return TextReadoutSharedPtr a readout, or nullptr if allocation failed, in which case
* tag_extracted_name and tags are not moved.
*/
virtual TextReadoutSharedPtr makeTextReadout(absl::string_view name,
std::string&& tag_extracted_name,
std::vector<Tag>&& tags) PURE;

/**
* Determines whether this stats allocator requires bounded stat-name size.
*/
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 @@ -94,5 +94,18 @@ class Gauge : public virtual Metric {

typedef std::shared_ptr<Gauge> GaugeSharedPtr;

/**
* A string, possibly non-ASCII.
*/
class TextReadout : public virtual Metric {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about TextGauge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, this new class is meant to remove the need to abuse gauges, so in some sense it's explicitly not a gauge. The name "gauge" suggests the image of a circle with numbers and a needle bouncing around, like what comes up in a Google image search for "gauge".

Copy link
Contributor

Choose a reason for hiding this comment

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

:-) . Agree. It sounds more like a numeric thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this to indicate that it s a stat? If yes, may be

TextMetric/TextStat/TextValuedStat may convey it more obviously?

Another thing, I was looking statsd and prometheus documentation and they do not seem to support text type of stats - so if we migrate the 64-bit hash gauges to this do we have to worry about the existing users?

public:
virtual ~TextReadout() {}

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

typedef std::shared_ptr<TextReadout> TextReadoutSharedPtr;

} // 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 @@ -29,15 +29,18 @@ namespace Envoy {

#define GENERATE_COUNTER_STRUCT(NAME) Stats::Counter& NAME##_;
#define GENERATE_GAUGE_STRUCT(NAME) Stats::Gauge& NAME##_;
#define GENERATE_TEXT_READOUT_STRUCT(NAME) Stats::TextReadout& NAME##_;
#define GENERATE_HISTOGRAM_STRUCT(NAME) Stats::Histogram& NAME##_;

#define FINISH_STAT_DECL_(X) + std::string(#X)),

#define POOL_COUNTER_PREFIX(POOL, PREFIX) (POOL).counter(PREFIX FINISH_STAT_DECL_
#define POOL_GAUGE_PREFIX(POOL, PREFIX) (POOL).gauge(PREFIX FINISH_STAT_DECL_
#define POOL_TEXT_READOUT_PREFIX(POOL, PREFIX) (POOL).textReadout(PREFIX FINISH_STAT_DECL_
#define POOL_HISTOGRAM_PREFIX(POOL, PREFIX) (POOL).histogram(PREFIX FINISH_STAT_DECL_

#define POOL_COUNTER(POOL) POOL_COUNTER_PREFIX(POOL, "")
#define POOL_GAUGE(POOL) POOL_GAUGE_PREFIX(POOL, "")
#define POOL_TEXT_READOUT(POOL) POOL_TEXT_READOUT_PREFIX(POOL, "")
#define POOL_HISTOGRAM(POOL) POOL_HISTOGRAM_PREFIX(POOL, "")
} // namespace Envoy
5 changes: 5 additions & 0 deletions include/envoy/stats/store.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,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
8 changes: 8 additions & 0 deletions source/common/stats/isolated_store_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ IsolatedStoreImpl::IsolatedStoreImpl()
std::vector<Tag> tags;
return alloc_.makeGauge(name, std::move(tag_extracted_name), std::move(tags));
}),
text_readouts_([this](const std::string& name) -> TextReadoutSharedPtr {
std::string tag_extracted_name = name;
std::vector<Tag> tags;
return alloc_.makeTextReadout(name, std::move(tag_extracted_name), std::move(tags));
}),
histograms_([this](const std::string& name) -> HistogramSharedPtr {
return std::make_shared<HistogramImpl>(name, *this, std::string(name), std::vector<Tag>());
}) {}
Expand All @@ -38,6 +43,9 @@ struct IsolatedScopeImpl : public Scope {
void deliverHistogramToSinks(const Histogram&, uint64_t) override {}
Counter& counter(const std::string& name) override { return parent_.counter(prefix_ + name); }
Gauge& gauge(const std::string& name) override { return parent_.gauge(prefix_ + name); }
TextReadout& textReadout(const std::string& name) override {
return parent_.textReadout(prefix_ + name);
}
Histogram& histogram(const std::string& name) override {
return parent_.histogram(prefix_ + name);
}
Expand Down
5 changes: 5 additions & 0 deletions source/common/stats/isolated_store_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ class IsolatedStoreImpl : public Store {
ScopePtr createScope(const std::string& name) override;
void deliverHistogramToSinks(const Histogram&, uint64_t) override {}
Gauge& gauge(const std::string& name) override { return gauges_.get(name); }
TextReadout& textReadout(const std::string& name) override { return text_readouts_.get(name); }
Histogram& histogram(const std::string& name) override {
Histogram& histogram = histograms_.get(name);
return histogram;
Expand All @@ -70,6 +71,9 @@ class IsolatedStoreImpl : public Store {
// Stats::Store
std::vector<CounterSharedPtr> counters() const override { return counters_.toVector(); }
std::vector<GaugeSharedPtr> gauges() const override { return gauges_.toVector(); }
std::vector<TextReadoutSharedPtr> textReadouts() const override {
return text_readouts_.toVector();
}
std::vector<ParentHistogramSharedPtr> histograms() const override {
return std::vector<ParentHistogramSharedPtr>{};
}
Expand All @@ -78,6 +82,7 @@ class IsolatedStoreImpl : public Store {
HeapStatDataAllocator alloc_;
IsolatedStatsCache<Counter> counters_;
IsolatedStatsCache<Gauge> gauges_;
IsolatedStatsCache<TextReadout> text_readouts_;
IsolatedStatsCache<Histogram> histograms_;
const StatsOptionsImpl stats_options_;
};
Expand Down
7 changes: 7 additions & 0 deletions source/common/stats/source_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,12 @@ std::vector<GaugeSharedPtr>& SourceImpl::cachedGauges() {
}
return *gauges_;
}
std::vector<TextReadoutSharedPtr>& SourceImpl::cachedTextReadouts() {
if (!text_readouts_) {
text_readouts_ = store_.textReadouts();
}
return *text_readouts_;
}
std::vector<ParentHistogramSharedPtr>& SourceImpl::cachedHistograms() {
if (!histograms_) {
histograms_ = store_.histograms();
Expand All @@ -27,6 +33,7 @@ std::vector<ParentHistogramSharedPtr>& SourceImpl::cachedHistograms() {
void SourceImpl::clearCache() {
counters_.reset();
gauges_.reset();
text_readouts_.reset();
histograms_.reset();
}

Expand Down
2 changes: 2 additions & 0 deletions source/common/stats/source_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@ class SourceImpl : public Source {
// Stats::Source
std::vector<CounterSharedPtr>& cachedCounters() override;
std::vector<GaugeSharedPtr>& cachedGauges() override;
std::vector<TextReadoutSharedPtr>& cachedTextReadouts() override;
std::vector<ParentHistogramSharedPtr>& cachedHistograms() override;
void clearCache() override;

private:
Store& store_;
absl::optional<std::vector<CounterSharedPtr>> counters_;
absl::optional<std::vector<GaugeSharedPtr>> gauges_;
absl::optional<std::vector<TextReadoutSharedPtr>> text_readouts_;
absl::optional<std::vector<ParentHistogramSharedPtr>> histograms_;
};

Expand Down
84 changes: 82 additions & 2 deletions source/common/stats/stat_data_allocator_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ template <class StatData> class StatDataAllocatorImpl : public StatDataAllocator
std::vector<Tag>&& tags) override;
GaugeSharedPtr makeGauge(absl::string_view name, std::string&& tag_extracted_name,
std::vector<Tag>&& tags) override;
TextReadoutSharedPtr makeTextReadout(absl::string_view name, std::string&& tag_extracted_name,
std::vector<Tag>&& tags) override;

/**
* @param name the full name of the stat.
Expand Down Expand Up @@ -69,6 +71,7 @@ template <class StatData> class CounterImpl : public Counter, public MetricImpl
// Stats::Metric
std::string name() const override { return std::string(data_.name()); }
const char* nameCStr() const override { return data_.name(); }
bool used() const override { return data_.flags_ & Flags::Used; }

// Stats::Counter
void add(uint64_t amount) override {
Expand All @@ -80,7 +83,6 @@ template <class StatData> class CounterImpl : public Counter, public MetricImpl
void inc() override { add(1); }
uint64_t latch() override { return data_.pending_increment_.exchange(0); }
void reset() override { data_.value_ = 0; }
bool used() const override { return data_.flags_ & Flags::Used; }
uint64_t value() const override { return data_.value_; }

private:
Expand Down Expand Up @@ -121,6 +123,7 @@ template <class StatData> class GaugeImpl : public Gauge, public MetricImpl {
// Stats::Metric
std::string name() const override { return std::string(data_.name()); }
const char* nameCStr() const override { return data_.name(); }
bool used() const override { return data_.flags_ & Flags::Used; }

// Stats::Gauge
virtual void add(uint64_t amount) override {
Expand All @@ -139,7 +142,6 @@ template <class StatData> class GaugeImpl : public Gauge, public MetricImpl {
data_.value_ -= amount;
}
virtual uint64_t value() const override { return data_.value_; }
bool used() const override { return data_.flags_ & Flags::Used; }

private:
StatData& data_;
Expand Down Expand Up @@ -167,6 +169,73 @@ class NullGaugeImpl : public Gauge {
uint64_t value() const override { return 0; }
};

/**
* TextReadout implementation that wraps a StatData. Max length 15; set() will truncate.
Copy link
Contributor

Choose a reason for hiding this comment

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

per f2f conversation this is really cool but if we wait till your hot-restart-without-shm PR then this can be simpler and not have limits.

Does it make sense to wait till then?

Copy link
Contributor

Choose a reason for hiding this comment

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

also during an update an observer might see garbage for the string the atomic behavior is per-uint64. In fact the length might be adjusted and expose old data, which is technically a security hole, though you only get 15 bytes of data through :)

*/
template <class StatData> class TextReadoutImpl : public TextReadout, public MetricImpl {
public:
TextReadoutImpl(StatData& data, StatDataAllocatorImpl<StatData>& alloc,
std::string&& tag_extracted_name, std::vector<Tag>&& tags)
: MetricImpl(std::move(tag_extracted_name), std::move(tags)), data_(data), alloc_(alloc) {}
~TextReadoutImpl() { alloc_.free(data_); }

// Stats::Metric
std::string name() const override { return std::string(data_.name()); }
const char* nameCStr() const override { return data_.name(); }
bool used() const override { return data_.flags_ & Flags::Used; }

// Stats::TextReadout
virtual void set(const std::string& value) override {
uint64_t data1 = 0;
uint64_t data2 = 0;
uint8_t* data1_ptr = reinterpret_cast<uint8_t*>(&data1);
uint8_t* data2_ptr = reinterpret_cast<uint8_t*>(&data2);
uint8_t total_length = static_cast<uint8_t>(value.length() <= 15 ? value.length() : 15);
Copy link
Contributor

Choose a reason for hiding this comment

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

std::min(15, value.length);

int length1 = total_length <= 7 ? total_length : 7;
Copy link
Contributor

Choose a reason for hiding this comment

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

std::min(7, total_length);

int length2 = total_length - length1;
data1_ptr[0] = total_length;
memcpy(data1_ptr + 1, value.data(), length1);
if (total_length > 7) {
memcpy(data2_ptr, value.data() + 7, length2);
}
data_.value_.store(data1);
data_.pending_increment_.store(data2);
data_.flags_ |= Flags::Used;
}
virtual std::string value() const override {
uint64_t data1 = data_.value_.load();
uint64_t data2 = data_.pending_increment_.load();
uint8_t* data1_ptr = reinterpret_cast<uint8_t*>(&data1);
uint8_t* data2_ptr = reinterpret_cast<uint8_t*>(&data2);
uint8_t total_length = data1_ptr[0];
char buf[15];
memcpy(buf, data1_ptr + 1, 7);
memcpy(buf + 7, data2_ptr, 8);
return std::string(buf, total_length);
}

private:
StatData& data_;
StatDataAllocatorImpl<StatData>& alloc_;
};

/**
* Null text readout implementation.
* No-ops on all calls and requires no underlying metric or data.
*/
class NullTextReadoutImpl : public TextReadout {
public:
NullTextReadoutImpl() {}
~NullTextReadoutImpl() {}
std::string name() const override { return ""; }
const char* nameCStr() const override { return ""; }
const std::string& tagExtractedName() const override { CONSTRUCT_ON_FIRST_USE(std::string, ""); }
const std::vector<Tag>& tags() const override { CONSTRUCT_ON_FIRST_USE(std::vector<Tag>, {}); }
void set(const std::string&) override {}
bool used() const override { return false; }
std::string value() const override { return std::string(); }
};

template <class StatData>
CounterSharedPtr StatDataAllocatorImpl<StatData>::makeCounter(absl::string_view name,
std::string&& tag_extracted_name,
Expand All @@ -191,5 +260,16 @@ GaugeSharedPtr StatDataAllocatorImpl<StatData>::makeGauge(absl::string_view name
std::move(tags));
}

template <class StatData>
TextReadoutSharedPtr StatDataAllocatorImpl<StatData>::makeTextReadout(
absl::string_view name, std::string&& tag_extracted_name, std::vector<Tag>&& tags) {
StatData* data = alloc(name);
if (data == nullptr) {
return nullptr;
}
return std::make_shared<TextReadoutImpl<StatData>>(*data, *this, std::move(tag_extracted_name),
std::move(tags));
}

} // namespace Stats
} // namespace Envoy
45 changes: 45 additions & 0 deletions source/common/stats/thread_local_store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ void ThreadLocalStoreImpl::setStatsMatcher(StatsMatcherPtr&& stats_matcher) {
for (ScopeImpl* scope : scopes_) {
removeRejectedStats(scope->central_cache_.counters_, deleted_counters_);
removeRejectedStats(scope->central_cache_.gauges_, deleted_gauges_);
removeRejectedStats(scope->central_cache_.text_readouts_, deleted_text_readouts_);
removeRejectedStats(scope->central_cache_.histograms_, deleted_histograms_);
}
}
Expand Down Expand Up @@ -111,6 +112,22 @@ std::vector<GaugeSharedPtr> ThreadLocalStoreImpl::gauges() const {
return ret;
}

std::vector<TextReadoutSharedPtr> ThreadLocalStoreImpl::textReadouts() const {
// Handle de-dup due to overlapping scopes.
std::vector<TextReadoutSharedPtr> ret;
CharStarHashSet names;
Thread::LockGuard lock(lock_);
for (ScopeImpl* scope : scopes_) {
for (auto& text_readout : scope->central_cache_.text_readouts_) {
if (names.insert(text_readout.first).second) {
ret.push_back(text_readout.second);
}
}
}

return ret;
}

std::vector<ParentHistogramSharedPtr> ThreadLocalStoreImpl::histograms() const {
std::vector<ParentHistogramSharedPtr> ret;
Thread::LockGuard lock(lock_);
Expand Down Expand Up @@ -359,6 +376,34 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gauge(const std::string& name) {
tls_cache);
}

TextReadout& ThreadLocalStoreImpl::ScopeImpl::textReadout(const std::string& name) {
// See comments in counter(). There is no super clean way (via templates or otherwise) to
// share this code so I'm leaving it largely duplicated for now.
//
// Note that we can do map.find(final_name.c_str()), but we cannot do
// map[final_name.c_str()] as the char*-keyed maps would then save the pointer to
// a temporary, and address sanitization errors would follow. Instead we must
// do a find() first, using that if it succeeds. If it fails, then after we
// construct the stat we can insert it into the required maps.
std::string final_name = prefix_ + name;
if (parent_.rejects(final_name)) {
return null_readout_;
}

StatMap<TextReadoutSharedPtr>* tls_cache = nullptr;
if (!parent_.shutting_down_ && parent_.tls_) {
tls_cache = &parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_].text_readouts_;
}

return safeMakeStat<TextReadout>(
final_name, central_cache_.text_readouts_,
[](StatDataAllocator& allocator, absl::string_view name, std::string&& tag_extracted_name,
std::vector<Tag>&& tags) -> TextReadoutSharedPtr {
return allocator.makeTextReadout(name, std::move(tag_extracted_name), std::move(tags));
},
tls_cache);
}

Histogram& ThreadLocalStoreImpl::ScopeImpl::histogram(const std::string& name) {
// See comments in counter(). There is no super clean way (via templates or otherwise) to
// share this code so I'm leaving it largely duplicated for now.
Expand Down
Loading