diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 4498f1170b362..0860d37225e62 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -34,6 +34,17 @@ minor_behavior_changes: Optimized prometheus stats endpoint. Users should see a roughly 30-40% latency improvement in calls to the endpoint for cases where the scrape results in lots of cluster stats. There should be no visible changes to users, or incompatibilities. +- area: stats + change: | + The prometheus formatter now strips a registered custom stat namespace (e.g. ``wasmcustom``) when + it appears as a non-leading, non-trailing segment of a tag-extracted stat name, so that custom + metrics created on a non-root scope render with the standard ``envoy_`` prefix and without the + internal namespace. For example an upstream Wasm counter that previously rendered as + ``envoy_cluster_wasmcustom_upstream_rq_2xx`` now renders as ``envoy_cluster_upstream_rq_2xx``, + matching listener Wasm and native cluster metrics. The cluster_name (and other extracted) labels + are preserved. This primarily fixes upstream HTTP filter Wasm metrics emitted under cluster + scope. This behavioral change can be temporarily reverted by setting runtime guard + ``envoy.reloadable_features.strip_scoped_custom_stat_namespace`` to ``false``. - area: tls change: | ``SslSocket`` now reports ``ECONNRESET`` as ``ConnectionReset`` by reading the system error diff --git a/envoy/stats/custom_stat_namespaces.h b/envoy/stats/custom_stat_namespaces.h index 175a80efe6b7a..880574fc44c2c 100644 --- a/envoy/stats/custom_stat_namespaces.h +++ b/envoy/stats/custom_stat_namespaces.h @@ -1,5 +1,7 @@ #pragma once +#include + #include "envoy/common/pure.h" #include "absl/strings/string_view.h" @@ -46,6 +48,15 @@ class CustomStatNamespaces { */ virtual absl::optional stripRegisteredPrefix(const absl::string_view stat_name) const PURE; + + /** + * Like stripRegisteredPrefix, but for a registered namespace appearing as a non-leading, + * non-trailing segment of the dotted stat name (e.g. "cluster.wasmcustom."). Returns + * std::string rather than string_view because the result spans both sides of the removed + * segment. + */ + virtual absl::optional + stripRegisteredInnerNamespace(absl::string_view stat_name) const PURE; }; } // namespace Stats diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index cb461bd701b33..0cb4f3503bed6 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -102,6 +102,7 @@ RUNTIME_GUARD(envoy_reloadable_features_skip_dns_lookup_for_proxied_requests); RUNTIME_GUARD(envoy_reloadable_features_skip_pending_overflow_count_on_active_rq); RUNTIME_GUARD(envoy_reloadable_features_ssl_socket_report_connection_reset); RUNTIME_GUARD(envoy_reloadable_features_strict_stats_matcher_unpacked); +RUNTIME_GUARD(envoy_reloadable_features_strip_scoped_custom_stat_namespace); RUNTIME_GUARD(envoy_reloadable_features_tcp_proxy_odcds_over_ads_fix); RUNTIME_GUARD(envoy_reloadable_features_test_feature_true); RUNTIME_GUARD(envoy_reloadable_features_tls_certificate_compression_brotli); diff --git a/source/common/stats/custom_stat_namespaces_impl.cc b/source/common/stats/custom_stat_namespaces_impl.cc index f5acb34ebcfbe..b6211dcfc3b58 100644 --- a/source/common/stats/custom_stat_namespaces_impl.cc +++ b/source/common/stats/custom_stat_namespaces_impl.cc @@ -3,6 +3,8 @@ #include "source/common/common/assert.h" #include "source/common/common/thread.h" +#include "absl/strings/str_cat.h" + namespace Envoy { namespace Stats { @@ -29,5 +31,32 @@ CustomStatNamespacesImpl::stripRegisteredPrefix(const absl::string_view stat_nam return absl::nullopt; }; +absl::optional +CustomStatNamespacesImpl::stripRegisteredInnerNamespace(absl::string_view stat_name) const { + ASSERT_IS_MAIN_OR_TEST_THREAD(); + if (namespaces_.empty()) { + return absl::nullopt; + } + + // Skip the leading scope segment, scan inner segments left-to-right, stop before the leaf. + size_t segment_start = stat_name.find('.'); + if (segment_start == absl::string_view::npos) { + return absl::nullopt; + } + ++segment_start; + + while (segment_start < stat_name.size()) { + const size_t segment_end = stat_name.find('.', segment_start); + if (segment_end == absl::string_view::npos) { + return absl::nullopt; + } + if (registered(stat_name.substr(segment_start, segment_end - segment_start))) { + return absl::StrCat(stat_name.substr(0, segment_start), stat_name.substr(segment_end + 1)); + } + segment_start = segment_end + 1; + } + return absl::nullopt; +} + } // namespace Stats } // namespace Envoy diff --git a/source/common/stats/custom_stat_namespaces_impl.h b/source/common/stats/custom_stat_namespaces_impl.h index 0f90b28d5ba43..0a204e963d4ae 100644 --- a/source/common/stats/custom_stat_namespaces_impl.h +++ b/source/common/stats/custom_stat_namespaces_impl.h @@ -16,6 +16,8 @@ class CustomStatNamespacesImpl : public CustomStatNamespaces { void registerStatNamespace(const absl::string_view name) override; absl::optional stripRegisteredPrefix(const absl::string_view stat_name) const override; + absl::optional + stripRegisteredInnerNamespace(absl::string_view stat_name) const override; private: absl::flat_hash_set namespaces_; diff --git a/source/server/admin/BUILD b/source/server/admin/BUILD index 2e6603cf6cee3..f7ba5d594ca48 100644 --- a/source/server/admin/BUILD +++ b/source/server/admin/BUILD @@ -224,6 +224,7 @@ envoy_cc_library( ":utils_lib", "//envoy/stats:custom_stat_namespaces_interface", "//source/common/buffer:buffer_lib", + "//source/common/runtime:runtime_lib", "//source/common/stats:histogram_lib", "//source/common/upstream:host_utility_lib", "@prometheus_metrics_model//:client_model_cc_proto", diff --git a/source/server/admin/prometheus_stats.cc b/source/server/admin/prometheus_stats.cc index 297a11f49d522..dd5f6d4ee9f87 100644 --- a/source/server/admin/prometheus_stats.cc +++ b/source/server/admin/prometheus_stats.cc @@ -8,6 +8,7 @@ #include "source/common/common/macros.h" #include "source/common/common/regex.h" #include "source/common/protobuf/protobuf.h" +#include "source/common/runtime/runtime_features.h" #include "source/common/stats/histogram_impl.h" #include "source/common/upstream/host_utility.h" @@ -956,6 +957,17 @@ PrometheusStatsFormatter::metricName(std::string&& extracted_name, return sanitized_name; } + // Inner-segment custom namespace (e.g. upstream Wasm under a cluster scope). + if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.strip_scoped_custom_stat_namespace")) { + absl::optional inner_namespace_stripped = + custom_namespaces.stripRegisteredInnerNamespace(extracted_name); + if (inner_namespace_stripped.has_value()) { + sanitizeNameInPlace(inner_namespace_stripped.value()); + return absl::StrCat("envoy_", inner_namespace_stripped.value()); + } + } + // If it does not have a custom namespace, add namespacing prefix to avoid conflicts, as per best // practice: https://prometheus.io/docs/practices/naming/#metric-names Also, naming conventions on // https://prometheus.io/docs/concepts/data_model/ diff --git a/source/server/admin/prometheus_stats.h b/source/server/admin/prometheus_stats.h index 1377511963fea..ec8482cc74e4f 100644 --- a/source/server/admin/prometheus_stats.h +++ b/source/server/admin/prometheus_stats.h @@ -126,10 +126,12 @@ class PrometheusStatsFormatter { const Http::RequestHeaderMap& headers); /** - * Format the given metric name, and prefixed with "envoy_" if it does not have a custom - * stat namespace. If it has a custom stat namespace AND the name without the custom namespace - * has a valid prometheus namespace, the trimmed name is returned. - * Otherwise, return nullopt. + * Format a tag-extracted name for Prometheus output: + * - leading registered custom namespace -> stripped, no "envoy_" prefix. + * - inner registered custom namespace -> stripped, "envoy_" prefix kept. + * Gated by "envoy.reloadable_features.strip_scoped_custom_stat_namespace". + * - otherwise -> "envoy_" prefix added. + * Returns nullopt if the result would not be a valid Prometheus name. */ static absl::optional metricName(std::string&& extracted_name, diff --git a/test/common/stats/custom_stat_namespaces_impl_test.cc b/test/common/stats/custom_stat_namespaces_impl_test.cc index e0500fe5edcf7..643806414ded2 100644 --- a/test/common/stats/custom_stat_namespaces_impl_test.cc +++ b/test/common/stats/custom_stat_namespaces_impl_test.cc @@ -32,5 +32,53 @@ TEST(CustomStatNamespacesImpl, StripRegisteredPrefix) { EXPECT_EQ(actual.value(), "my.extension.metric"); } +TEST(CustomStatNamespacesImpl, StripRegisteredInnerNamespace) { + CustomStatNamespacesImpl namespaces; + + // Empty registry. + EXPECT_FALSE(namespaces.stripRegisteredInnerNamespace("cluster.foo.metric").has_value()); + + namespaces.registerStatNamespace("wasmcustom"); + + // Depth 1. + { + const absl::optional actual = + namespaces.stripRegisteredInnerNamespace("cluster.wasmcustom.upstream_rq_2xx"); + EXPECT_TRUE(actual.has_value()); + EXPECT_EQ(actual.value(), "cluster.upstream_rq_2xx"); + } + + // Depth 2. + { + const absl::optional actual = + namespaces.stripRegisteredInnerNamespace("scope.subscope.wasmcustom.metric"); + EXPECT_TRUE(actual.has_value()); + EXPECT_EQ(actual.value(), "scope.subscope.metric"); + } + + // Multi-token leaf. + { + const absl::optional actual = + namespaces.stripRegisteredInnerNamespace("cluster.wasmcustom.foo.bar.baz"); + EXPECT_TRUE(actual.has_value()); + EXPECT_EQ(actual.value(), "cluster.foo.bar.baz"); + } + + // Leading match: caller should use stripRegisteredPrefix instead. + EXPECT_FALSE(namespaces.stripRegisteredInnerNamespace("wasmcustom.metric").has_value()); + + // Trailing match: that's the metric leaf, not a namespace boundary. + EXPECT_FALSE(namespaces.stripRegisteredInnerNamespace("cluster.foo.wasmcustom").has_value()); + + // No registered segment in the middle. + EXPECT_FALSE(namespaces.stripRegisteredInnerNamespace("cluster.foo.bar.metric").has_value()); + + // Substring within a segment doesn't count. + EXPECT_FALSE(namespaces.stripRegisteredInnerNamespace("cluster.wasmcustomx.metric").has_value()); + + // No dot at all. + EXPECT_FALSE(namespaces.stripRegisteredInnerNamespace("wasmcustom").has_value()); +} + } // namespace Stats } // namespace Envoy diff --git a/test/server/admin/BUILD b/test/server/admin/BUILD index e3bcbf81d16c3..78de8decdfc89 100644 --- a/test/server/admin/BUILD +++ b/test/server/admin/BUILD @@ -207,6 +207,7 @@ envoy_cc_test( "//test/mocks/thread_local:thread_local_mocks", "//test/mocks/upstream:cluster_manager_mocks", "//test/test_common:stats_utility_lib", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", ], ) diff --git a/test/server/admin/prometheus_stats_test.cc b/test/server/admin/prometheus_stats_test.cc index 440c7f2155bff..31c3cf0f65e6b 100644 --- a/test/server/admin/prometheus_stats_test.cc +++ b/test/server/admin/prometheus_stats_test.cc @@ -18,6 +18,7 @@ #include "test/mocks/thread_local/mocks.h" #include "test/mocks/upstream/cluster_manager.h" #include "test/test_common/stats_utility.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" using testing::NiceMock; @@ -179,6 +180,67 @@ TEST_F(PrometheusStatsFormatterTest, CustomNamespace) { EXPECT_EQ(expected, actual.value()); } +TEST_F(PrometheusStatsFormatterTest, ScopedCustomNamespace) { + Stats::CustomStatNamespacesImpl custom_namespaces; + custom_namespaces.registerStatNamespace("wasmcustom"); + std::string raw = "cluster.wasmcustom.upstream_rq_2xx"; + std::string expected = "envoy_cluster_upstream_rq_2xx"; + auto actual = PrometheusStatsFormatter::metricName(std::move(raw), custom_namespaces); + EXPECT_TRUE(actual.has_value()); + EXPECT_EQ(expected, actual.value()); +} + +TEST_F(PrometheusStatsFormatterTest, ScopedCustomNamespaceDeeperNesting) { + Stats::CustomStatNamespacesImpl custom_namespaces; + custom_namespaces.registerStatNamespace("wasmcustom"); + std::string raw = "scope.subscope.wasmcustom.metric"; + std::string expected = "envoy_scope_subscope_metric"; + auto actual = PrometheusStatsFormatter::metricName(std::move(raw), custom_namespaces); + EXPECT_TRUE(actual.has_value()); + EXPECT_EQ(expected, actual.value()); +} + +TEST_F(PrometheusStatsFormatterTest, ScopedCustomNamespaceRuntimeGuardDisabled) { + // Guard off: inner namespace is preserved verbatim. + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.strip_scoped_custom_stat_namespace", "false"}}); + + Stats::CustomStatNamespacesImpl custom_namespaces; + custom_namespaces.registerStatNamespace("wasmcustom"); + std::string raw = "cluster.wasmcustom.upstream_rq_2xx"; + std::string expected = "envoy_cluster_wasmcustom_upstream_rq_2xx"; + auto actual = PrometheusStatsFormatter::metricName(std::move(raw), custom_namespaces); + EXPECT_TRUE(actual.has_value()); + EXPECT_EQ(expected, actual.value()); +} + +TEST_F(PrometheusStatsFormatterTest, LeadingCustomNamespaceUnaffectedByGuard) { + // Prefix stripping (used by listener Wasm) must keep working when the inner-segment guard is + // off. + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.strip_scoped_custom_stat_namespace", "false"}}); + + Stats::CustomStatNamespacesImpl custom_namespaces; + custom_namespaces.registerStatNamespace("wasmcustom"); + std::string raw = "wasmcustom.user_metric"; + std::string expected = "user_metric"; + auto actual = PrometheusStatsFormatter::metricName(std::move(raw), custom_namespaces); + EXPECT_TRUE(actual.has_value()); + EXPECT_EQ(expected, actual.value()); +} + +TEST_F(PrometheusStatsFormatterTest, ScopedUnregisteredCustomNamespace) { + Stats::CustomStatNamespacesImpl custom_namespaces; + custom_namespaces.registerStatNamespace("promstattest"); + std::string raw = "cluster.wasmcustom.upstream_rq_2xx"; + std::string expected = "envoy_cluster_wasmcustom_upstream_rq_2xx"; + auto actual = PrometheusStatsFormatter::metricName(std::move(raw), custom_namespaces); + EXPECT_TRUE(actual.has_value()); + EXPECT_EQ(expected, actual.value()); +} + TEST_F(PrometheusStatsFormatterTest, CustomNamespaceWithInvalidPromnamespace) { Stats::CustomStatNamespacesImpl custom_namespaces; custom_namespaces.registerStatNamespace("promstattest"); @@ -202,6 +264,30 @@ TEST_F(PrometheusStatsFormatterTest, FormattedTags) { EXPECT_EQ(expected, actual); } +// End-to-end: cluster-scoped custom namespace counter renders with namespace stripped and +// cluster_name preserved as a label. +TEST_F(PrometheusStatsFormatterTest, ScopedCustomNamespaceFullPipeline) { + Stats::CustomStatNamespacesImpl custom_namespaces; + custom_namespaces.registerStatNamespace("wasmcustom"); + + addCounter("cluster.wasmcustom.upstream_rq_2xx", + {{makeStat("envoy.cluster_name"), makeStat("test_cluster_1")}}); + + Buffer::OwnedImpl response; + const uint64_t size = PrometheusStatsFormatter::statsAsPrometheusText( + counters_, gauges_, histograms_, textReadouts_, endpoints_helper_->cm_, response, + StatsParams(), custom_namespaces); + EXPECT_EQ(1UL, size); + + const std::string output = response.toString(); + EXPECT_NE(output.find("# TYPE envoy_cluster_upstream_rq_2xx counter"), std::string::npos) + << output; + EXPECT_NE(output.find("envoy_cluster_upstream_rq_2xx{envoy_cluster_name=\"test_cluster_1\"}"), + std::string::npos) + << output; + EXPECT_EQ(output.find("wasmcustom"), std::string::npos) << output; +} + TEST_F(PrometheusStatsFormatterTest, MetricNameCollison) { Stats::CustomStatNamespacesImpl custom_namespaces;