From 6ae48f6e2c79de9c91f23e86bc88484b3d393d09 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 21 Apr 2020 15:08:37 -0700 Subject: [PATCH 1/8] metrics service sink: add config option to report counters as deltas Signed-off-by: Jose Nino --- .../config/metrics/v3/metrics_service.proto | 4 + .../config/metrics/v3/metrics_service.proto | 4 + .../stat_sinks/metrics_service/config.cc | 3 +- .../grpc_metrics_service_impl.cc | 19 ++-- .../grpc_metrics_service_impl.h | 5 +- .../grpc_metrics_service_impl_test.cc | 88 +++++++++++++------ tools/code_format/check_format.py | 2 +- 7 files changed, 86 insertions(+), 39 deletions(-) diff --git a/api/envoy/config/metrics/v3/metrics_service.proto b/api/envoy/config/metrics/v3/metrics_service.proto index ad9879055ba3c..3d6b61a9697ec 100644 --- a/api/envoy/config/metrics/v3/metrics_service.proto +++ b/api/envoy/config/metrics/v3/metrics_service.proto @@ -25,4 +25,8 @@ message MetricsServiceConfig { // The upstream gRPC cluster that hosts the metrics service. core.v3.GrpcService grpc_service = 1 [(validate.rules).message = {required: true}]; + + // If true, counters are reported as the delta between flushing intervals. Otherwise, the current + // counter value is reported. Defaults to false. + bool report_counters_as_deltas = 2; } diff --git a/generated_api_shadow/envoy/config/metrics/v3/metrics_service.proto b/generated_api_shadow/envoy/config/metrics/v3/metrics_service.proto index ad9879055ba3c..3d6b61a9697ec 100644 --- a/generated_api_shadow/envoy/config/metrics/v3/metrics_service.proto +++ b/generated_api_shadow/envoy/config/metrics/v3/metrics_service.proto @@ -25,4 +25,8 @@ message MetricsServiceConfig { // The upstream gRPC cluster that hosts the metrics service. core.v3.GrpcService grpc_service = 1 [(validate.rules).message = {required: true}]; + + // If true, counters are reported as the delta between flushing intervals. Otherwise, the current + // counter value is reported. Defaults to false. + bool report_counters_as_deltas = 2; } diff --git a/source/extensions/stat_sinks/metrics_service/config.cc b/source/extensions/stat_sinks/metrics_service/config.cc index 4f8402e201b0e..4701730a8fd51 100644 --- a/source/extensions/stat_sinks/metrics_service/config.cc +++ b/source/extensions/stat_sinks/metrics_service/config.cc @@ -33,7 +33,8 @@ Stats::SinkPtr MetricsServiceSinkFactory::createStatsSink(const Protobuf::Messag grpc_service, server.stats(), false), server.localInfo()); - return std::make_unique(grpc_metrics_streamer, server.timeSource()); + return std::make_unique(grpc_metrics_streamer, server.timeSource(), + sink_config.report_counters_as_deltas()); } ProtobufTypes::MessagePtr MetricsServiceSinkFactory::createEmptyConfigProto() { diff --git a/source/extensions/stat_sinks/metrics_service/grpc_metrics_service_impl.cc b/source/extensions/stat_sinks/metrics_service/grpc_metrics_service_impl.cc index cfea996f40d78..85f2a63b7fb3d 100644 --- a/source/extensions/stat_sinks/metrics_service/grpc_metrics_service_impl.cc +++ b/source/extensions/stat_sinks/metrics_service/grpc_metrics_service_impl.cc @@ -34,19 +34,26 @@ void GrpcMetricsStreamerImpl::send(envoy::service::metrics::v3::StreamMetricsMes } MetricsServiceSink::MetricsServiceSink(const GrpcMetricsStreamerSharedPtr& grpc_metrics_streamer, - TimeSource& time_source) - : grpc_metrics_streamer_(grpc_metrics_streamer), time_source_(time_source) {} + TimeSource& time_source, + const bool report_counters_as_deltas) + : grpc_metrics_streamer_(grpc_metrics_streamer), time_source_(time_source), + report_counters_as_deltas_(report_counters_as_deltas) {} -void MetricsServiceSink::flushCounter(const Stats::Counter& counter) { +void MetricsServiceSink::flushCounter( + const Stats::MetricSnapshot::CounterSnapshot& counter_snapshot) { io::prometheus::client::MetricFamily* metrics_family = message_.add_envoy_metrics(); metrics_family->set_type(io::prometheus::client::MetricType::COUNTER); - metrics_family->set_name(counter.name()); + metrics_family->set_name(counter_snapshot.counter_.get().name()); auto* metric = metrics_family->add_metric(); metric->set_timestamp_ms(std::chrono::duration_cast( time_source_.systemTime().time_since_epoch()) .count()); auto* counter_metric = metric->mutable_counter(); - counter_metric->set_value(counter.value()); + if (report_counters_as_deltas_) { + counter_metric->set_value(counter_snapshot.delta_); + } else { + counter_metric->set_value(counter_snapshot.counter_.get().value()); + } } void MetricsServiceSink::flushGauge(const Stats::Gauge& gauge) { @@ -110,7 +117,7 @@ void MetricsServiceSink::flush(Stats::MetricSnapshot& snapshot) { snapshot.histograms().size()); for (const auto& counter : snapshot.counters()) { if (counter.counter_.get().used()) { - flushCounter(counter.counter_.get()); + flushCounter(counter); } } diff --git a/source/extensions/stat_sinks/metrics_service/grpc_metrics_service_impl.h b/source/extensions/stat_sinks/metrics_service/grpc_metrics_service_impl.h index f8d500a058496..84c2d19695f48 100644 --- a/source/extensions/stat_sinks/metrics_service/grpc_metrics_service_impl.h +++ b/source/extensions/stat_sinks/metrics_service/grpc_metrics_service_impl.h @@ -73,11 +73,11 @@ class MetricsServiceSink : public Stats::Sink { public: // MetricsService::Sink MetricsServiceSink(const GrpcMetricsStreamerSharedPtr& grpc_metrics_streamer, - TimeSource& time_system); + TimeSource& time_system, const bool report_counters_as_deltas); void flush(Stats::MetricSnapshot& snapshot) override; void onHistogramComplete(const Stats::Histogram&, uint64_t) override {} - void flushCounter(const Stats::Counter& counter); + void flushCounter(const Stats::MetricSnapshot::CounterSnapshot& counter_snapshot); void flushGauge(const Stats::Gauge& gauge); void flushHistogram(const Stats::ParentHistogram& envoy_histogram); @@ -85,6 +85,7 @@ class MetricsServiceSink : public Stats::Sink { GrpcMetricsStreamerSharedPtr grpc_metrics_streamer_; envoy::service::metrics::v3::StreamMetricsMessage message_; TimeSource& time_source_; + const bool report_counters_as_deltas_; }; } // namespace MetricsService diff --git a/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc b/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc index c7543903d0f90..4e02ac5cab9f2 100644 --- a/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc +++ b/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc @@ -90,35 +90,29 @@ class MockGrpcMetricsStreamer : public GrpcMetricsStreamer { MOCK_METHOD(void, send, (envoy::service::metrics::v3::StreamMetricsMessage & message)); }; -class TestGrpcMetricsStreamer : public GrpcMetricsStreamer { +class MetricsServiceSinkTest : public testing::Test { public: - int metric_count; - // GrpcMetricsStreamer - void send(envoy::service::metrics::v3::StreamMetricsMessage& message) override { - metric_count = message.envoy_metrics_size(); - } -}; + MetricsServiceSinkTest() {} -class MetricsServiceSinkTest : public testing::Test {}; - -TEST(MetricsServiceSinkTest, CheckSendCall) { - NiceMock snapshot; - Event::SimulatedTimeSystem time_system; + NiceMock snapshot_; + Event::SimulatedTimeSystem time_system_; std::shared_ptr streamer_{new MockGrpcMetricsStreamer()}; +}; - MetricsServiceSink sink(streamer_, time_system); +TEST_F(MetricsServiceSinkTest, CheckSendCall) { + MetricsServiceSink sink(streamer_, time_system_, false); auto counter = std::make_shared>(); counter->name_ = "test_counter"; counter->latch_ = 1; counter->used_ = true; - snapshot.counters_.push_back({1, *counter}); + snapshot_.counters_.push_back({1, *counter}); auto gauge = std::make_shared>(); gauge->name_ = "test_gauge"; gauge->value_ = 1; gauge->used_ = true; - snapshot.gauges_.push_back(*gauge); + snapshot_.gauges_.push_back(*gauge); auto histogram = std::make_shared>(); histogram->name_ = "test_histogram"; @@ -126,35 +120,71 @@ TEST(MetricsServiceSinkTest, CheckSendCall) { EXPECT_CALL(*streamer_, send(_)); - sink.flush(snapshot); + sink.flush(snapshot_); } -TEST(MetricsServiceSinkTest, CheckStatsCount) { - NiceMock snapshot; - Event::SimulatedTimeSystem time_system; - std::shared_ptr streamer_{new TestGrpcMetricsStreamer()}; - - MetricsServiceSink sink(streamer_, time_system); +TEST_F(MetricsServiceSinkTest, CheckStatsCount) { + MetricsServiceSink sink(streamer_, time_system_, false); auto counter = std::make_shared>(); counter->name_ = "test_counter"; - counter->latch_ = 1; + counter->value_ = 100; counter->used_ = true; - snapshot.counters_.push_back({1, *counter}); + snapshot_.counters_.push_back({1, *counter}); auto gauge = std::make_shared>(); gauge->name_ = "test_gauge"; gauge->value_ = 1; gauge->used_ = true; - snapshot.gauges_.push_back(*gauge); + snapshot_.gauges_.push_back(*gauge); - sink.flush(snapshot); - EXPECT_EQ(2, (*streamer_).metric_count); + EXPECT_CALL(*streamer_, send(_)) + .WillOnce(Invoke([](envoy::service::metrics::v3::StreamMetricsMessage& message) { + EXPECT_EQ(2, message.envoy_metrics_size()); + })); + sink.flush(snapshot_); // Verify only newly added metrics come after endFlush call. gauge->used_ = false; - sink.flush(snapshot); - EXPECT_EQ(1, (*streamer_).metric_count); + EXPECT_CALL(*streamer_, send(_)) + .WillOnce(Invoke([](envoy::service::metrics::v3::StreamMetricsMessage& message) { + EXPECT_EQ(1, message.envoy_metrics_size()); + })); + sink.flush(snapshot_); +} + +TEST_F(MetricsServiceSinkTest, ReportCountersValues) { + MetricsServiceSink sink(streamer_, time_system_, false); + + auto counter = std::make_shared>(); + counter->name_ = "test_counter"; + counter->value_ = 100; + counter->used_ = true; + snapshot_.counters_.push_back({1, *counter}); + + EXPECT_CALL(*streamer_, send(_)) + .WillOnce(Invoke([](envoy::service::metrics::v3::StreamMetricsMessage& message) { + EXPECT_EQ(1, message.envoy_metrics_size()); + EXPECT_EQ(100, message.envoy_metrics(0).metric(0).counter().value()); + })); + sink.flush(snapshot_); +} + +TEST_F(MetricsServiceSinkTest, ReportCountersAsDeltas) { + MetricsServiceSink sink(streamer_, time_system_, true); + + auto counter = std::make_shared>(); + counter->name_ = "test_counter"; + counter->value_ = 100; + counter->used_ = true; + snapshot_.counters_.push_back({1, *counter}); + + EXPECT_CALL(*streamer_, send(_)) + .WillOnce(Invoke([](envoy::service::metrics::v3::StreamMetricsMessage& message) { + EXPECT_EQ(1, message.envoy_metrics_size()); + EXPECT_EQ(1, message.envoy_metrics(0).metric(0).counter().value()); + })); + sink.flush(snapshot_); } } // namespace diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index c9683f8cce4de..887fa48798ba4 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -1031,7 +1031,7 @@ def ownedDirectories(error_messages): results = [] def PooledCheckFormat(path_predicate): - pool = multiprocessing.Pool(processes=args.num_workers) + pool = multiprocessing.get_context("fork").Pool(processes=args.num_workers) # For each file in target_path, start a new task in the pool and collect the # results (results is passed by reference, and is used as an output). for root, _, files in os.walk(target_path): From 021890cc86e30797532090138a71c6029da2d606 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 21 Apr 2020 15:21:18 -0700 Subject: [PATCH 2/8] check format rever Signed-off-by: Jose Nino --- tools/code_format/check_format.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index 887fa48798ba4..c9683f8cce4de 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -1031,7 +1031,7 @@ def ownedDirectories(error_messages): results = [] def PooledCheckFormat(path_predicate): - pool = multiprocessing.get_context("fork").Pool(processes=args.num_workers) + pool = multiprocessing.Pool(processes=args.num_workers) # For each file in target_path, start a new task in the pool and collect the # results (results is passed by reference, and is used as an output). for root, _, files in os.walk(target_path): From 0ff7bab94dd0f33134647cedd6accfa9704c43bf Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 21 Apr 2020 15:47:50 -0700 Subject: [PATCH 3/8] update release notes Signed-off-by: Jose Nino --- docs/root/version_history/current.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index a445ccfb421e5..a63f97fb110e1 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -7,7 +7,7 @@ Changes * access loggers: added GRPC_STATUS operator on logging format. * access loggers: extened specifier for FilterStateFormatter to output :ref:`unstructured log string `. * dynamic forward proxy: added :ref:`SNI based dynamic forward proxy ` support. -* fault: added support for controlling the percentage of requests that abort, delay and response rate limits faults +* fault: added support for controlling the percentage of requests that abort, delay and response rate limits faults are applied to using :ref:`HTTP headers ` to the HTTP fault filter. * filter: add `upstram_rq_time` stats to the GPRC stats filter. Disabled by default and can be enabled via :ref:`enable_upstream_stats `. @@ -18,6 +18,7 @@ Changes * logger: added :ref:`--log-format-prefix-with-location ` command line option to prefix '%v' with file path and line number. * network filters: added a :ref:`postgres proxy filter `. * router: allow retries of streaming or incomplete requests. This removes stat `rq_retry_skipped_request_not_complete`. +* stats: added the option to :ref:`report counters as deltas ` to the metrics service stats sink. * tracing: tracing configuration has been made fully dynamic and every HTTP connection manager can now have a separate :ref:`tracing provider `. From 9f2d4ea2c60b18ff93725bad69cc963850c0920f Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 23 Apr 2020 14:01:35 -0700 Subject: [PATCH 4/8] update Signed-off-by: Jose Nino --- api/envoy/config/metrics/v3/metrics_service.proto | 4 +++- .../metrics_service/grpc_metrics_service_impl_test.cc | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/api/envoy/config/metrics/v3/metrics_service.proto b/api/envoy/config/metrics/v3/metrics_service.proto index 3d6b61a9697ec..dc3426b640789 100644 --- a/api/envoy/config/metrics/v3/metrics_service.proto +++ b/api/envoy/config/metrics/v3/metrics_service.proto @@ -4,6 +4,8 @@ package envoy.config.metrics.v3; import "envoy/config/core/v3/grpc_service.proto"; +import "google/protobuf/wrappers.proto"; + import "udpa/annotations/status.proto"; import "udpa/annotations/versioning.proto"; import "validate/validate.proto"; @@ -28,5 +30,5 @@ message MetricsServiceConfig { // If true, counters are reported as the delta between flushing intervals. Otherwise, the current // counter value is reported. Defaults to false. - bool report_counters_as_deltas = 2; + google.protobuf.BoolValue report_counters_as_deltas = 2; } diff --git a/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc b/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc index 4e02ac5cab9f2..f3d07c816aefb 100644 --- a/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc +++ b/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc @@ -92,7 +92,7 @@ class MockGrpcMetricsStreamer : public GrpcMetricsStreamer { class MetricsServiceSinkTest : public testing::Test { public: - MetricsServiceSinkTest() {} + MetricsServiceSinkTest() = default; NiceMock snapshot_; Event::SimulatedTimeSystem time_system_; From fc594c7d63789eec53e5d79afa1ec20a66ba29a7 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 23 Apr 2020 14:44:59 -0700 Subject: [PATCH 5/8] generated Signed-off-by: Jose Nino --- .../envoy/config/metrics/v3/metrics_service.proto | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/generated_api_shadow/envoy/config/metrics/v3/metrics_service.proto b/generated_api_shadow/envoy/config/metrics/v3/metrics_service.proto index 3d6b61a9697ec..dc3426b640789 100644 --- a/generated_api_shadow/envoy/config/metrics/v3/metrics_service.proto +++ b/generated_api_shadow/envoy/config/metrics/v3/metrics_service.proto @@ -4,6 +4,8 @@ package envoy.config.metrics.v3; import "envoy/config/core/v3/grpc_service.proto"; +import "google/protobuf/wrappers.proto"; + import "udpa/annotations/status.proto"; import "udpa/annotations/versioning.proto"; import "validate/validate.proto"; @@ -28,5 +30,5 @@ message MetricsServiceConfig { // If true, counters are reported as the delta between flushing intervals. Otherwise, the current // counter value is reported. Defaults to false. - bool report_counters_as_deltas = 2; + google.protobuf.BoolValue report_counters_as_deltas = 2; } From a1cb22580ba57ab08b50c8ce542662aba0d5426e Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 27 Apr 2020 14:24:10 -0700 Subject: [PATCH 6/8] alpha Signed-off-by: Jose Nino --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 6f087e3d9a77e..cd9551605f4a7 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -27,8 +27,8 @@ Changes to set :ref:`x-request-id ` header in response even if tracing is not forced. * router: allow retries of streaming or incomplete requests. This removes stat `rq_retry_skipped_request_not_complete`. -* stats: added the option to :ref:`report counters as deltas ` to the metrics service stats sink. * router: allow retries by default when upstream responds with :ref:`x-envoy-overloaded `. +* stats: added the option to :ref:`report counters as deltas ` to the metrics service stats sink. * tracing: tracing configuration has been made fully dynamic and every HTTP connection manager can now have a separate :ref:`tracing provider `. * upstream: fixed a bug where Envoy would panic when receiving a GRPC SERVICE_UNKNOWN status on the health check. From 023bf0c4282595e8757254aad1c33468810cab23 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 27 Apr 2020 17:23:54 -0700 Subject: [PATCH 7/8] wrapped value Signed-off-by: Jose Nino --- source/extensions/stat_sinks/metrics_service/config.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/source/extensions/stat_sinks/metrics_service/config.cc b/source/extensions/stat_sinks/metrics_service/config.cc index 4701730a8fd51..69f0860d228b0 100644 --- a/source/extensions/stat_sinks/metrics_service/config.cc +++ b/source/extensions/stat_sinks/metrics_service/config.cc @@ -33,8 +33,9 @@ Stats::SinkPtr MetricsServiceSinkFactory::createStatsSink(const Protobuf::Messag grpc_service, server.stats(), false), server.localInfo()); - return std::make_unique(grpc_metrics_streamer, server.timeSource(), - sink_config.report_counters_as_deltas()); + return std::make_unique( + grpc_metrics_streamer, server.timeSource(), + PROTOBUF_GET_WRAPPED_OR_DEFAULT(sink_config, report_counters_as_deltas, false)); } ProtobufTypes::MessagePtr MetricsServiceSinkFactory::createEmptyConfigProto() { From 1d2c07bc1624d04bf1cd688fea42666e9b940974 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 28 Apr 2020 11:27:52 -0700 Subject: [PATCH 8/8] comments Signed-off-by: Jose Nino --- api/envoy/config/metrics/v3/metrics_service.proto | 2 ++ .../envoy/config/metrics/v3/metrics_service.proto | 2 ++ .../metrics_service/grpc_metrics_service_impl_test.cc | 2 ++ 3 files changed, 6 insertions(+) diff --git a/api/envoy/config/metrics/v3/metrics_service.proto b/api/envoy/config/metrics/v3/metrics_service.proto index dc3426b640789..0e078c0916f8a 100644 --- a/api/envoy/config/metrics/v3/metrics_service.proto +++ b/api/envoy/config/metrics/v3/metrics_service.proto @@ -30,5 +30,7 @@ message MetricsServiceConfig { // If true, counters are reported as the delta between flushing intervals. Otherwise, the current // counter value is reported. Defaults to false. + // Eventually (https://github.com/envoyproxy/envoy/issues/10968) if this value is not set, the + // sink will take updates from the :ref:`MetricsResponse `. google.protobuf.BoolValue report_counters_as_deltas = 2; } diff --git a/generated_api_shadow/envoy/config/metrics/v3/metrics_service.proto b/generated_api_shadow/envoy/config/metrics/v3/metrics_service.proto index dc3426b640789..0e078c0916f8a 100644 --- a/generated_api_shadow/envoy/config/metrics/v3/metrics_service.proto +++ b/generated_api_shadow/envoy/config/metrics/v3/metrics_service.proto @@ -30,5 +30,7 @@ message MetricsServiceConfig { // If true, counters are reported as the delta between flushing intervals. Otherwise, the current // counter value is reported. Defaults to false. + // Eventually (https://github.com/envoyproxy/envoy/issues/10968) if this value is not set, the + // sink will take updates from the :ref:`MetricsResponse `. google.protobuf.BoolValue report_counters_as_deltas = 2; } diff --git a/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc b/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc index f3d07c816aefb..d0e5325607e98 100644 --- a/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc +++ b/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc @@ -153,6 +153,7 @@ TEST_F(MetricsServiceSinkTest, CheckStatsCount) { sink.flush(snapshot_); } +// Test that verifies counters are correctly reported as current value when configured to do so. TEST_F(MetricsServiceSinkTest, ReportCountersValues) { MetricsServiceSink sink(streamer_, time_system_, false); @@ -170,6 +171,7 @@ TEST_F(MetricsServiceSinkTest, ReportCountersValues) { sink.flush(snapshot_); } +// Test that verifies counters are reported as the delta between flushes when configured to do so. TEST_F(MetricsServiceSinkTest, ReportCountersAsDeltas) { MetricsServiceSink sink(streamer_, time_system_, true);