metrics service: allow only histogram summaries

api/envoy/config/metrics/v3/metrics_service.proto

@@ -19,6 +19,18 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
 
 // [#protodoc-title: Metrics service]
 
+// HistogramEmitMode is used to configure which metric types should be emitted for histograms.
+enum HistogramEmitMode {
+  // Emit Histogram and Summary metric types.
+  SUMMARY_AND_HISTOGRAM = 0;
+
+  // Emit only Summary metric types.
+  SUMMARY = 1;
+
+  // Emit only Histogram metric types.
+  HISTOGRAM = 2;
+}
+
 // Metrics Service is configured as a built-in ``envoy.stat_sinks.metrics_service`` :ref:`StatsSink
 // <envoy_v3_api_msg_config.metrics.v3.StatsSink>`. This opaque configuration will be used to create
 // Metrics Service.
@@ -34,6 +46,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
 //           transport_api_version: V3
 //
 // [#extension: envoy.stat_sinks.metrics_service]
+// [#next-free-field: 6]
 message MetricsServiceConfig {
   option (udpa.annotations.versioning).previous_message_type =
       "envoy.config.metrics.v2.MetricsServiceConfig";
@@ -55,4 +68,7 @@ message MetricsServiceConfig {
   // and the tag extracted name will be used instead of the full name, which may contain values used by the tag
   // extractor or additional tags added during stats creation.
   bool emit_tags_as_labels = 4;
+
+  // Specify which metrics types to emit for histograms. Defaults to SUMMARY_AND_HISTOGRAM.
+  HistogramEmitMode histogram_emit_mode = 5 [(validate.rules).enum = {defined_only: true}];
 }

changelogs/current.yaml

@@ -388,6 +388,11 @@ new_features:
     added new field :ref:`signature_algorithms
     <envoy_v3_api_field_extensions.transport_sockets.tls.v3.TlsParameters.signature_algorithms>` to set signature
     algorithms.
+- area: metrics_service
+  change: |
+    added new configuration field :ref:`histogram_emit_mode
+    <envoy_v3_api_field_config.metrics.v3.MetricsServiceConfig.histogram_emit_mode>` to configure which stats
+    should be emitted for histograms.
 
 deprecated:
 - area: ext_authz

source/extensions/stat_sinks/metrics_service/BUILD

@@ -22,6 +22,7 @@ envoy_cc_library(
         "//envoy/upstream:cluster_manager_interface",
         "//source/common/common:assert_lib",
         "//source/common/grpc:async_client_lib",
+        "@envoy_api//envoy/config/metrics/v3:pkg_cc_proto",
         "@envoy_api//envoy/service/metrics/v3:pkg_cc_proto",
     ],
 )

source/extensions/stat_sinks/metrics_service/config.cc

