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: [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]
