(kudu) branch master updated: [metrics] add --metrics_prometheus_default_metrics for a server-side allowlist

[al...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

asf-gitbox-commits pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git


The following commit(s) were added to refs/heads/master by this push:
     new ee47a9ca1 [metrics] add --metrics_prometheus_default_metrics for a server-side allowlist
ee47a9ca1 is described below

commit ee47a9ca1939200e227f9bf266c97a7c0eed3ccc
Author: Yan-Daojiang <ya...@163.com>
AuthorDate: Thu Sep 10 18:09:18 2026 +0800

    [metrics] add --metrics_prometheus_default_metrics for a server-side allowlist
    
    The /metrics_prometheus endpoint already honors a per-request 'metrics'
    allowlist (a case-insensitive substring match), but there is no server-side
    default: only severity has one, via --metrics_default_level. As a result, any
    scrape that omits the 'metrics' parameter -- an ad-hoc curl, a one-off script,
    or a misconfigured collector -- receives the full metric set, and the allowlist
    must be carried in every scrape URL. That URL channel is bounded by the ~16KB
    request buffer and is prone to silent degradation: if the value collapses to no
    usable token, filtering is disabled and the full output is returned.
    
    Add --metrics_prometheus_default_metrics, mirroring the existing
    --metrics_prometheus_default_merge_rules / --metrics_prometheus_default_quantiles
    family: a request's own 'metrics' parameter takes precedence over the flag
    default, which is applied only when the request carries none; an empty value
    (the default) preserves the current behavior. This turns the allowlist from a
    per-request convention into a server-side policy that covers parameter-less
    scrapes as well.
    
    Also simplify the 'export_all' check in Histogram::WriteAsPrometheus() per
    a review suggestion from Change-Id: I01b48aa.
    
    Change-Id: I70ef54eef456108f8503a4edfdfb68fab3173353
    Reviewed-on: http://gerrit.cloudera.org:8080/24836
    Tested-by: Alexey Serbin <al...@apache.org>
    Reviewed-by: Alexey Serbin <al...@apache.org>
---
 src/kudu/server/default_path_handlers.cc |  6 +++
 src/kudu/tserver/tablet_server-test.cc   | 31 +++++++++++++++
 src/kudu/util/metrics-test.cc            | 68 ++++++++++++++++++++++++++++++++
 src/kudu/util/metrics.cc                 | 65 ++++++++++++++++++++++++++++--
 src/kudu/util/metrics.h                  |  7 ++++
 5 files changed, 173 insertions(+), 4 deletions(-)

diff --git a/src/kudu/server/default_path_handlers.cc b/src/kudu/server/default_path_handlers.cc
index 86bb8ad04..4e09c9c45 100644
--- a/src/kudu/server/default_path_handlers.cc
+++ b/src/kudu/server/default_path_handlers.cc
@@ -552,6 +552,12 @@ static void WriteMetricsAsPrometheus(const MetricRegistry* const metrics,
   // default configured via --metrics_prometheus_default_quantiles.
   GetPrometheusQuantiles(ParseArray(req.parsed_args, kQuantilesParam), &opts.quantiles);
 
+  // A 'metrics' query parameter (already parsed into 'entity_metrics' above)
+  // takes precedence over the server-side default configured via
+  // --metrics_prometheus_default_metrics, which restricts even parameter-less
+  // scrapes to the configured allowlist.
+  GetPrometheusMetricsFilter(&opts.filters.entity_metrics);
+
   // The hostname label is emitted either in the label-based non-merged format,
   // or whenever merging is active (merged output is always label-based).
   if (FLAGS_metrics_prometheus_use_entity_labels || !opts.merge_rules.empty()) {
diff --git a/src/kudu/tserver/tablet_server-test.cc b/src/kudu/tserver/tablet_server-test.cc
index 6a3b087c6..c1dc57523 100644
--- a/src/kudu/tserver/tablet_server-test.cc
+++ b/src/kudu/tserver/tablet_server-test.cc
@@ -219,6 +219,7 @@ DECLARE_string(block_manager);
 DECLARE_string(env_inject_eio_globs);
 DECLARE_string(env_inject_full_globs);
 DECLARE_string(metrics_default_level);
+DECLARE_string(metrics_prometheus_default_metrics);
 DECLARE_string(webserver_doc_root);
 DECLARE_uint32(tablet_apply_pool_overload_threshold_ms);
 
@@ -4623,6 +4624,36 @@ TEST_F(TabletServerTest, PrometheusMetricsNameFiltering) {
   }
 }
 
+// Verify that --metrics_prometheus_default_metrics applies the allowlist to a
+// parameter-less scrape, and that a ?metrics= query parameter overrides it.
+TEST_F(TabletServerTest, PrometheusDefaultMetricsFlag) {
+  google::FlagSaver saver;
+  FLAGS_metrics_prometheus_default_metrics = "raft_term";
+
+  const string base_url = Substitute("http://$0/metrics_prometheus",
+                                     mini_server_->bound_http_addr().ToString());
+  {
+    // No query parameter: the flag default restricts the output to raft_term.
+    EasyCurl c;
+    faststring buf;
+    ASSERT_OK(c.FetchURL(base_url, &buf));
+    const auto& str = buf.ToString();
+    NO_FATALS(CheckPrometheusOutput(str));
+    ASSERT_STR_MATCHES(str, "raft_term ");
+    ASSERT_STR_NOT_MATCHES(str, "threads_running ");
+  }
+  {
+    // ?metrics=threads_running overrides the flag default for this request.
+    EasyCurl c;
+    faststring buf;
+    ASSERT_OK(c.FetchURL(base_url + "?metrics=threads_running", &buf));
+    const auto& str = buf.ToString();
+    NO_FATALS(CheckPrometheusOutput(str));
+    ASSERT_STR_MATCHES(str, "threads_running ");
+    ASSERT_STR_NOT_MATCHES(str, "raft_term ");
+  }
+}
+
 // Verify that ?ids= filters the output to only entities with the given ID.
 TEST_F(TabletServerTest, PrometheusMetricsIdFiltering) {
   const string base_url = Substitute("http://$0/metrics_prometheus",
diff --git a/src/kudu/util/metrics-test.cc b/src/kudu/util/metrics-test.cc
index 7a762c33c..54a918cda 100644
--- a/src/kudu/util/metrics-test.cc
+++ b/src/kudu/util/metrics-test.cc
@@ -62,6 +62,7 @@ DECLARE_bool(metrics_prometheus_use_entity_labels);
 DECLARE_bool(metrics_prometheus_export_hostname);
 DECLARE_string(metrics_prometheus_default_merge_rules);
 DECLARE_string(metrics_prometheus_default_quantiles);
+DECLARE_string(metrics_prometheus_default_metrics);
 
 namespace kudu {
 
@@ -550,6 +551,73 @@ TEST_F(MetricsTest, PrometheusMalformedDefaultMergeRulesFlagTest) {
   ASSERT_TRUE(ContainsKey(rules, "tablet"));
 }
 
+// The server-wide --metrics_prometheus_default_metrics allowlist is applied
+// when a request carries no 'metrics' filter of its own.
+TEST_F(MetricsTest, PrometheusDefaultMetricsFromFlagTest) {
+  google::FlagSaver saver;
+  FLAGS_metrics_prometheus_default_metrics = "test_counter";
+
+  // No per-request allowlist: the flag default seeds the filter.
+  vector<string> entity_metrics;
+  GetPrometheusMetricsFilter(&entity_metrics);
+  ASSERT_EQ(vector<string>{"test_counter"}, entity_metrics);
+}
+
+// A request's own 'metrics' allowlist takes precedence over the flag default.
+TEST_F(MetricsTest, PrometheusRequestMetricsOverrideFlagTest) {
+  google::FlagSaver saver;
+  FLAGS_metrics_prometheus_default_metrics = "test_counter";
+
+  // The request-supplied allowlist is left untouched by the flag default.
+  vector<string> entity_metrics = {"raft_term"};
+  GetPrometheusMetricsFilter(&entity_metrics);
+  ASSERT_EQ(vector<string>{"raft_term"}, entity_metrics);
+}
+
+// A --metrics_prometheus_default_metrics value with no usable token (e.g. only
+// commas) yields an empty allowlist, so no metric name filtering is performed.
+TEST_F(MetricsTest, PrometheusNoUsableDefaultMetricsFlagTest) {
+  google::FlagSaver saver;
+
+  // Empty flag and empty request: nothing to filter by.
+  FLAGS_metrics_prometheus_default_metrics = "";
+  vector<string> entity_metrics;
+  GetPrometheusMetricsFilter(&entity_metrics);
+  ASSERT_TRUE(entity_metrics.empty());
+
+  // Only commas: SplitStringUsing() skips the empty tokens, so the allowlist
+  // stays empty rather than degrading to an all-matching empty substring.
+  FLAGS_metrics_prometheus_default_metrics = ",,";
+  entity_metrics.clear();
+  GetPrometheusMetricsFilter(&entity_metrics);
+  ASSERT_TRUE(entity_metrics.empty());
+}
+
+// The server-wide --metrics_prometheus_default_metrics allowlist restricts the
+// exported metrics end-to-end when a request carries no 'metrics' filter.
+TEST_F(MetricsTest, PrometheusDefaultMetricsFilterAppliedTest) {
+  google::FlagSaver saver;
+  FLAGS_metrics_prometheus_default_metrics = "tablet_test_counter";
+
+  MetricRegistry registry;
+  auto tablet = METRIC_ENTITY_tablet.Instantiate(&registry, "tablet-1");
+  METRIC_tablet_test_counter.Instantiate(tablet)->IncrementBy(7);
+  auto table = METRIC_ENTITY_table.Instantiate(&registry, "table-1");
+  METRIC_table_test_counter.Instantiate(table)->IncrementBy(9);
+
+  // No per-request allowlist: the flag default restricts the output.
+  MetricPrometheusOptions opts;
+  GetPrometheusMetricsFilter(&opts.filters.entity_metrics);
+
+  ostringstream output;
+  PrometheusWriter writer(&output);
+  ASSERT_OK(registry.WriteAsPrometheus(&writer, opts));
+
+  const auto& out = output.str();
+  ASSERT_STR_CONTAINS(out, "tablet_test_counter");
+  ASSERT_STR_NOT_CONTAINS(out, "table_test_counter");
+}
+
 TEST_F(MetricsTest, CounterPrometheusTest) {
   scoped_refptr<Counter> requests(new Counter(&METRIC_test_counter));
 
diff --git a/src/kudu/util/metrics.cc b/src/kudu/util/metrics.cc
index 4d45cc709..d8cacc223 100644
--- a/src/kudu/util/metrics.cc
+++ b/src/kudu/util/metrics.cc
@@ -182,6 +182,46 @@ DEFINE_validator(metrics_prometheus_default_quantiles,
   return true;
 });
 
+DEFINE_string(metrics_prometheus_default_metrics, "",
+              "The default metric name allowlist applied by the "
+              "'/metrics_prometheus' endpoint when the request does not carry a "
+              "'metrics' query parameter. The value is a comma-separated list of "
+              "case-insensitive substrings; a metric is exported when its name "
+              "contains at least one of them, matching the semantics of the "
+              "'metrics' query parameter. For example, "
+              "'connections_accepted,raft_term' restricts the output to metrics "
+              "whose names contain either substring. This applies the allowlist "
+              "to every scrape, including parameter-less ones, rather than "
+              "relying on each consumer to pass 'metrics'. Empty by default, "
+              "i.e. no metric name filtering is performed unless a request asks "
+              "for it.");
+TAG_FLAG(metrics_prometheus_default_metrics, advanced);
+TAG_FLAG(metrics_prometheus_default_metrics, runtime);
+TAG_FLAG(metrics_prometheus_default_metrics, evolving);
+DEFINE_validator(metrics_prometheus_default_metrics,
+                 [](const char* flag_name, const string& value) {
+  // An empty value disables default filtering and is always valid. A metric
+  // name substring has no internal structure to validate, but warn about (but
+  // tolerate) a non-empty value that yields no usable token (e.g. only commas),
+  // which would silently export every metric instead of restricting the
+  // output; see GetPrometheusMetricsFilter(). Only this zero-token case is
+  // guarded: a value that yields a whitespace-only token (e.g. " " or "a, ")
+  // survives SplitStringUsing(), matches no metric name, and produces the
+  // opposite silent failure -- an empty scrape -- which is left unguarded here
+  // to match the 'metrics' query parameter and the sibling default flags.
+  if (value.empty()) {
+    return true;
+  }
+  vector<string> tokens;
+  SplitStringUsing(value, ",", &tokens);
+  if (tokens.empty()) {
+    LOG(WARNING) << Substitute(
+        "--$0 is set to '$1' but contains no usable metric name; no metric "
+        "name filtering will be applied", flag_name, value);
+  }
+  return true;
+});
+
 // Process/server-wide metrics should go into the 'server' entity.
 // More complex applications will define other entities.
 METRIC_DEFINE_entity(server);
@@ -342,6 +382,18 @@ void GetPrometheusQuantiles(const vector<string>& request_quantiles,
   }
 }
 
+void GetPrometheusMetricsFilter(vector<string>* entity_metrics) {
+  // A request-supplied 'metrics' allowlist (already parsed into
+  // 'entity_metrics') takes precedence over the server-side default. The value
+  // is used verbatim by MatchNameInList(), so no further parsing is needed.
+  if (!entity_metrics->empty()) {
+    return;
+  }
+  if (!FLAGS_metrics_prometheus_default_metrics.empty()) {
+    SplitStringUsing(FLAGS_metrics_prometheus_default_metrics, ",", entity_metrics);
+  }
+}
+
 //
 // MetricUnit
 //
@@ -1476,11 +1528,16 @@ Status Histogram::WriteAsPrometheus(PrometheusWriter* writer,
   const char* const unit = MetricUnit::Name(prototype_->unit());
   DCHECK(unit);
 
-  // An all-empty selection exports every quantile (the default). This is
-  // loop-invariant, so evaluate it once here rather than per quantile line.
+  // An all-empty selection exports every quantile (the default). The selection
+  // is filled contiguously from the first slot in canonical order (see
+  // ParseQuantiles()), so an empty first slot means nothing was selected. This
+  // is loop-invariant, so evaluate it once here rather than per quantile line.
   const auto& quantiles = opts.quantiles;
-  const bool export_all = std::all_of(quantiles.begin(), quantiles.end(),
-                                      [](const char* q) { return q == nullptr; });
+  const bool export_all = (quantiles[0] == nullptr);
+  // Guard the contiguous-fill invariant the shortcut above relies on: an empty
+  // first slot must imply an all-empty selection.
+  DCHECK(!export_all || std::all_of(quantiles.begin(), quantiles.end(),
+                                    [](const char* q) { return q == nullptr; }));
 
   // A snapshot is taken to have more consistent statistics while generating
   // the output.
diff --git a/src/kudu/util/metrics.h b/src/kudu/util/metrics.h
index 57e223ac2..99dc7219e 100644
--- a/src/kudu/util/metrics.h
+++ b/src/kudu/util/metrics.h
@@ -505,6 +505,13 @@ typedef std::array<const char*, kNumHistogramQuantiles> HistogramQuantiles;
 void GetPrometheusQuantiles(const std::vector<std::string>& request_quantiles,
                             HistogramQuantiles* quantiles);
 
+// Apply the server-side default metric name allowlist for a /metrics_prometheus
+// request to 'entity_metrics'. A request-supplied 'metrics' allowlist (already
+// parsed into 'entity_metrics') takes precedence; the server-wide default
+// configured via --metrics_prometheus_default_metrics is applied only when
+// 'entity_metrics' is empty. An empty result means "export every metric".
+void GetPrometheusMetricsFilter(std::vector<std::string>* entity_metrics);
+
 struct MetricJsonOptions {
   MetricJsonOptions() :
     include_raw_histograms(false),