You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "chenBright (via GitHub)" <gi...@apache.org> on 2026/09/21 07:56:10 UTC
[PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
chenBright opened a new pull request, #3557:
URL: https://github.com/apache/brpc/pull/3557
### What problem does this PR solve?
Issue Number: resolve
Problem Summary:
1. bvar lacks a histogram metric: users cannot export bucketed observations
(Prometheus `histogram` type).
3. A bvar and an mbvar can expose the same Prometheus metric name after
separator normalization. Prometheus rejects the whole scrape when a metric
name appears with different types/label sets.
5. A `MultiDimension` label may collide with a label reserved by the underlying
metric (e.g. `le` of Histogram, `quantile` of LatencyRecorder), producing
conflicting samples.
### What is changed and the side effects?
Changed:
- Add `bvar::Histogram` with explicit bucket bounds (`BucketSchema`), exporting
`_bucket{le=...}` / `_sum` / `_count` families, with `bvar::Window` support.
- Add `bvar::detail::PrometheusNameRegistry`: `Variable::expose*` and `
MVariable::expose*` now allocate normalized Prometheus names through one
shared registry, so a name taken by a bvar cannot be re-used by an mbvar and
vice versa; names are released on `hide()` / `hide_all()`.
- Composite metrics declare themselves via `MetricFamily`; `MultiDimension` rejects
outer labels that collide with the metric's reserved labels, for builtin and user-defined
composite metrics alike.
Side effects:
- Performance effects:
- Breaking backward compatibility:
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4068266819
##########
src/bvar/histogram.cpp:
##########
@@ -0,0 +1,258 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <algorithm> // std::adjacent_find
+#include <functional> // std::greater_equal
+#include <iterator> // std::next
+#include <string> // std::string
+#include "butil/float_util.h" // butil::IsFinite
+#include "butil/logging.h" // LOG
+#include "butil/strings/string_number_conversions.h" // butil::DoubleToString
+#include "bvar/histogram.h"
+
+namespace bvar {
+
+Histogram::BucketSchema::BucketSchema(std::initializer_list<double> bounds)
+ : _bounds(bounds) {
+ validate_bounds();
+}
+
+Histogram::BucketSchema::BucketSchema(const std::vector<double>& bounds)
+ : _bounds(bounds) {
+ validate_bounds();
+}
+
+void Histogram::BucketSchema::validate_bounds() {
+ for (auto it = _bounds.begin(); it != _bounds.end();) {
+ if (!butil::IsFinite(*it)) {
+ LOG(ERROR) << "Bucket bounds must be finite, dropping " << *it;
+ it = _bounds.erase(it);
+ } else {
+ ++it;
+ }
+ }
+
+ // Drop the bounds that do not keep the sequence strictly ascending.
+ auto it = _bounds.begin();
+ while ((it = std::adjacent_find(it, _bounds.end(),
+ std::greater_equal<>())) != _bounds.end()) {
+ auto bad = std::next(it);
+ LOG(ERROR) << "Bucket bounds must be strictly ascending, dropping "
+ << *bad << " which is not greater than " << *it;
+ _bounds.erase(bad);
+ }
+ if (_bounds.size() >= MAX_HISTOGRAM_BUCKETS) {
+ // Shrinking a vector drops the tail, keeping the tightest bounds.
+ LOG(ERROR) << "A histogram takes at most " << MAX_HISTOGRAM_BUCKETS - 1
+ << " bounds, dropping the "
+ << _bounds.size() - (MAX_HISTOGRAM_BUCKETS - 1)
+ << " ones past the limit";
+ _bounds.resize(MAX_HISTOGRAM_BUCKETS - 1);
+ }
+ if (_bounds.empty()) {
+ // A schema without any bound would send everything into the +Inf
+ // bucket, which no quantile can be read off. Keep the histogram
+ // usable rather than aborting the process over a misconfiguration.
+ LOG(ERROR) << "A histogram needs at least one bound, falling back to 1";
+ _bounds.push_back(1.0);
+ }
+}
+
+std::ostream& operator<<(std::ostream& os, const Histogram::Value& v) {
+ os << "{\"count\":" << v.num << ",\"sum\":"
+ << detail::prometheus_double_to_string(v.sum)
+ << ",\"counts\":[";
+ size_t nbuckets = std::min(v.num_buckets, MAX_HISTOGRAM_BUCKETS);
+ for (size_t i = 0; i < nbuckets; ++i) {
+ if (i != 0) {
+ os << ',';
+ }
+ os << v.counts[i];
+ }
+ return os << "]}";
+}
+
+Histogram::Histogram(const BucketSchema& schema)
+ : _schema(schema)
+ // Both identities carry `num_buckets` so that a value combined out of no
+ // agent at all still knows how wide it is.
+ , _combiner(std::make_shared<combiner_type>(value_type(schema.num_buckets()),
+ value_type(schema.num_buckets())))
+ , _sampler(nullptr) {
+}
+
+Histogram::Histogram(const butil::StringPiece& name, const BucketSchema& schema)
+ : Histogram(schema) {
+ expose(name);
+}
+
+Histogram::Histogram(const butil::StringPiece& prefix,
+ const butil::StringPiece& name,
+ const BucketSchema& schema)
+ : Histogram(schema) {
+ expose_as(prefix, name);
+}
+
+Histogram::~Histogram() {
+ // Calling hide() manually is a MUST required by Variable.
+ hide();
+ if (_sampler != nullptr) {
+ _sampler->destroy();
+ }
+}
+
+Histogram& Histogram::operator<<(double value) {
+ if (BAIDU_UNLIKELY(!butil::IsFinite(value))) {
+ LOG_EVERY_SECOND(WARNING) << "Ignoring non-finite value=" << value
+ << " recorded into Histogram(" << name() << ')';
+ return *this;
+ }
+ agent_type* agent = _combiner->get_or_create_tls_agent();
+ if (BAIDU_UNLIKELY(agent == nullptr)) {
+ LOG(FATAL) << "Fail to create agent";
+ return *this;
+ }
+ // `_schema` outlives the call, the op only borrows it to find the bucket.
+ agent->element.modify(detail::AddSampleToHistogram(&_schema), value);
+ return *this;
+}
+
+Histogram::sampler_type* Histogram::get_sampler() {
+ if (_sampler == nullptr) {
+ _sampler = new sampler_type(this);
+ _sampler->set_debug_name(name());
+ _sampler->schedule();
+ }
+ return _sampler;
+}
+
+int Histogram::expose_impl(const butil::StringPiece& prefix,
+ const butil::StringPiece& name,
+ DisplayFilter display_filter) {
+ int rc = Variable::expose_impl(prefix, name, display_filter);
+ if (rc == 0 && _sampler != nullptr) {
+ _sampler->set_debug_name(this->name());
+ }
+ return rc;
+}
+
+void Histogram::describe(std::ostream& os, bool /*quote_string*/) const {
+ value_type v = get_value();
+ os << "{\"count\":" << v.num
+ << ",\"sum\":" << detail::prometheus_double_to_string(v.sum)
+ << ",\"bounds\":[";
Review Comment:
Fixed in 901c61450c9e47221b0ebd0da0f01351f5307f53 .
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4061539602
##########
src/brpc/builtin/prometheus_metrics_service.cpp:
##########
@@ -228,19 +311,18 @@ void PrometheusMetricsService::default_method(::google::protobuf::RpcController*
int DumpPrometheusMetricsToIOBuf(butil::IOBuf* output) {
butil::IOBufBuilder os;
PrometheusMetricsDumper dumper(&os, g_server_info_prefix);
- const int ndump = bvar::Variable::dump_exposed(&dumper, nullptr);
+ int ndump = bvar::Variable::dump_exposed(&dumper, nullptr);
if (ndump < 0) {
return -1;
}
os.move_to(*output);
if (bvar::FLAGS_bvar_max_dump_multi_dimension_metric_number > 0) {
PrometheusMetricsDumper dumper_md(&os, g_server_info_prefix);
- const int ndump_md = bvar::MVariableBase::dump_exposed(&dumper_md, nullptr);
- if (ndump_md < 0) {
- return -1;
+ size_t ndump_md = bvar::MVariableBase::dump_exposed(&dumper_md, nullptr);
+ if (ndump_md > 0) {
+ output->append(butil::IOBuf::Movable(os.buf()));
}
Review Comment:
`MVariableBase::dump_exposed` documents and implements `-1` as its error sentinel, but its return type is `size_t`; this change stores that sentinel in `ndump_md` and only checks `> 0`. Any error is therefore treated as a successful non-empty dump, and the partial buffer is appended instead of returning `-1` as before. Preserve the sentinel check before appending the buffer.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4060447686
##########
src/bvar/multi_dimension.h:
##########
@@ -84,14 +98,32 @@ class MultiDimension : public MVariable<KeyType> {
typedef butil::DoublyBufferedData<MetricMap> MetricMapDBD;
typedef typename MetricMapDBD::ScopedPtr MetricMapScopedPtr;
- explicit MultiDimension(const key_type& labels);
-
+ // `args` are copied and supplied as const references to each value's
+ // constructor. Only overloads that can construct T this way participate.
+ // With no args, T must be default-constructible. A Histogram is the
+ // typical one, its buckets are fixed at construction:
+ // bvar::MultiDimension<bvar::Histogram> h(
+ // "rpc_latency", {"method"},
+ // bvar::Histogram::BucketSchema({10, 50, 100, 500, 1000}));
+ // They are copied once into the MultiDimension, nothing needs to outlive
+ // the call.
+ template <typename... Args,
+ std::enable_if_t<std::is_constructible<
+ T, const typename std::decay<Args>::type&...>::value, int> = 0>
Review Comment:
bRPC no longer supports C++11.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4061056478
##########
src/brpc/builtin/prometheus_metrics_service.cpp:
##########
@@ -86,14 +86,96 @@ 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, "Infinity" allowed).
+//
+// 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;
+ }
+ if (*p == '+' || *p == '-') {
+ ++p;
+ if (p == end) {
+ return false; // a lone sign
+ }
+ }
+ const char c = *p;
+ if (c >= '0' && c <= '9') {
+ // "123", "123." or "123.45".
+ while (p != end && *p >= '0' && *p <= '9') {
+ ++p;
+ }
+ if (p != end && *p == '.') {
+ ++p;
+ while (p != end && *p >= '0' && *p <= '9') {
+ ++p;
+ }
+ }
+ } else if (c == '.') {
+ // ".5": the digits before the point may be omitted, those after may not.
+ ++p;
+ if (p == end || *p < '0' || *p > '9') {
+ return false;
+ }
+ while (p != end && *p >= '0' && *p <= '9') {
+ ++p;
+ }
+ } else {
+ return MatchWordIgnoreCase(p, end, "inf") ||
+ MatchWordIgnoreCase(p, end, "infinity") ||
+ MatchWordIgnoreCase(p, end, "nan");
Review Comment:
`Infinity` is not a valid Prometheus text-format sample literal; the grammar accepts `NaN`, `+Inf`, and `-Inf` (with the usual numeric forms), not the spelled-out `Infinity`. Returning true here causes a value such as `Infinity` to be emitted and can make the entire `/brpc_metrics` scrape invalid. Please remove the `infinity` alternative and its corresponding positive tests.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4061767892
##########
src/bvar/multi_dimension_inl.h:
##########
@@ -224,177 +235,145 @@ bool MultiDimension<T, KeyType, Shared>::has_stats(const K& labels_value) {
template <typename T, typename KeyType, bool Shared>
template <typename U>
-typename std::enable_if<!butil::is_same<LatencyRecorder, U>::value, size_t>::type
+std::enable_if_t<!detail::IsCompositeMetric<U>::value, size_t>
MultiDimension<T, KeyType, Shared>::dump_impl(Dumper* dumper, const DumpOptions* options) {
std::vector<key_type> label_names;
list_stats(&label_names);
if (label_names.empty() || !dumper->dump_comment(this->name(), METRIC_TYPE_GAUGE)) {
return 0;
}
size_t n = 0;
+ std::string key;
for (auto &label_name : label_names) {
value_ptr_type bvar = get_stats_impl(label_name);
if (nullptr == bvar) {
continue;
}
std::ostringstream oss;
bvar->describe(oss, options->quote_string);
- std::ostringstream oss_key;
- make_dump_key(oss_key, label_name);
- if (!dumper->dump_mvar(oss_key.str(), oss.str())) {
+ make_dump_key(&key, label_name);
+ if (!dumper->dump_mvar(key, oss.str())) {
continue;
}
n++;
}
return n;
}
+namespace detail {
+// Forwards to another Dumper and counts the metrics that went through, which is
+// how MultiDimension answers with the number of dumped metrics rather than the
+// number of times it called dump_samples().
+class CountingDumper : public Dumper {
+public:
+ explicit CountingDumper(Dumper* dumper) : _dumper(dumper), _count(0) {}
+
+ // Only what the wrapped dumper accepted is counted: a false is a request to
+ // stop, that metric did not make it out.
+ bool dump(const std::string& name, const butil::StringPiece& desc) override {
+ if (!_dumper->dump(name, desc)) {
+ return false;
+ }
+ ++_count;
+ return true;
+ }
+ bool dump_mvar(const std::string& name, const butil::StringPiece& desc) override {
+ if (!_dumper->dump_mvar(name, desc)) {
+ return false;
+ }
+ ++_count;
+ return true;
+ }
+ // A comment describes a family, it is not a metric of its own.
+ bool dump_comment(const std::string& name, const std::string& type) override {
+ return _dumper->dump_comment(name, type);
+ }
+
+ size_t count() const { return _count; }
+
+private:
+ Dumper* _dumper;
+ size_t _count;
+};
+} // namespace detail
+
template <typename T, typename KeyType, bool Shared>
template <typename U>
-typename std::enable_if<butil::is_same<LatencyRecorder, U>::value, size_t>::type
+std::enable_if_t<detail::IsCompositeMetric<U>::value, size_t>
MultiDimension<T, KeyType, Shared>::dump_impl(Dumper* dumper, const DumpOptions*) {
std::vector<key_type> label_names;
list_stats(&label_names);
if (label_names.empty()) {
return 0;
}
- // The latency of one quantile. The quantile must be a fraction to meet
- // prometheus specification, e.g. 0.99 for p99.
- struct LatencyPercentile {
- double quantile;
- int64_t latency;
- };
- // All the values dumped for one label set.
- struct DumpedStats {
- const key_type* label_name;
- LatencyPercentile latency_percentiles[5];
- int64_t avg_latency;
- int64_t max_latency;
- int64_t qps;
- int64_t count;
- };
- // Read all the values in one traversal, so that a LatencyRecorder is looked
- // up only once no matter how many metrics are dumped for it. Keep the values
- // instead of the LatencyRecorder pointers, which delete_stats() may free.
- std::vector<DumpedStats> stats_list;
- stats_list.reserve(label_names.size());
- for (const auto& label_name : label_names) {
- bvar::LatencyRecorder* bvar = get_stats_impl(label_name);
- if (!bvar) {
- continue;
+ const std::vector<MetricFamily>& families = U::list_metric_families();
+ detail::CountingDumper counting_dumper(dumper);
+ std::string family_name;
+ std::string labels;
+ // Families outside, label sets inside.
+ for (size_t f = 0; f < families.size(); ++f) {
+ // suffix is nullable, as collect_metric_family_names() knows.
+ family_name.assign(this->name());
+ if (families[f].suffix != nullptr) {
+ family_name.append(families[f].suffix);
}
- DumpedStats stats{};
- stats.label_name = &label_name;
- stats.latency_percentiles[0].quantile = FLAGS_bvar_latency_p1 / 100.0;
- stats.latency_percentiles[1].quantile = FLAGS_bvar_latency_p2 / 100.0;
- stats.latency_percentiles[2].quantile = FLAGS_bvar_latency_p3 / 100.0;
- stats.latency_percentiles[3].quantile = 0.999;
- stats.latency_percentiles[4].quantile = 0.9999;
- for (auto& lp : stats.latency_percentiles) {
- lp.latency = bvar->latency_percentile(lp.quantile);
+ // One TYPE line per family, ahead of all its samples.
+ if (!counting_dumper.dump_comment(family_name, families[f].type)) {
+ break;
}
- stats.avg_latency = bvar->latency();
- stats.max_latency = bvar->max_latency();
- stats.qps = bvar->qps();
- stats.count = bvar->count();
- stats_list.push_back(stats);
- }
-
- size_t n = 0;
-
- // To meet prometheus specification, we must guarantee no second TYPE line for one metric name
-
- // latency comment
- dumper->dump_comment(this->name() + "_latency", METRIC_TYPE_GAUGE);
- for (const auto& stats : stats_list) {
- for (const auto& lp : stats.latency_percentiles) {
- std::ostringstream oss_latency_key;
- make_dump_key(oss_latency_key, *stats.label_name, "_latency", lp.quantile);
- if (dumper->dump_mvar(oss_latency_key.str(), std::to_string(lp.latency))) {
- n++;
+ for (const auto& label_name : label_names) {
+ value_ptr_type bvar = get_stats_impl(label_name);
+ if (bvar == nullptr) {
+ continue;
+ }
+ labels.clear();
+ append_labels_kvpair_body(&labels, label_name);
+ // A false asks to stop dumping, as Dumper::dump() does. Going on
+ // would write samples under a family whose TYPE line the dumper
+ // has already given up on.
+ if (!bvar->dump_samples(&counting_dumper, f, family_name, labels)) {
Review Comment:
The generic composite dump path now keeps a raw `value_ptr_type` from `get_stats_impl()` while calling `dump_samples()`. For the default `Shared=false` MultiDimension, `delete_stats()`/`clear_stats()` can delete that object after the map read is released; the previous LatencyRecorder-specific implementation deliberately copied all values first because deletion could free the pointers. A concurrent clear/delete can therefore make `dump_samples()` dereference freed storage. Snapshot the composite values (or otherwise hold ownership) before emitting samples, as the old LatencyRecorder path did.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4068277537
##########
src/bvar/latency_recorder.cpp:
##########
@@ -235,6 +240,16 @@ int LatencyRecorder::expose(const butil::StringPiece& prefix1,
_latency.set_debug_name(prefix);
_latency_percentile.set_debug_name(prefix);
+ // Exposing a LatencyRecorder is all-or-nothing: the prometheus exporter
+ // turns the sub-bvars below back into one summary and would happily write
+ // that summary out with empty values if only some of them made it.
+ bool expose_succeeded = false;
+ BUTIL_SCOPE_EXIT {
+ if (!expose_succeeded) {
+ hide();
+ }
+ };
Review Comment:
`DumpLatencyRecorderSuffix()` synthesizes the summary names for every exposed bvar whose name starts with `g_server_info_prefix`, not only `MethodStatus` (see `prometheus_metrics_service.cpp:273-286`). This new all-or-nothing path still reserves only the child names, so a standalone `LatencyRecorder` exposed as `rpc_server_0_foo` can coexist with a plain bvar named `rpc_server_0_foo` or `rpc_server_0_foo_sum`; the scrape then contains duplicate metric names. Reserve and release the synthesized names in `LatencyRecorder::expose()`/`hide()` as well, or make the exporter synthesis apply only to `MethodStatus`.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4069132530
##########
src/bvar/latency_recorder.cpp:
##########
@@ -235,6 +240,16 @@ int LatencyRecorder::expose(const butil::StringPiece& prefix1,
_latency.set_debug_name(prefix);
_latency_percentile.set_debug_name(prefix);
+ // Exposing a LatencyRecorder is all-or-nothing: the prometheus exporter
+ // turns the sub-bvars below back into one summary and would happily write
+ // that summary out with empty values if only some of them made it.
+ bool expose_succeeded = false;
+ BUTIL_SCOPE_EXIT {
+ if (!expose_succeeded) {
+ hide();
+ }
+ };
Review Comment:
In d01fd3e16267686a38680fe20454085c8371d41e , Moved the deduplication into `PrometheusMetricsDumper`, which is the only place that knows about the synthesized names. It records every metric name it writes during a scrape, including the ones it makes up and the ones the mbvar pass writes, and skips a family whose name is already taken after logging an error, all or nothing so that half a summary never reaches the scrape.
The name reservation registry is gone with it, so `expose()` keeps its old behaviour and nothing that used to expose successfully starts failing. A collision is now reported at scrape time instead, and prometheus still gets a response it can parse.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3557:
URL: https://github.com/apache/brpc/pull/3557#issuecomment-5761777204
Is this change incompatible with some existing user code? Consider add a flag
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4060333756
##########
docs/en/bvar_c++.md:
##########
@@ -375,6 +375,56 @@ write_latency << the_latency_of_write;
```
+# bvar::Histogram
+
+Counts the recorded values into a fixed set of buckets, namely the prometheus histogram.
+
+```c++
+// Explicit bucket bounds of your own, plus a +Inf bucket.
+bvar::Histogram g_write_latency("table2_my_table_write_latency",
+ {10, 20, 50, 100, 500, 1000, 5000});
+// In your write function
+g_write_latency << the_latency_of_write;
+```
+
+A Histogram has no default constructor either: the bucket bounds must be given at construction (initializer_list / vector):
+
+```c++
+bvar::Histogram g_size("foo_size", {128, 1024, 8192, 65536});
+
+std::vector<double> bounds = LoadBoundsFromConfig();
+bvar::Histogram g_size2("foo_size2", bvar::Histogram::BucketSchema(bounds));
+```
+
+The upper bounds of the buckets are described by a Histogram::BucketSchema, following the semantics of the prometheus `le` label: the bucket at index i counts the values v satisfying `bound_at(i-1) < v <= bound_at(i)`, and one extra `+Inf` bucket counts the rest.
+
+At most `MAX_HISTOGRAM_BUCKETS` (32, the `+Inf` one included) buckets are allowed. Bounds and observations are doubles, and bounds must be finite, strictly ascending and non-empty; otherwise Histogram::BucketSchema logs an error and drops the offending bounds (falling back to `{1}` if nothing is left), and non-finite observations are ignored.
+
+A Histogram exports a family of prometheus metrics rather than a single value:
+```
+# HELP table2_my_table_write_latency
+# TYPE table2_my_table_write_latency histogram
+table2_my_table_write_latency_bucket{le="10"} 3
+table2_my_table_write_latency_bucket{le="20"} 17
+...
+table2_my_table_write_latency_bucket{le="+Inf"} 4021
+table2_my_table_write_latency_sum 1234567
+table2_my_table_write_latency_count 4021
+```
+
+Bucket counts accumulate since construction and never decrease; when exported, they are converted to the cumulative `le` counts that prometheus expects. Quantiles are computed by the prometheus monitoring system: `histogram_quantile(rate(table2_my_table_write_latency_bucket[1m]), 0.99)`.
Review Comment:
The PromQL example has the arguments to `histogram_quantile` reversed and omits the required aggregation by `le`. It should use the quantile first and a `sum by (le)(rate(...))` expression as the second argument; otherwise the documented query is invalid or produces incorrect results.
##########
src/bvar/multi_dimension.h:
##########
@@ -84,14 +98,32 @@ class MultiDimension : public MVariable<KeyType> {
typedef butil::DoublyBufferedData<MetricMap> MetricMapDBD;
typedef typename MetricMapDBD::ScopedPtr MetricMapScopedPtr;
- explicit MultiDimension(const key_type& labels);
-
+ // `args` are copied and supplied as const references to each value's
+ // constructor. Only overloads that can construct T this way participate.
+ // With no args, T must be default-constructible. A Histogram is the
+ // typical one, its buckets are fixed at construction:
+ // bvar::MultiDimension<bvar::Histogram> h(
+ // "rpc_latency", {"method"},
+ // bvar::Histogram::BucketSchema({10, 50, 100, 500, 1000}));
+ // They are copied once into the MultiDimension, nothing needs to outlive
+ // the call.
+ template <typename... Args,
+ std::enable_if_t<std::is_constructible<
+ T, const typename std::decay<Args>::type&...>::value, int> = 0>
Review Comment:
This introduces `std::enable_if_t`, which is a C++14 alias, while the surrounding code uses the C++11 `typename std::enable_if<...>::type` form. If the project still supports its existing C++11 toolchains, this header will fail to compile; use the C++11 form or update the project language requirement consistently.
##########
src/bvar/multi_dimension_inl.h:
##########
@@ -224,177 +235,141 @@ bool MultiDimension<T, KeyType, Shared>::has_stats(const K& labels_value) {
template <typename T, typename KeyType, bool Shared>
template <typename U>
-typename std::enable_if<!butil::is_same<LatencyRecorder, U>::value, size_t>::type
+std::enable_if_t<!detail::IsCompositeMetric<U>::value, size_t>
MultiDimension<T, KeyType, Shared>::dump_impl(Dumper* dumper, const DumpOptions* options) {
std::vector<key_type> label_names;
list_stats(&label_names);
if (label_names.empty() || !dumper->dump_comment(this->name(), METRIC_TYPE_GAUGE)) {
return 0;
}
size_t n = 0;
+ std::string key;
for (auto &label_name : label_names) {
value_ptr_type bvar = get_stats_impl(label_name);
if (nullptr == bvar) {
continue;
}
std::ostringstream oss;
bvar->describe(oss, options->quote_string);
- std::ostringstream oss_key;
- make_dump_key(oss_key, label_name);
- if (!dumper->dump_mvar(oss_key.str(), oss.str())) {
+ make_dump_key(&key, label_name);
+ if (!dumper->dump_mvar(key, oss.str())) {
continue;
}
n++;
}
return n;
}
+namespace detail {
+// Forwards to another Dumper and counts the metrics that went through, which is
+// how MultiDimension answers with the number of dumped metrics rather than the
+// number of times it called dump_samples().
+class CountingDumper : public Dumper {
+public:
+ explicit CountingDumper(Dumper* dumper) : _dumper(dumper), _count(0) {}
+
+ // Only what the wrapped dumper accepted is counted: a false is a request to
+ // stop, that metric did not make it out.
+ bool dump(const std::string& name, const butil::StringPiece& desc) override {
+ if (!_dumper->dump(name, desc)) {
+ return false;
+ }
+ ++_count;
+ return true;
+ }
+ bool dump_mvar(const std::string& name, const butil::StringPiece& desc) override {
+ if (!_dumper->dump_mvar(name, desc)) {
+ return false;
+ }
+ ++_count;
+ return true;
+ }
+ // A comment describes a family, it is not a metric of its own.
+ bool dump_comment(const std::string& name, const std::string& type) override {
+ return _dumper->dump_comment(name, type);
+ }
+
+ size_t count() const { return _count; }
+
+private:
+ Dumper* _dumper;
+ size_t _count;
+};
+} // namespace detail
+
template <typename T, typename KeyType, bool Shared>
template <typename U>
-typename std::enable_if<butil::is_same<LatencyRecorder, U>::value, size_t>::type
+std::enable_if_t<detail::IsCompositeMetric<U>::value, size_t>
MultiDimension<T, KeyType, Shared>::dump_impl(Dumper* dumper, const DumpOptions*) {
std::vector<key_type> label_names;
list_stats(&label_names);
if (label_names.empty()) {
return 0;
}
- // The latency of one quantile. The quantile must be a fraction to meet
- // prometheus specification, e.g. 0.99 for p99.
- struct LatencyPercentile {
- double quantile;
- int64_t latency;
- };
- // All the values dumped for one label set.
- struct DumpedStats {
- const key_type* label_name;
- LatencyPercentile latency_percentiles[5];
- int64_t avg_latency;
- int64_t max_latency;
- int64_t qps;
- int64_t count;
- };
- // Read all the values in one traversal, so that a LatencyRecorder is looked
- // up only once no matter how many metrics are dumped for it. Keep the values
- // instead of the LatencyRecorder pointers, which delete_stats() may free.
- std::vector<DumpedStats> stats_list;
- stats_list.reserve(label_names.size());
- for (const auto& label_name : label_names) {
- bvar::LatencyRecorder* bvar = get_stats_impl(label_name);
- if (!bvar) {
- continue;
- }
- DumpedStats stats{};
- stats.label_name = &label_name;
- stats.latency_percentiles[0].quantile = FLAGS_bvar_latency_p1 / 100.0;
- stats.latency_percentiles[1].quantile = FLAGS_bvar_latency_p2 / 100.0;
- stats.latency_percentiles[2].quantile = FLAGS_bvar_latency_p3 / 100.0;
- stats.latency_percentiles[3].quantile = 0.999;
- stats.latency_percentiles[4].quantile = 0.9999;
- for (auto& lp : stats.latency_percentiles) {
- lp.latency = bvar->latency_percentile(lp.quantile);
+ const std::vector<MetricFamily>& families = U::list_metric_families();
+ detail::CountingDumper counting_dumper(dumper);
+ std::string family_name;
+ std::string labels;
+ // Families outside, label sets inside.
+ for (size_t f = 0; f < families.size(); ++f) {
+ family_name.assign(this->name()).append(families[f].suffix);
Review Comment:
`MetricFamily::suffix` is nullable according to `collect_metric_family_names`, which explicitly checks for `nullptr`. Appending it directly here invokes the `std::string::append(const char*)` overload with a null pointer for a valid custom composite metric. Build the family name conditionally, as the name-collection helper does.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4061707702
##########
src/brpc/builtin/prometheus_metrics_service.cpp:
##########
@@ -228,19 +311,18 @@ void PrometheusMetricsService::default_method(::google::protobuf::RpcController*
int DumpPrometheusMetricsToIOBuf(butil::IOBuf* output) {
butil::IOBufBuilder os;
PrometheusMetricsDumper dumper(&os, g_server_info_prefix);
- const int ndump = bvar::Variable::dump_exposed(&dumper, nullptr);
+ int ndump = bvar::Variable::dump_exposed(&dumper, nullptr);
if (ndump < 0) {
return -1;
}
os.move_to(*output);
if (bvar::FLAGS_bvar_max_dump_multi_dimension_metric_number > 0) {
PrometheusMetricsDumper dumper_md(&os, g_server_info_prefix);
- const int ndump_md = bvar::MVariableBase::dump_exposed(&dumper_md, nullptr);
- if (ndump_md < 0) {
- return -1;
+ size_t ndump_md = bvar::MVariableBase::dump_exposed(&dumper_md, nullptr);
+ if (ndump_md > 0) {
+ output->append(butil::IOBuf::Movable(os.buf()));
}
Review Comment:
Fixed in 12fa7fd7b52dda902fe527f7ebd9832e35ffbd7b.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4069188451
##########
src/bvar/mvariable.cpp:
##########
@@ -189,6 +198,7 @@ int MVariableBase::expose_impl(const butil::StringPiece& prefix,
if (entry == nullptr) {
entry = &m[_name];
entry->ref = _ref;
Review Comment:
Correct that the two maps stay separate, and that is deliberate. The guarantee now lives in `PrometheusMetricsDumper`, which is the only place that sees every name at once: the bvar pass, the mbvar pass, and the names the exporter itself makes up, such as `_avg_latency` and the summary `_sum`, which no variable is ever exposed under. An expose time registry cannot cover those without duplicating the exporter naming rules inside bvar, and that duplication is exactly what left the gap you reported on [latency_recorder.cpp](https://github.com/apache/brpc/pull/latency_recorder.cpp).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4068116123
##########
src/bvar/histogram.cpp:
##########
@@ -0,0 +1,258 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <algorithm> // std::adjacent_find
+#include <functional> // std::greater_equal
+#include <iterator> // std::next
+#include <string> // std::string
+#include "butil/float_util.h" // butil::IsFinite
+#include "butil/logging.h" // LOG
+#include "butil/strings/string_number_conversions.h" // butil::DoubleToString
+#include "bvar/histogram.h"
+
+namespace bvar {
+
+Histogram::BucketSchema::BucketSchema(std::initializer_list<double> bounds)
+ : _bounds(bounds) {
+ validate_bounds();
+}
+
+Histogram::BucketSchema::BucketSchema(const std::vector<double>& bounds)
+ : _bounds(bounds) {
+ validate_bounds();
+}
+
+void Histogram::BucketSchema::validate_bounds() {
+ for (auto it = _bounds.begin(); it != _bounds.end();) {
+ if (!butil::IsFinite(*it)) {
+ LOG(ERROR) << "Bucket bounds must be finite, dropping " << *it;
+ it = _bounds.erase(it);
+ } else {
+ ++it;
+ }
+ }
+
+ // Drop the bounds that do not keep the sequence strictly ascending.
+ auto it = _bounds.begin();
+ while ((it = std::adjacent_find(it, _bounds.end(),
+ std::greater_equal<>())) != _bounds.end()) {
+ auto bad = std::next(it);
+ LOG(ERROR) << "Bucket bounds must be strictly ascending, dropping "
+ << *bad << " which is not greater than " << *it;
+ _bounds.erase(bad);
+ }
+ if (_bounds.size() >= MAX_HISTOGRAM_BUCKETS) {
+ // Shrinking a vector drops the tail, keeping the tightest bounds.
+ LOG(ERROR) << "A histogram takes at most " << MAX_HISTOGRAM_BUCKETS - 1
+ << " bounds, dropping the "
+ << _bounds.size() - (MAX_HISTOGRAM_BUCKETS - 1)
+ << " ones past the limit";
+ _bounds.resize(MAX_HISTOGRAM_BUCKETS - 1);
+ }
+ if (_bounds.empty()) {
+ // A schema without any bound would send everything into the +Inf
+ // bucket, which no quantile can be read off. Keep the histogram
+ // usable rather than aborting the process over a misconfiguration.
+ LOG(ERROR) << "A histogram needs at least one bound, falling back to 1";
+ _bounds.push_back(1.0);
+ }
+}
+
+std::ostream& operator<<(std::ostream& os, const Histogram::Value& v) {
+ os << "{\"count\":" << v.num << ",\"sum\":"
+ << detail::prometheus_double_to_string(v.sum)
+ << ",\"counts\":[";
+ size_t nbuckets = std::min(v.num_buckets, MAX_HISTOGRAM_BUCKETS);
+ for (size_t i = 0; i < nbuckets; ++i) {
+ if (i != 0) {
+ os << ',';
+ }
+ os << v.counts[i];
+ }
+ return os << "]}";
+}
+
+Histogram::Histogram(const BucketSchema& schema)
+ : _schema(schema)
+ // Both identities carry `num_buckets` so that a value combined out of no
+ // agent at all still knows how wide it is.
+ , _combiner(std::make_shared<combiner_type>(value_type(schema.num_buckets()),
+ value_type(schema.num_buckets())))
+ , _sampler(nullptr) {
+}
+
+Histogram::Histogram(const butil::StringPiece& name, const BucketSchema& schema)
+ : Histogram(schema) {
+ expose(name);
+}
+
+Histogram::Histogram(const butil::StringPiece& prefix,
+ const butil::StringPiece& name,
+ const BucketSchema& schema)
+ : Histogram(schema) {
+ expose_as(prefix, name);
+}
+
+Histogram::~Histogram() {
+ // Calling hide() manually is a MUST required by Variable.
+ hide();
+ if (_sampler != nullptr) {
+ _sampler->destroy();
+ }
+}
+
+Histogram& Histogram::operator<<(double value) {
+ if (BAIDU_UNLIKELY(!butil::IsFinite(value))) {
+ LOG_EVERY_SECOND(WARNING) << "Ignoring non-finite value=" << value
+ << " recorded into Histogram(" << name() << ')';
+ return *this;
+ }
+ agent_type* agent = _combiner->get_or_create_tls_agent();
+ if (BAIDU_UNLIKELY(agent == nullptr)) {
+ LOG(FATAL) << "Fail to create agent";
+ return *this;
+ }
+ // `_schema` outlives the call, the op only borrows it to find the bucket.
+ agent->element.modify(detail::AddSampleToHistogram(&_schema), value);
+ return *this;
+}
+
+Histogram::sampler_type* Histogram::get_sampler() {
+ if (_sampler == nullptr) {
+ _sampler = new sampler_type(this);
+ _sampler->set_debug_name(name());
+ _sampler->schedule();
+ }
+ return _sampler;
+}
+
+int Histogram::expose_impl(const butil::StringPiece& prefix,
+ const butil::StringPiece& name,
+ DisplayFilter display_filter) {
+ int rc = Variable::expose_impl(prefix, name, display_filter);
+ if (rc == 0 && _sampler != nullptr) {
+ _sampler->set_debug_name(this->name());
+ }
+ return rc;
+}
+
+void Histogram::describe(std::ostream& os, bool /*quote_string*/) const {
+ value_type v = get_value();
+ os << "{\"count\":" << v.num
+ << ",\"sum\":" << detail::prometheus_double_to_string(v.sum)
+ << ",\"bounds\":[";
Review Comment:
`describe()` is documented and consumed as JSON, but `prometheus_double_to_string()` emits `+Inf`/`-Inf` when a finite observation sum overflows. Those bare tokens are not valid JSON, so `/vars` becomes unparsable after a legitimate overflow even though the Prometheus exporter needs this spelling. Keep Prometheus formatting confined to `dump_samples()` and serialize non-finite JSON values in a JSON-compatible form (or otherwise define the overflow representation).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright merged PR #3557:
URL: https://github.com/apache/brpc/pull/3557
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3557:
URL: https://github.com/apache/brpc/pull/3557#issuecomment-5771481442
LGTM
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4061707702
##########
src/brpc/builtin/prometheus_metrics_service.cpp:
##########
@@ -228,19 +311,18 @@ void PrometheusMetricsService::default_method(::google::protobuf::RpcController*
int DumpPrometheusMetricsToIOBuf(butil::IOBuf* output) {
butil::IOBufBuilder os;
PrometheusMetricsDumper dumper(&os, g_server_info_prefix);
- const int ndump = bvar::Variable::dump_exposed(&dumper, nullptr);
+ int ndump = bvar::Variable::dump_exposed(&dumper, nullptr);
if (ndump < 0) {
return -1;
}
os.move_to(*output);
if (bvar::FLAGS_bvar_max_dump_multi_dimension_metric_number > 0) {
PrometheusMetricsDumper dumper_md(&os, g_server_info_prefix);
- const int ndump_md = bvar::MVariableBase::dump_exposed(&dumper_md, nullptr);
- if (ndump_md < 0) {
- return -1;
+ size_t ndump_md = bvar::MVariableBase::dump_exposed(&dumper_md, nullptr);
+ if (ndump_md > 0) {
+ output->append(butil::IOBuf::Movable(os.buf()));
}
Review Comment:
Fixed in 12fa7fd7b52dda902fe527f7ebd9832e35ffbd7b .
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4061707702
##########
src/brpc/builtin/prometheus_metrics_service.cpp:
##########
@@ -228,19 +311,18 @@ void PrometheusMetricsService::default_method(::google::protobuf::RpcController*
int DumpPrometheusMetricsToIOBuf(butil::IOBuf* output) {
butil::IOBufBuilder os;
PrometheusMetricsDumper dumper(&os, g_server_info_prefix);
- const int ndump = bvar::Variable::dump_exposed(&dumper, nullptr);
+ int ndump = bvar::Variable::dump_exposed(&dumper, nullptr);
if (ndump < 0) {
return -1;
}
os.move_to(*output);
if (bvar::FLAGS_bvar_max_dump_multi_dimension_metric_number > 0) {
PrometheusMetricsDumper dumper_md(&os, g_server_info_prefix);
- const int ndump_md = bvar::MVariableBase::dump_exposed(&dumper_md, nullptr);
- if (ndump_md < 0) {
- return -1;
+ size_t ndump_md = bvar::MVariableBase::dump_exposed(&dumper_md, nullptr);
+ if (ndump_md > 0) {
+ output->append(butil::IOBuf::Movable(os.buf()));
}
Review Comment:
Fixed in 12fa7fd7b52dda902fe527f7ebd9832e35ffbd7b.
##########
src/brpc/builtin/prometheus_metrics_service.cpp:
##########
@@ -228,19 +311,18 @@ void PrometheusMetricsService::default_method(::google::protobuf::RpcController*
int DumpPrometheusMetricsToIOBuf(butil::IOBuf* output) {
butil::IOBufBuilder os;
PrometheusMetricsDumper dumper(&os, g_server_info_prefix);
- const int ndump = bvar::Variable::dump_exposed(&dumper, nullptr);
+ int ndump = bvar::Variable::dump_exposed(&dumper, nullptr);
if (ndump < 0) {
return -1;
}
os.move_to(*output);
if (bvar::FLAGS_bvar_max_dump_multi_dimension_metric_number > 0) {
PrometheusMetricsDumper dumper_md(&os, g_server_info_prefix);
- const int ndump_md = bvar::MVariableBase::dump_exposed(&dumper_md, nullptr);
- if (ndump_md < 0) {
- return -1;
+ size_t ndump_md = bvar::MVariableBase::dump_exposed(&dumper_md, nullptr);
+ if (ndump_md > 0) {
+ output->append(butil::IOBuf::Movable(os.buf()));
}
Review Comment:
Fixed in 12fa7fd7b52dda902fe527f7ebd9832e35ffbd7b .
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on PR #3557:
URL: https://github.com/apache/brpc/pull/3557#issuecomment-5770409472
> Is this change incompatible with some existing user code? Consider add a flag
@wwbmmm These conflicts are not introduced by this PR. They were already reachable,
Histogram only makes them easy to hit: one Histogram maps to three sample names
(`_bucket`, `_sum`, `_count`) and reserves the `le` label, so names and labels
that used to look free now collide. The PR fixes the underlying problems instead
of working around them for Histogram.
What can now be rejected and was not before:
1. A bvar and an mbvar exposing the same Prometheus name. The two used to keep
separate namespaces, so `/brpc_metrics` could print two different metrics
under one name. They now share one registry and the second expose fails.
Nothing changes among plain bvars, because the Prometheus name of a bvar is
its own name, which `VarMap` already keys on.
2. Exposing an mbvar under a name that is already taken.
`MVariableBase::expose_impl` used to return 0 without exposing anything. It
now returns -1, like `Variable::expose`.
3. The names a `LatencyRecorder` synthesizes for Prometheus (`X`, `X_sum`,
`X_avg_latency`) are reserved in `MethodStatus::Expose`, so a user bvar can
no longer take one of them. `Server::Start` ignores this return value, so no
server fails to start because of it.
4. `dump()` now writes a value only if it is a valid Prometheus number. It used
to skip only values starting with `"`, so a bool gflag (`true`), the json of
a compound `PassiveStatus` (`{...}`) and `-nan` all went out as metric values.
5. A MultiDimension whose label name collides with one its value type reserves.
It logs an error and refuses to expose. Recording is untouched, so
`*md.get_stats({...}) << v` still works and still returns a valid pointer.
None of this rejects anything that used to work. In every case the old behaviour
produced output Prometheus cannot parse, and one bad line fails the entire
scrape, not just that metric. So the real choice is between a metric that
silently never arrives and an error log at expose time.
That is also why I do not think a flag is worth it. To help anyone it would have
to default to the old behaviour, which keeps the broken scrape broken by
default, and what it preserves is not a working metric.
`-bvar_abort_on_same_name` already exists for users who want name conflicts to
be fatal rather than logged.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4062104418
##########
src/bvar/multi_dimension_inl.h:
##########
@@ -224,177 +235,145 @@ bool MultiDimension<T, KeyType, Shared>::has_stats(const K& labels_value) {
template <typename T, typename KeyType, bool Shared>
template <typename U>
-typename std::enable_if<!butil::is_same<LatencyRecorder, U>::value, size_t>::type
+std::enable_if_t<!detail::IsCompositeMetric<U>::value, size_t>
MultiDimension<T, KeyType, Shared>::dump_impl(Dumper* dumper, const DumpOptions* options) {
std::vector<key_type> label_names;
list_stats(&label_names);
if (label_names.empty() || !dumper->dump_comment(this->name(), METRIC_TYPE_GAUGE)) {
return 0;
}
size_t n = 0;
+ std::string key;
for (auto &label_name : label_names) {
value_ptr_type bvar = get_stats_impl(label_name);
if (nullptr == bvar) {
continue;
}
std::ostringstream oss;
bvar->describe(oss, options->quote_string);
- std::ostringstream oss_key;
- make_dump_key(oss_key, label_name);
- if (!dumper->dump_mvar(oss_key.str(), oss.str())) {
+ make_dump_key(&key, label_name);
+ if (!dumper->dump_mvar(key, oss.str())) {
continue;
}
n++;
}
return n;
}
+namespace detail {
+// Forwards to another Dumper and counts the metrics that went through, which is
+// how MultiDimension answers with the number of dumped metrics rather than the
+// number of times it called dump_samples().
+class CountingDumper : public Dumper {
+public:
+ explicit CountingDumper(Dumper* dumper) : _dumper(dumper), _count(0) {}
+
+ // Only what the wrapped dumper accepted is counted: a false is a request to
+ // stop, that metric did not make it out.
+ bool dump(const std::string& name, const butil::StringPiece& desc) override {
+ if (!_dumper->dump(name, desc)) {
+ return false;
+ }
+ ++_count;
+ return true;
+ }
+ bool dump_mvar(const std::string& name, const butil::StringPiece& desc) override {
+ if (!_dumper->dump_mvar(name, desc)) {
+ return false;
+ }
+ ++_count;
+ return true;
+ }
+ // A comment describes a family, it is not a metric of its own.
+ bool dump_comment(const std::string& name, const std::string& type) override {
+ return _dumper->dump_comment(name, type);
+ }
+
+ size_t count() const { return _count; }
+
+private:
+ Dumper* _dumper;
+ size_t _count;
+};
+} // namespace detail
+
template <typename T, typename KeyType, bool Shared>
template <typename U>
-typename std::enable_if<butil::is_same<LatencyRecorder, U>::value, size_t>::type
+std::enable_if_t<detail::IsCompositeMetric<U>::value, size_t>
MultiDimension<T, KeyType, Shared>::dump_impl(Dumper* dumper, const DumpOptions*) {
std::vector<key_type> label_names;
list_stats(&label_names);
if (label_names.empty()) {
return 0;
}
- // The latency of one quantile. The quantile must be a fraction to meet
- // prometheus specification, e.g. 0.99 for p99.
- struct LatencyPercentile {
- double quantile;
- int64_t latency;
- };
- // All the values dumped for one label set.
- struct DumpedStats {
- const key_type* label_name;
- LatencyPercentile latency_percentiles[5];
- int64_t avg_latency;
- int64_t max_latency;
- int64_t qps;
- int64_t count;
- };
- // Read all the values in one traversal, so that a LatencyRecorder is looked
- // up only once no matter how many metrics are dumped for it. Keep the values
- // instead of the LatencyRecorder pointers, which delete_stats() may free.
- std::vector<DumpedStats> stats_list;
- stats_list.reserve(label_names.size());
- for (const auto& label_name : label_names) {
- bvar::LatencyRecorder* bvar = get_stats_impl(label_name);
- if (!bvar) {
- continue;
+ const std::vector<MetricFamily>& families = U::list_metric_families();
+ detail::CountingDumper counting_dumper(dumper);
+ std::string family_name;
+ std::string labels;
+ // Families outside, label sets inside.
+ for (size_t f = 0; f < families.size(); ++f) {
+ // suffix is nullable, as collect_metric_family_names() knows.
+ family_name.assign(this->name());
+ if (families[f].suffix != nullptr) {
+ family_name.append(families[f].suffix);
}
- DumpedStats stats{};
- stats.label_name = &label_name;
- stats.latency_percentiles[0].quantile = FLAGS_bvar_latency_p1 / 100.0;
- stats.latency_percentiles[1].quantile = FLAGS_bvar_latency_p2 / 100.0;
- stats.latency_percentiles[2].quantile = FLAGS_bvar_latency_p3 / 100.0;
- stats.latency_percentiles[3].quantile = 0.999;
- stats.latency_percentiles[4].quantile = 0.9999;
- for (auto& lp : stats.latency_percentiles) {
- lp.latency = bvar->latency_percentile(lp.quantile);
+ // One TYPE line per family, ahead of all its samples.
+ if (!counting_dumper.dump_comment(family_name, families[f].type)) {
+ break;
}
- stats.avg_latency = bvar->latency();
- stats.max_latency = bvar->max_latency();
- stats.qps = bvar->qps();
- stats.count = bvar->count();
- stats_list.push_back(stats);
- }
-
- size_t n = 0;
-
- // To meet prometheus specification, we must guarantee no second TYPE line for one metric name
-
- // latency comment
- dumper->dump_comment(this->name() + "_latency", METRIC_TYPE_GAUGE);
- for (const auto& stats : stats_list) {
- for (const auto& lp : stats.latency_percentiles) {
- std::ostringstream oss_latency_key;
- make_dump_key(oss_latency_key, *stats.label_name, "_latency", lp.quantile);
- if (dumper->dump_mvar(oss_latency_key.str(), std::to_string(lp.latency))) {
- n++;
+ for (const auto& label_name : label_names) {
+ value_ptr_type bvar = get_stats_impl(label_name);
+ if (bvar == nullptr) {
+ continue;
+ }
+ labels.clear();
+ append_labels_kvpair_body(&labels, label_name);
+ // A false asks to stop dumping, as Dumper::dump() does. Going on
+ // would write samples under a family whose TYPE line the dumper
+ // has already given up on.
+ if (!bvar->dump_samples(&counting_dumper, f, family_name, labels)) {
Review Comment:
This is not a regression introduced by this change.
The lifetime contract is documented in [multi_dimension.h](http://localhost:54508/api/v1/resource/extension/i-1789897269253-06186460327962506/3/Library/Application%20Support/JetBrains/CLion2025.3/plugins/codewiz-idea-plugin/server_lib/.rcs-buildin/rednote-codewiz/0.10.2/src/bvar/multi_dimension.h:55-58):
If Shared' is false, get_stats' returns a raw pointer, delete_stats' and clear_stats' are not thread safe.
If Shared' is true, get_statsreturns a shared_ptr,delete_stats' and `clear_stats' are thread safe.
So concurrent dump + delete_stats under Shared=false is a documented user error, not a bug. The pre-existing non-composite [dump_impl](http://localhost:54508/api/v1/resource/extension/i-1789897269253-06186460327962506/3/Library/Application%20Support/JetBrains/CLion2025.3/plugins/codewiz-idea-plugin/server_lib/.rcs-buildin/rednote-codewiz/0.10.2/src/bvar/multi_dimension_inl.h:238-261) follows the exact same pattern — get_stats_impl() then dereference — so the composite path is consistent with the established behavior, not a new risk.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4068105002
##########
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:
Fixed in 3d8fa67c0c5bfdec639989fd1ebc0f005a4a4c45 .
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4069208581
##########
src/bvar/variable.cpp:
##########
@@ -179,7 +201,14 @@ int Variable::expose_impl(const butil::StringPiece& prefix,
}
}
to_underscored_name(&_name, name);
-
+
+ bool expose_succeeded = false;
+ BUTIL_SCOPE_EXIT {
+ if (!expose_succeeded) {
+ _name.clear();
+ }
+ };
+
VarMapWithLock& m = get_var_map(_name);
Review Comment:
The rollback is not there to release a reservation. `_name` is built before the map insert, so every failure return past that point has to put it back, and in `MVariableBase::expose_impl()` the metric count cap returns early and used to leave the mvar carrying a name it was never exposed under. `Variable::expose_impl()` already did this with an explicit clear at the end of the conflict path, the guard only makes it cover any later early return too.
The cross bvar and mbvar naming guarantee is deliberately not here, see the reply on [mvariable.cpp](https://github.com/apache/brpc/pull/mvariable.cpp). It lives in PrometheusMetricsDumper, which is the only place that sees both passes and the synthesized names together, and keeping it out of expose() is what makes this PR compatible with code that exposes such a pair today.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3557:
URL: https://github.com/apache/brpc/pull/3557#discussion_r4069150559
##########
src/bvar/mvariable.cpp:
##########
@@ -189,6 +198,7 @@ int MVariableBase::expose_impl(const butil::StringPiece& prefix,
if (entry == nullptr) {
entry = &m[_name];
entry->ref = _ref;
Review Comment:
This still checks collisions only in the mvariable map; `Variable::expose_impl` performs the equivalent lookup in a separate bvar map, so a bvar and an mvar such as `foo.bar` and `foo_bar` can both expose successfully and emit the same normalized Prometheus name with potentially different types/labels. The PR's cross-bvar/mbvar collision guarantee is therefore not implemented; both expose paths need to claim the same normalized-name registry (and release it on hide).
##########
src/bvar/variable.cpp:
##########
@@ -179,7 +201,14 @@ int Variable::expose_impl(const butil::StringPiece& prefix,
}
}
to_underscored_name(&_name, name);
-
+
+ bool expose_succeeded = false;
+ BUTIL_SCOPE_EXIT {
+ if (!expose_succeeded) {
+ _name.clear();
+ }
+ };
+
VarMapWithLock& m = get_var_map(_name);
Review Comment:
The new rollback only clears `_name`; it does not reserve the normalized name in a registry shared with `MVariableBase`. `Variable::expose_impl()` still inserts only into `get_var_map()`, while mbvars insert into a separate map, so a bvar and an mbvar can both successfully expose the same normalized Prometheus name (for example `foo.bar` and `foo::bar`). This leaves the stated cross bvar/mbvar collision problem unresolved outside the scrape-time exporter workaround; coordinate both expose/hide paths through one shared reservation mechanism.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org