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