Copilot commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4068058767
##########
src/brpc/builtin/prometheus_metrics_service.cpp:
##########
@@ -86,14 +99,104 @@ class PrometheusMetricsDumper : public bvar::Dumper {
butil::StringPiece GetMetricsName(const std::string& name) {
auto pos = name.find_first_of('{');
- int size = (pos == std::string::npos) ? name.size() : pos;
+ int size = pos == std::string::npos ? name.size() : pos;
return butil::StringPiece(name.data(), size);
}
+// Case-insensitive match of [p, end) against the NUL-terminated, lower-case
+// ASCII `word`. The whole rest of the input must be consumed, so "inf" matches
+// but "infx" does not.
+static bool MatchWordIgnoreCase(const char* p, const char* end, const char*
word) {
+ for (; p != end && *word != '\0'; ++p, ++word) {
+ char c = *p;
+ if (c >= 'A' && c <= 'Z') {
+ c = static_cast<char>(c - 'A' + 'a');
+ }
+ if (c != *word) {
+ return false;
+ }
+ }
+ return p == end && *word == '\0';
+}
+
+// Whether `s` is a number prometheus would accept as a sample value: a float64
+// in the decimal, or one of the specials it spells as "+Inf"/"-Inf"/"NaN"
+// (case-insensitive). The spelled-out "Infinity" is not part of the grammar,
+// so it is rejected even though Go's strconv.ParseFloat would accept it. Nor
+// is a signed NaN: prometheus consumes the sign and then only looks for Inf
+// after it, so "-nan", which is what glibc's printf("%g") makes of a negative
+// NaN, would fail the whole scrape.
+//
+// Everything else is rejected: a quoted string, the json of a
Window<Histogram>
+// or a compound PassiveStatus, and the bare `true`/'false` of a bool gflag,
+// which sniffing only the first char let through. Skipping such a variable
+// is not cosmetic: one malformed line makes prometheus reject the whole
scrape,
+// not just that one metric.
+//
+// Scans the StringPiece in place, no copy and no allocation. Hexadecimal
+// floats are deliberately not accepted.
+bool IsDumpableToPrometheus(butil::StringPiece s) {
+ const char* p = s.data();
+ const char* const end = p + s.size();
+ if (p == end) {
+ return false;
+ }
Review Comment:
`butil::StringPiece()` stores a null data pointer, so computing `p + 0`
before checking whether the piece is empty invokes undefined pointer arithmetic
for a valid empty input. Check `s.empty()` before reading `data()` and forming
`end`.
##########
src/bvar/mvariable.h:
##########
@@ -93,7 +93,7 @@ class MVariableBase {
// `black_wildcards' and send them to `dumper'.
// Use default options when `options' is nullptr.
// Return number of dumped mvariables, -1 on error.
- static size_t dump_exposed(Dumper* dumper, const DumpOptions* options);
+ static int dump_exposed(Dumper* dumper, const DumpOptions* options);
Review Comment:
The implementation and new cap test now count emitted metric samples (one
histogram can contribute many), not dumped mvariables. Keeping the old contract
text makes the public return value misleading; update it to say “dumped
metrics.”
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
