-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -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 { | ||
|
@@ -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: | ||
|
@@ -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 { | ||
|
@@ -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_; | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about TextGauge?
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?