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