Skip to content

Commit

Permalink
prometheus: revert the condition for enabling aggregation
Browse files Browse the repository at this point in the history
in d7e16bb, we added the query time configuration for disabling
aggregation, but the condition for disabling aggregation was wrong.
before that change, we always enable the aggregation, but after that
change, the aggregation is disabled when the value of query parameter
of "__aggregate__" is not "false". in other words,

- if this query parameter is not set, the aggregation is disabled. this
  changes the existing behavior.
- if this query parameter's value is "false", the aggregation is
  enabeld. this is not the expected behavior.

so, in this change

- change the parameter from `disable_aggregation` to
  `enable_aggregation`. as the positive condition is
  always easier to understand.
- and update the handling accodingly.

Fixes d7e16bb
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>

Closes #2232
  • Loading branch information
tchaikov authored and nyh committed May 9, 2024
1 parent d3657ec commit 42f15a5
Showing 1 changed file with 13 additions and 13 deletions.
26 changes: 13 additions & 13 deletions src/core/prometheus.cc
Original file line number Diff line number Diff line change
Expand Up @@ -735,15 +735,15 @@ std::string get_value_as_string(std::stringstream& s, const mi::metric_value& va
return value_str;
}

future<> write_text_representation(output_stream<char>& out, const config& ctx, const metric_family_range& m, bool show_help, bool disable_aggregation, std::function<bool(const mi::labels_type&)> filter) {
return seastar::async([&ctx, &out, &m, show_help, disable_aggregation, filter] () mutable {
future<> write_text_representation(output_stream<char>& out, const config& ctx, const metric_family_range& m, bool show_help, bool enable_aggregation, std::function<bool(const mi::labels_type&)> filter) {
return seastar::async([&ctx, &out, &m, show_help, enable_aggregation, filter] () mutable {
bool found = false;
std::stringstream s;
for (metric_family& metric_family : m) {
auto name = ctx.prefix + "_" + metric_family.name();
found = false;
metric_aggregate_by_labels aggregated_values(metric_family.metadata().aggregate_labels);
bool should_aggregate = !disable_aggregation && !metric_family.metadata().aggregate_labels.empty();
bool should_aggregate = enable_aggregation && !metric_family.metadata().aggregate_labels.empty();
metric_family.foreach_metric([&s, &out, &ctx, &found, &name, &metric_family, &aggregated_values, should_aggregate, show_help, &filter](const mi::metric_value& value, const mi::metric_info& value_info) mutable {
s.clear();
s.str("");
Expand Down Expand Up @@ -788,12 +788,12 @@ future<> write_text_representation(output_stream<char>& out, const config& ctx,
});
}

future<> write_protobuf_representation(output_stream<char>& out, const config& ctx, metric_family_range& m, bool disable_aggregation, std::function<bool(const mi::labels_type&)> filter) {
return do_for_each(m, [&ctx, &out, disable_aggregation, filter=std::move(filter)](metric_family& metric_family) mutable {
future<> write_protobuf_representation(output_stream<char>& out, const config& ctx, metric_family_range& m, bool enable_aggregation, std::function<bool(const mi::labels_type&)> filter) {
return do_for_each(m, [&ctx, &out, enable_aggregation, filter=std::move(filter)](metric_family& metric_family) mutable {
std::string s;
google::protobuf::io::StringOutputStream os(&s);
metric_aggregate_by_labels aggregated_values(metric_family.metadata().aggregate_labels);
bool should_aggregate = !disable_aggregation && !metric_family.metadata().aggregate_labels.empty();
bool should_aggregate = enable_aggregation && !metric_family.metadata().aggregate_labels.empty();
auto& name = metric_family.name();
pm::MetricFamily mtf;
bool empty_metric = true;
Expand Down Expand Up @@ -896,16 +896,16 @@ class metrics_handler : public httpd::handler_base {
sstring metric_family_name = req->get_query_param("__name__");
bool prefix = trim_asterisk(metric_family_name);
bool show_help = req->get_query_param("__help__") != "false";
bool disable_aggregation = req->get_query_param("__aggregate__") != "false";
bool enable_aggregation = req->get_query_param("__aggregate__") != "false";
std::function<bool(const mi::labels_type&)> filter = make_filter(*req);
rep->write_body(is_protobuf_format ? "proto" : "txt", [this, is_protobuf_format, metric_family_name, prefix, show_help, disable_aggregation, filter] (output_stream<char>&& s) {
rep->write_body(is_protobuf_format ? "proto" : "txt", [this, is_protobuf_format, metric_family_name, prefix, show_help, enable_aggregation, filter] (output_stream<char>&& s) {
return do_with(metrics_families_per_shard(), output_stream<char>(std::move(s)),
[this, is_protobuf_format, prefix, &metric_family_name, show_help, disable_aggregation, filter] (metrics_families_per_shard& families, output_stream<char>& s) mutable {
return get_map_value(families).then([&s, &families, this, is_protobuf_format, prefix, &metric_family_name, show_help, disable_aggregation, filter]() mutable {
[this, is_protobuf_format, prefix, &metric_family_name, show_help, enable_aggregation, filter] (metrics_families_per_shard& families, output_stream<char>& s) mutable {
return get_map_value(families).then([&s, &families, this, is_protobuf_format, prefix, &metric_family_name, show_help, enable_aggregation, filter]() mutable {
return do_with(get_range(families, metric_family_name, prefix),
[&s, this, is_protobuf_format, show_help, disable_aggregation, filter](metric_family_range& m) {
return (is_protobuf_format) ? write_protobuf_representation(s, _ctx, m, disable_aggregation, filter) :
write_text_representation(s, _ctx, m, show_help, disable_aggregation, filter);
[&s, this, is_protobuf_format, show_help, enable_aggregation, filter](metric_family_range& m) {
return (is_protobuf_format) ? write_protobuf_representation(s, _ctx, m, enable_aggregation, filter) :
write_text_representation(s, _ctx, m, show_help, enable_aggregation, filter);
});
}).finally([&s] () mutable {
return s.close();
Expand Down

0 comments on commit 42f15a5

Please sign in to comment.