@@ -39,7 +39,7 @@ MetricsServiceSinkFactory::createStatsSink(const Protobuf::Message& config,
                                              envoy::service::metrics::v3::StreamMetricsResponse>>(
       grpc_metrics_streamer,
       PROTOBUF_GET_WRAPPED_OR_DEFAULT(sink_config, report_counters_as_deltas, false),
-      sink_config.emit_tags_as_labels());
+      sink_config.emit_tags_as_labels(), sink_config.histogram_emit_mode());
 }
 
 ProtobufTypes::MessagePtr MetricsServiceSinkFactory::createEmptyConfigProto() {

source/extensions/stat_sinks/metrics_service/grpc_metrics_service_impl.cc

@@ -3,6 +3,7 @@
 #include <chrono>
 
 #include "envoy/common/exception.h"
+#include "envoy/config/metrics/v3/metrics_service.pb.h"
 #include "envoy/event/dispatcher.h"
 #include "envoy/service/metrics/v3/metrics_service.pb.h"
 #include "envoy/stats/histogram.h"
@@ -68,7 +69,12 @@ MetricsPtr MetricsFlusher::flush(Stats::MetricSnapshot& snapshot) const {
 
   for (const auto& histogram : snapshot.histograms()) {
     if (predicate_(histogram.get())) {
-      flushHistogram(*metrics->Add(), *metrics->Add(), histogram.get(), snapshot_time_ms);
+      if (emit_summary_) {
+        flushSummary(*metrics->Add(), histogram.get(), snapshot_time_ms);
+      }
+      if (emit_histogram_) {
+        flushHistogram(*metrics->Add(), histogram.get(), snapshot_time_ms);
+      }
     }
   }
 
@@ -96,29 +102,13 @@ void MetricsFlusher::flushGauge(io::prometheus::client::MetricFamily& metrics_fa
   gauge_metric->set_value(gauge.value());
 }
 
-void MetricsFlusher::flushHistogram(io::prometheus::client::MetricFamily& summary_metrics_family,
-                                    io::prometheus::client::MetricFamily& histogram_metrics_family,
+void MetricsFlusher::flushHistogram(io::prometheus::client::MetricFamily& metrics_family,
                                     const Stats::ParentHistogram& envoy_histogram,
                                     int64_t snapshot_time_ms) const {
-  // TODO(ramaraochavali): Currently we are sending both quantile information and bucket
-  // information. We should make this configurable if it turns out that sending both affects
-  // performance.
 
-  // Add summary information for histograms.
-  auto* summary_metric =
-      populateMetricsFamily(summary_metrics_family, io::prometheus::client::MetricType::SUMMARY,
-                            snapshot_time_ms, envoy_histogram);
-  auto* summary = summary_metric->mutable_summary();
   const Stats::HistogramStatistics& hist_stats = envoy_histogram.intervalStatistics();
-  for (size_t i = 0; i < hist_stats.supportedQuantiles().size(); i++) {
-    auto* quantile = summary->add_quantile();
-    quantile->set_quantile(hist_stats.supportedQuantiles()[i]);
-    quantile->set_value(hist_stats.computedQuantiles()[i]);
-  }
-
-  // Add bucket information for histograms.
   auto* histogram_metric =
-      populateMetricsFamily(histogram_metrics_family, io::prometheus::client::MetricType::HISTOGRAM,
+      populateMetricsFamily(metrics_family, io::prometheus::client::MetricType::HISTOGRAM,
                             snapshot_time_ms, envoy_histogram);
   auto* histogram = histogram_metric->mutable_histogram();
   histogram->set_sample_count(hist_stats.sampleCount());
@@ -130,6 +120,23 @@ void MetricsFlusher::flushHistogram(io::prometheus::client::MetricFamily& summar
   }
 }
 
+void MetricsFlusher::flushSummary(io::prometheus::client::MetricFamily& metrics_family,
+                                  const Stats::ParentHistogram& envoy_histogram,
+                                  int64_t snapshot_time_ms) const {
+
+  const Stats::HistogramStatistics& hist_stats = envoy_histogram.intervalStatistics();
+  auto* summary_metric =
+      populateMetricsFamily(metrics_family, io::prometheus::client::MetricType::SUMMARY,
+                            snapshot_time_ms, envoy_histogram);
+  auto* summary = summary_metric->mutable_summary();
+  for (size_t i = 0; i < hist_stats.supportedQuantiles().size(); i++) {
+    auto* quantile = summary->add_quantile();
+    quantile->set_quantile(hist_stats.supportedQuantiles()[i]);
+    quantile->set_value(hist_stats.computedQuantiles()[i]);
+  }
+  summary->set_sample_count(hist_stats.sampleCount());
+}
+
 io::prometheus::client::Metric*
 MetricsFlusher::populateMetricsFamily(io::prometheus::client::MetricFamily& metrics_family,
                                       io::prometheus::client::MetricType type,

source/extensions/stat_sinks/metrics_service/grpc_metrics_service_impl.h

@@ -2,6 +2,7 @@
 
 #include <memory>
 
+#include "envoy/config/metrics/v3/metrics_service.pb.h"
 #include "envoy/grpc/async_client.h"
 #include "envoy/local_info/local_info.h"
 #include "envoy/network/connection.h"
@@ -22,6 +23,7 @@ namespace MetricsService {
 
 using MetricsPtr =
     std::unique_ptr<Envoy::Protobuf::RepeatedPtrField<io::prometheus::client::MetricFamily>>;
+using HistogramEmitMode = envoy::config::metrics::v3::HistogramEmitMode;
 
 /**
  * Interface for metrics streamer.
@@ -82,10 +84,14 @@ using GrpcMetricsStreamerImplPtr = std::unique_ptr<GrpcMetricsStreamerImpl>;
 class MetricsFlusher {
 public:
   MetricsFlusher(
-      bool report_counters_as_deltas, bool emit_labels,
+      bool report_counters_as_deltas, bool emit_labels, HistogramEmitMode histogram_emit_mode,
       std::function<bool(const Stats::Metric&)> predicate =
           [](const auto& metric) { return metric.used(); })
       : report_counters_as_deltas_(report_counters_as_deltas), emit_labels_(emit_labels),
+        emit_summary_(histogram_emit_mode == HistogramEmitMode::SUMMARY_AND_HISTOGRAM ||
+                      histogram_emit_mode == HistogramEmitMode::SUMMARY),
+        emit_histogram_(histogram_emit_mode == HistogramEmitMode::SUMMARY_AND_HISTOGRAM ||
+                        histogram_emit_mode == HistogramEmitMode::HISTOGRAM),
         predicate_(predicate) {}
 
   MetricsPtr flush(Stats::MetricSnapshot& snapshot) const;
@@ -96,10 +102,11 @@ class MetricsFlusher {
                     int64_t snapshot_time_ms) const;
   void flushGauge(io::prometheus::client::MetricFamily& metrics_family, const Stats::Gauge& gauge,
                   int64_t snapshot_time_ms) const;
-  void flushHistogram(io::prometheus::client::MetricFamily& summary_metrics_family,
-                      io::prometheus::client::MetricFamily& histogram_metrics_family,
+  void flushHistogram(io::prometheus::client::MetricFamily& metrics_family,
                       const Stats::ParentHistogram& envoy_histogram,
                       int64_t snapshot_time_ms) const;
+  void flushSummary(io::prometheus::client::MetricFamily& metrics_family,
+                    const Stats::ParentHistogram& envoy_histogram, int64_t snapshot_time_ms) const;
 
   io::prometheus::client::Metric*
   populateMetricsFamily(io::prometheus::client::MetricFamily& metrics_family,
@@ -108,6 +115,8 @@ class MetricsFlusher {
 
   const bool report_counters_as_deltas_;
   const bool emit_labels_;
+  const bool emit_summary_;
+  const bool emit_histogram_;
   const std::function<bool(const Stats::Metric&)> predicate_;
 };
 
@@ -118,9 +127,10 @@ template <class RequestProto, class ResponseProto> class MetricsServiceSink : pu
 public:
   MetricsServiceSink(
       const GrpcMetricsStreamerSharedPtr<RequestProto, ResponseProto>& grpc_metrics_streamer,
-      bool report_counters_as_deltas, bool emit_labels)
-      : MetricsServiceSink(grpc_metrics_streamer,
-                           MetricsFlusher(report_counters_as_deltas, emit_labels)) {}
+      bool report_counters_as_deltas, bool emit_labels, HistogramEmitMode histogram_emit_mode)
+      : MetricsServiceSink(
+            grpc_metrics_streamer,
+            MetricsFlusher(report_counters_as_deltas, emit_labels, histogram_emit_mode)) {}
 
   MetricsServiceSink(
       const GrpcMetricsStreamerSharedPtr<RequestProto, ResponseProto>& grpc_metrics_streamer,

test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc

@@ -135,7 +135,8 @@ class MetricsServiceSinkTest : public testing::Test {
 TEST_F(MetricsServiceSinkTest, CheckSendCall) {
   MetricsServiceSink<envoy::service::metrics::v3::StreamMetricsMessage,
                      envoy::service::metrics::v3::StreamMetricsResponse>
-      sink(streamer_, false, false);
+      sink(streamer_, false, false,
+           envoy::config::metrics::v3::HistogramEmitMode::SUMMARY_AND_HISTOGRAM);
 
   addCounterToSnapshot("test_counter", 1, 1);
   addGaugeToSnapshot("test_gauge", 1);
@@ -149,7 +150,8 @@ TEST_F(MetricsServiceSinkTest, CheckSendCall) {
 TEST_F(MetricsServiceSinkTest, CheckStatsCount) {
   MetricsServiceSink<envoy::service::metrics::v3::StreamMetricsMessage,
                      envoy::service::metrics::v3::StreamMetricsResponse>
-      sink(streamer_, false, false);
+      sink(streamer_, false, false,
+           envoy::config::metrics::v3::HistogramEmitMode::SUMMARY_AND_HISTOGRAM);
 
   addCounterToSnapshot("test_counter", 1, 100);
   addGaugeToSnapshot("test_gauge", 1);
@@ -171,7 +173,8 @@ TEST_F(MetricsServiceSinkTest, CheckStatsCount) {
 TEST_F(MetricsServiceSinkTest, ReportCountersValues) {
   MetricsServiceSink<envoy::service::metrics::v3::StreamMetricsMessage,
                      envoy::service::metrics::v3::StreamMetricsResponse>
-      sink(streamer_, false, false);
+      sink(streamer_, false, false,
+           envoy::config::metrics::v3::HistogramEmitMode::SUMMARY_AND_HISTOGRAM);
 
   addCounterToSnapshot("test_counter", 1, 100);
 
@@ -192,7 +195,8 @@ TEST_F(MetricsServiceSinkTest, ReportCountersAsDeltas) {
     // This test won't emit any labels.
     MetricsServiceSink<envoy::service::metrics::v3::StreamMetricsMessage,
                        envoy::service::metrics::v3::StreamMetricsResponse>
-        sink(streamer_, true, false);
+        sink(streamer_, true, false,
+             envoy::config::metrics::v3::HistogramEmitMode::SUMMARY_AND_HISTOGRAM);
 
     EXPECT_CALL(*streamer_, send(_)).WillOnce(Invoke([](MetricsPtr&& metrics) {
       ASSERT_EQ(1, metrics->size());
@@ -209,7 +213,8 @@ TEST_F(MetricsServiceSinkTest, ReportCountersAsDeltas) {
     // This test will emit labels.
     MetricsServiceSink<envoy::service::metrics::v3::StreamMetricsMessage,
                        envoy::service::metrics::v3::StreamMetricsResponse>
-        sink(streamer_, true, true);
+        sink(streamer_, true, true,
+             envoy::config::metrics::v3::HistogramEmitMode::SUMMARY_AND_HISTOGRAM);
 
     EXPECT_CALL(*streamer_, send(_)).WillOnce(Invoke([](MetricsPtr&& metrics) {
       ASSERT_EQ(1, metrics->size());
@@ -241,7 +246,8 @@ TEST_F(MetricsServiceSinkTest, ReportMetricsWithTags) {
     // When the emit_tags flag is false, we don't emit the tags and use the full name.
     MetricsServiceSink<envoy::service::metrics::v3::StreamMetricsMessage,
                        envoy::service::metrics::v3::StreamMetricsResponse>
-        sink(streamer_, false, false);
+        sink(streamer_, false, false,
+             envoy::config::metrics::v3::HistogramEmitMode::SUMMARY_AND_HISTOGRAM);
 
     EXPECT_CALL(*streamer_, send(_)).WillOnce(Invoke([](MetricsPtr&& metrics) {
       EXPECT_EQ(4, metrics->size());
@@ -268,7 +274,8 @@ TEST_F(MetricsServiceSinkTest, ReportMetricsWithTags) {
   // When the emit_tags flag is true, we emit the tags as labels and use the tag extracted name.
   MetricsServiceSink<envoy::service::metrics::v3::StreamMetricsMessage,
                      envoy::service::metrics::v3::StreamMetricsResponse>
-      sink(streamer_, false, true);
+      sink(streamer_, false, true,
+           envoy::config::metrics::v3::HistogramEmitMode::SUMMARY_AND_HISTOGRAM);
 
   EXPECT_CALL(*streamer_, send(_)).WillOnce(Invoke([&expected_label_pair](MetricsPtr&& metrics) {
     EXPECT_EQ(4, metrics->size());
@@ -298,24 +305,87 @@ TEST_F(MetricsServiceSinkTest, FlushPredicate) {
 
   // Default predicate only accepts used metrics.
   {
-    MetricsFlusher flusher(true, true);
+    MetricsFlusher flusher(true, true,
+                           envoy::config::metrics::v3::HistogramEmitMode::SUMMARY_AND_HISTOGRAM);
     auto metrics = flusher.flush(snapshot_);
     EXPECT_EQ(1, metrics->size());
   }
 
   // Using a predicate that accepts all metrics, we'd flush both metrics.
   {
-    MetricsFlusher flusher(true, true, [](const auto&) { return true; });
+    MetricsFlusher flusher(true, true,
+                           envoy::config::metrics::v3::HistogramEmitMode::SUMMARY_AND_HISTOGRAM,
+                           [](const auto&) { return true; });
     auto metrics = flusher.flush(snapshot_);
     EXPECT_EQ(2, metrics->size());
   }
 
   // Using a predicate that rejects all metrics, we'd flush no metrics.
-  MetricsFlusher flusher(true, true, [](const auto&) { return false; });
+  MetricsFlusher flusher(true, true,
+                         envoy::config::metrics::v3::HistogramEmitMode::SUMMARY_AND_HISTOGRAM,
+                         [](const auto&) { return false; });
   auto metrics = flusher.flush(snapshot_);
   EXPECT_EQ(0, metrics->size());
 }
 
+// This test will emit summary and histogram.
+TEST_F(MetricsServiceSinkTest, HistogramEmitModeBoth) {
+  addHistogramToSnapshot("test_histogram");
+
+  MetricsServiceSink<envoy::service::metrics::v3::StreamMetricsMessage,
+                     envoy::service::metrics::v3::StreamMetricsResponse>
+      sink(streamer_, true, false,
+           envoy::config::metrics::v3::HistogramEmitMode::SUMMARY_AND_HISTOGRAM);
+
+  EXPECT_CALL(*streamer_, send(_)).WillOnce(Invoke([](MetricsPtr&& metrics) {
+    ASSERT_EQ(2, metrics->size());
+    EXPECT_EQ("test_histogram", (*metrics)[0].name());
+    EXPECT_EQ("test_histogram", (*metrics)[1].name());
+
+    const auto& metric1 = (*metrics)[0].metric(0);
+    EXPECT_TRUE(metric1.has_summary());
+    const auto& metric2 = (*metrics)[1].metric(0);
+    EXPECT_TRUE(metric2.has_histogram());
+  }));
+  sink.flush(snapshot_);
+}
+
+// This test will only summary.
+TEST_F(MetricsServiceSinkTest, HistogramEmitModeSummary) {
+  addHistogramToSnapshot("test_histogram");
+
+  MetricsServiceSink<envoy::service::metrics::v3::StreamMetricsMessage,
+                     envoy::service::metrics::v3::StreamMetricsResponse>
+      sink(streamer_, true, false, envoy::config::metrics::v3::HistogramEmitMode::SUMMARY);
+
+  EXPECT_CALL(*streamer_, send(_)).WillOnce(Invoke([](MetricsPtr&& metrics) {
+    ASSERT_EQ(1, metrics->size());
+    EXPECT_EQ("test_histogram", (*metrics)[0].name());
+
+    const auto& metric1 = (*metrics)[0].metric(0);
+    EXPECT_TRUE(metric1.has_summary());
+  }));
+  sink.flush(snapshot_);
+}
+
+// This test will only histogram.
+TEST_F(MetricsServiceSinkTest, HistogramEmitModeHistogram) {
+  addHistogramToSnapshot("test_histogram");
+
+  MetricsServiceSink<envoy::service::metrics::v3::StreamMetricsMessage,
+                     envoy::service::metrics::v3::StreamMetricsResponse>
+      sink(streamer_, true, false, envoy::config::metrics::v3::HistogramEmitMode::HISTOGRAM);
+
+  EXPECT_CALL(*streamer_, send(_)).WillOnce(Invoke([](MetricsPtr&& metrics) {
+    ASSERT_EQ(1, metrics->size());
+    EXPECT_EQ("test_histogram", (*metrics)[0].name());
+
+    const auto& metric1 = (*metrics)[0].metric(0);
+    EXPECT_TRUE(metric1.has_histogram());
+  }));
+  sink.flush(snapshot_);
+}
+
 } // namespace
 } // namespace MetricsService
 } // namespace StatSinks