stats: add new TextReadout stat type #5844
Conversation
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
| /** | ||
| * A string, possibly non-ASCII. | ||
| */ | ||
| class TextReadout : public virtual Metric { |
There was a problem hiding this comment.
How about TextGauge?
There was a problem hiding this comment.
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.
:-) . Agree. It sounds more like a numeric thing.
There was a problem hiding this comment.
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?
| 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.
std::min(15, value.length);
| 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); | ||
| int length1 = total_length <= 7 ? total_length : 7; |
There was a problem hiding this comment.
std::min(7, total_length);
| @@ -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.
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.
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 :)
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
Per discussion I think we should temporarily close this one, and re-open after shm stats are no longer needed for hot-restart. @fredlas is that OK? |
stale
bot
removed
the
stale
|
Sure, makes sense. |
This PR adds API support required for envoyproxy#5770 . Once envoyproxy#5844 lands, I will output it using the text stat. Risk Level: Low Testing: N/A Signed-off-by: Rama Chavali <rama.rao@salesforce.com> Signed-off-by: Fred Douglas <fredlas@google.com>
|
Can we reopen this? |
|
I don't think we can re-open the PR as it's on a deleted branch, but I re-opened #5790 to track the request. Feel free to try to patch this PR into a new branch and go for it, but be aware that some of the underlying code this PR changes might have changed to much to make this patchable. |
Based on envoyproxy#5844 by @fredlas. Signed-off-by: Misha Efimov <mef@google.com>
* stats: add new TextReadout stat type Based on envoyproxy#5844 by @fredlas. Signed-off-by: Misha Efimov <mef@google.com> Signed-off-by: pengg <pengg@google.com>
Description: Add the TextReadout stat type, for holding (max 15 bytes) strings. A followup PR will migrate the 64-bit hash gauges.
I chose TextReadout because Text/TextSharedPtr is not obviously something other than a slightly-wrapped string; in particular it's not obvious that it's a stat. On the other hand, I wasn't entirely sure if just Readout would imply a string strongly enough to make everyone happy. I'm open to suggestions (including chopping it down to Readout).
We could get to 16 bytes if we're willing to use bits in the Metric's flags to store length, but the need to stay within the numeric stats' memory footprint is temporary anyways.
Risk Level: low
Testing: roughly duplicated every bit of test logic involving the word "gauge", added thorough test of the split-across-two-uint64_ts string storage
#4974 #5770 #5790 @ramaraochavali