From e5d13ea484bcc1e05298ab7e4a6034eb540cae40 Mon Sep 17 00:00:00 2001 From: qqustc Date: Mon, 22 Jun 2020 18:47:07 -0400 Subject: [PATCH 01/44] Add counter/histogram to record total number of requests and latency metrics in NH (#1) * Create statistics.md * Update benchmark_client_impl.cc * Update benchmark_client_impl.h * Update stream_decoder.cc * Update stream_decoder.h * Update BUILD * Update benchmark_http_client_test.cc * Update test_integration_basics.py * Update stream_decoder_test.cc * local commit 1 Signed-off-by: qqustc Commit from master Signed-off-by: qqustc --- docs/root/statistics.md | 63 +++++++++++++++++++++ source/client/benchmark_client_impl.cc | 15 ++++- source/client/benchmark_client_impl.h | 12 ++-- source/client/stream_decoder.cc | 9 ++- source/client/stream_decoder.h | 1 + test/BUILD | 2 +- test/benchmark_http_client_test.cc | 50 +++++++++++++--- test/integration/test_integration_basics.py | 12 ++-- test/stream_decoder_test.cc | 9 +++ 9 files changed, 152 insertions(+), 21 deletions(-) create mode 100644 docs/root/statistics.md diff --git a/docs/root/statistics.md b/docs/root/statistics.md new file mode 100644 index 000000000..a8926f672 --- /dev/null +++ b/docs/root/statistics.md @@ -0,0 +1,63 @@ +# Nighthawk Statistics + +## Background +Currently Nighthawk only outputs metrics at the end of a test run. There are no metrics streamed during a test run. In order to collect all the useful Nighthawk metrics, we plan to instrument Nighthawk with the functionality to stream its metrics. + + +## Statistics in BenchMarkClient +Per worker Statistics are defined in [benchmark_client_impl.h](https://github.com/envoyproxy/nighthawk/blob/master/source/client/benchmark_client_impl.h) + +Name | Type | Description +-----| ----- | ---------------- +total_req_sent | Counter | Total number of requests sent from Nighthawk +http_xxx | Counter | Total number of response with code xxx +stream_resets | Counter | Total number of sream reset +pool_overflow | Counter | Total number of times connection pool overflowed +pool_connection_failure | Counter | Total number of times pool connection failed +latency_on_success_req | Histogram | Latency (in Microseconds) histogram of successful request with code 2xx +latency_on_error_req | Histogram | Latency (in Microseconds) histogram of error request with code other than 2xx + +## Envoy Metrics Model +Since Nighthawk is built on top of [Envoy](https://github.com/envoyproxy/envoy) and similar metrics have been exported from Envoy, it is natural to follow the example in Envoy. Furthermore *Envoy typed metrics are already being used in Nighthawk* ([example](https://github.com/envoyproxy/nighthawk/blob/master/source/client/benchmark_client_impl.h#L33-L46)). + + +Envoy has 3 types of metrics +- Counters: Unsigned integers (can only increase) represent how many times an event happens, e.g. total number of requests. +- Gauges: Unsigned integers (can increase or decrease), e.g. number of active connections. +- Histograms: Unsigned integers that will yield summarized percentile values. E.g. latency distributions. + + +In Envoy, the stat [Store](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/include/envoy/stats/store.h#L29) is a singleton and provides a simple interface by which the rest of the code can obtain handles to scopes, counters, gauges, and histograms. Envoy counters and gauges are periodically (configured at ~5 sec interval) flushed to the sinks. Note that currently histogram values are sent directly to the sinks. A stat [Sink](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/include/envoy/stats/sink.h#L48) is an interface that takes generic stat data and translates it into a backend-specific wire format. Currently Envoy supports the TCP and UDP [statsd](https://github.com/b/statsd_spec) protocol (implemented in [statsd.h](https://github.com/envoyproxy/envoy/blob/master/source/extensions/stat_sinks/common/statsd/statsd.h)). Users can create their own Sink subclass to translate Envoy metrics into backend-specific format. + +Envoy metrics can be defined using a macro, e.g. +``` +#define ALL_CLUSTER_STATS(COUNTER, GAUGE, HISTOGRAM) + COUNTER(upstream_cx_total) + GAUGE(upstream_cx_active, NeverImport) + HISTOGRAM(upstream_cx_length, Milliseconds) +struct ClusterStats { + ALL_CLUSTER_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT) +}; +``` + +## Envoy Metrics Limitation +Currently Envoy metrics don't support key-value map. As a result, for metrics to be broken down by certain dimensions, we need to define a separate metric for each dimension. For example, currently Nighthawk defines separate [counters](https://github.com/envoyproxy/nighthawk/blob/master/source/client/benchmark_client_impl.h#L35-L40) to monitor the number of responses with corresponding response code. + +Due to this limitation, for the latency metric we define 2 metrics: *latency_on_success_req* (to capture latency of successful requests with 2xx) and *latency_on_error_req* (to capture latency of error requests with code other than 2xx). + + +## Envoy Metrics Flush +Envoy uses a flush timer to periodically flush metrics into stat sinks ([here](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/source/server/server.cc#L479-L480)) at a configured interval (default to 5 sec). For every metric flush, Envoy will call the function [flushMetricsToSinks()](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/source/server/server.cc#L175) to create a metric snapshot from Envoy stat store and flush the snapshot to all sinks through `sink->flush(snapshot)`. + + +## Metrics Export in Nighthawk +Currently a single Nighthawk can run with multiple workers. In the future, Nighthawk will be extended to be able to run multiple instances together. Since each Nighthawk worker sends requests independently, we decide to export per worker level metrics since it provides several advantages over global level metrics (aggregated across all workers). Notice that *there are existing Nighthawk metrics already defined at per worker level* ([example](https://github.com/envoyproxy/nighthawk/blob/bc72a52efdc489beaa0844b34f136e03394bd355/source/client/benchmark_client_impl.cc#L61)). +- Per worker level metrics provide information about the performance of each worker which will be hidden by the global level metrics. +- Keep the workers independent which makes it easier/efficient to scale up to multiple Nighthawks with large numbers of workers. + +Metrics can be defined at per worker level using [Scope](https://github.com/envoyproxy/envoy/blob/e9c2c8c4a0141c9634316e8283f98f412d0dd207/include/envoy/stats/scope.h#L35) ( e.g. `cluster..total_request_sent`). The dynamic portions of metric (e.g. `worker_id`) can be embedded into the metric name. A [TagSpecifier](https://github.com/envoyproxy/envoy/blob/7a652daf35d7d4a6a6bad5a010fe65947ee4411a/api/envoy/config/metrics/v3/stats.proto#L182) can be specified in the bootstrap configuration, which will transform dynamic portions into tags. When per worker level metrics are exported from Nighthawk, multiple per worker level metrics can be converted into a single metric with a `worker_id` label in the stat Sink if the corresponding backend metric supports key-value map. + +## Reference +- [Nighthawk: architecture and key concepts](https://github.com/envoyproxy/nighthawk/blob/master/docs/root/overview.md) +- [Envoy Stats System](https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md) +- [Envoy Stats blog](https://blog.envoyproxy.io/envoy-stats-b65c7f363342) diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index 37902e58f..3f5fbaa5a 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -58,7 +58,8 @@ BenchmarkClientHttpImpl::BenchmarkClientHttpImpl( response_statistic_(std::move(response_statistic)), response_header_size_statistic_(std::move(response_header_size_statistic)), response_body_size_statistic_(std::move(response_body_size_statistic)), use_h2_(use_h2), - benchmark_client_stats_({ALL_BENCHMARK_CLIENT_STATS(POOL_COUNTER(*scope_))}), + benchmark_client_stats_( + {ALL_BENCHMARK_CLIENT_STATS(POOL_COUNTER(*scope_), POOL_HISTOGRAM(*scope_))}), cluster_manager_(cluster_manager), http_tracer_(http_tracer), cluster_name_(std::string(cluster_name)), request_generator_(std::move(request_generator)), provide_resource_backpressure_(provide_resource_backpressure) { @@ -123,6 +124,7 @@ bool BenchmarkClientHttpImpl::tryStartRequest(CompletionCallback caller_completi *response_body_size_statistic_, request->header(), shouldMeasureLatencies(), content_length, generator_, http_tracer_); requests_initiated_++; + benchmark_client_stats_.total_req_sent_.inc(); pool_ptr->newStream(*stream_decoder, *stream_decoder); return true; } @@ -168,5 +170,14 @@ void BenchmarkClientHttpImpl::onPoolFailure(Envoy::Http::ConnectionPool::PoolFai } } +void BenchmarkClientHttpImpl::exportLatency(const uint32_t response_code, + const uint64_t latency_us) { + if (response_code > 199 && response_code <= 299) { + benchmark_client_stats_.latency_on_success_req_.recordValue(latency_us); + } else { + benchmark_client_stats_.latency_on_error_req_.recordValue(latency_us); + } +} + } // namespace Client -} // namespace Nighthawk \ No newline at end of file +} // namespace Nighthawk diff --git a/source/client/benchmark_client_impl.h b/source/client/benchmark_client_impl.h index a40e90a19..2903c935c 100644 --- a/source/client/benchmark_client_impl.h +++ b/source/client/benchmark_client_impl.h @@ -30,7 +30,7 @@ using namespace std::chrono_literals; using namespace Envoy; // We need this because of macro expectations. -#define ALL_BENCHMARK_CLIENT_STATS(COUNTER) \ +#define ALL_BENCHMARK_CLIENT_STATS(COUNTER, HISTOGRAM) \ COUNTER(stream_resets) \ COUNTER(http_1xx) \ COUNTER(http_2xx) \ @@ -39,10 +39,13 @@ using namespace Envoy; // We need this because of macro expectations. COUNTER(http_5xx) \ COUNTER(http_xxx) \ COUNTER(pool_overflow) \ - COUNTER(pool_connection_failure) + COUNTER(pool_connection_failure) \ + COUNTER(total_req_sent) \ + HISTOGRAM(latency_on_success_req, Microseconds) \ + HISTOGRAM(latency_on_error_req, Microseconds) struct BenchmarkClientStats { - ALL_BENCHMARK_CLIENT_STATS(GENERATE_COUNTER_STRUCT) + ALL_BENCHMARK_CLIENT_STATS(GENERATE_COUNTER_STRUCT, GENERATE_HISTOGRAM_STRUCT) }; class Http1PoolImpl : public Envoy::Http::Http1::ProdConnPoolImpl { @@ -104,6 +107,7 @@ class BenchmarkClientHttpImpl : public BenchmarkClient, // StreamDecoderCompletionCallback void onComplete(bool success, const Envoy::Http::ResponseHeaderMap& headers) override; void onPoolFailure(Envoy::Http::ConnectionPool::PoolFailureReason reason) override; + void exportLatency(const uint32_t response_code, const uint64_t latency_us) override; // Helpers Envoy::Http::ConnectionPool::Instance* pool() { @@ -142,4 +146,4 @@ class BenchmarkClientHttpImpl : public BenchmarkClient, }; } // namespace Client -} // namespace Nighthawk \ No newline at end of file +} // namespace Nighthawk diff --git a/source/client/stream_decoder.cc b/source/client/stream_decoder.cc index 96c6295d5..334c2238f 100644 --- a/source/client/stream_decoder.cc +++ b/source/client/stream_decoder.cc @@ -48,7 +48,14 @@ void StreamDecoder::decodeTrailers(Envoy::Http::ResponseTrailerMapPtr&& headers) void StreamDecoder::onComplete(bool success) { ASSERT(!success || complete_); if (success && measure_latencies_) { - latency_statistic_.addValue((time_source_.monotonicTime() - request_start_).count()); + const auto dur = time_source_.monotonicTime() - request_start_; + latency_statistic_.addValue(dur.count()); + // At this point StreamDecoder::decodeHeaders() should have been called. + if (stream_info_.response_code_.has_value()) { + decoder_completion_callback_.exportLatency( + stream_info_.response_code_.value(), + std::chrono::duration_cast(dur).count()); + } } upstream_timing_.onLastUpstreamRxByteReceived(time_source_); response_body_sizes_statistic_.addValue(stream_info_.bytesSent()); diff --git a/source/client/stream_decoder.h b/source/client/stream_decoder.h index 869a8d57e..6edb1b0e0 100644 --- a/source/client/stream_decoder.h +++ b/source/client/stream_decoder.h @@ -24,6 +24,7 @@ class StreamDecoderCompletionCallback { virtual ~StreamDecoderCompletionCallback() = default; virtual void onComplete(bool success, const Envoy::Http::ResponseHeaderMap& headers) PURE; virtual void onPoolFailure(Envoy::Http::ConnectionPool::PoolFailureReason reason) PURE; + virtual void exportLatency(const uint32_t response_code, const uint64_t latency_us) PURE; }; // TODO(oschaaf): create a StreamDecoderPool? diff --git a/test/BUILD b/test/BUILD index 868394027..c6d91d293 100644 --- a/test/BUILD +++ b/test/BUILD @@ -18,9 +18,9 @@ envoy_cc_test( "//test/test_common:environment_lib", "@envoy//source/common/http:header_map_lib_with_external_headers", "@envoy//source/common/network:utility_lib_with_external_headers", - "@envoy//source/common/stats:isolated_store_lib_with_external_headers", "@envoy//source/exe:process_wide_lib_with_external_headers", "@envoy//test/mocks/runtime:runtime_mocks", + "@envoy//test/mocks/stats:stats_mocks", "@envoy//test/mocks/stream_info:stream_info_mocks", "@envoy//test/mocks/thread_local:thread_local_mocks", "@envoy//test/mocks/upstream:upstream_mocks", diff --git a/test/benchmark_http_client_test.cc b/test/benchmark_http_client_test.cc index 0539f487b..f535eed2a 100644 --- a/test/benchmark_http_client_test.cc +++ b/test/benchmark_http_client_test.cc @@ -3,10 +3,10 @@ #include "external/envoy/source/common/http/header_map_impl.h" #include "external/envoy/source/common/network/utility.h" #include "external/envoy/source/common/runtime/runtime_impl.h" -#include "external/envoy/source/common/stats/isolated_store_impl.h" #include "external/envoy/source/exe/process_wide.h" #include "external/envoy/test/mocks/common.h" #include "external/envoy/test/mocks/runtime/mocks.h" +#include "external/envoy/test/mocks/stats/mocks.h" #include "external/envoy/test/mocks/stream_info/mocks.h" #include "external/envoy/test/mocks/thread_local/mocks.h" #include "external/envoy/test/mocks/upstream/mocks.h" @@ -124,7 +124,7 @@ class BenchmarkClientHttpTest : public Test { void setupBenchmarkClient() { client_ = std::make_unique( - *api_, *dispatcher_, store_, std::make_unique(), + *api_, *dispatcher_, mock_store_, std::make_unique(), std::make_unique(), std::make_unique(), std::make_unique(), false, cluster_manager_, http_tracer_, "benchmark", request_generator_, true); @@ -143,7 +143,8 @@ class BenchmarkClientHttpTest : public Test { } Envoy::Event::TestRealTimeSystem time_system_; - Envoy::Stats::IsolatedStoreImpl store_; + // deliverHistogramToSinks() is not implemented in IsolatedStoreImpl so test with a mock store. + Envoy::Stats::MockIsolatedStatsStore mock_store_; Envoy::Api::ApiPtr api_; Envoy::Event::DispatcherPtr dispatcher_; Envoy::Runtime::RandomGeneratorImpl generator_; @@ -166,45 +167,76 @@ TEST_F(BenchmarkClientHttpTest, BasicTestH1200) { response_code_ = "200"; testBasicFunctionality(2, 3, 10); EXPECT_EQ(5, getCounter("http_2xx")); + EXPECT_EQ(5, getCounter("total_req_sent")); } TEST_F(BenchmarkClientHttpTest, BasicTestH1300) { response_code_ = "300"; testBasicFunctionality(0, 11, 10); EXPECT_EQ(10, getCounter("http_3xx")); + EXPECT_EQ(10, getCounter("total_req_sent")); } TEST_F(BenchmarkClientHttpTest, BasicTestH1404) { response_code_ = "404"; testBasicFunctionality(0, 1, 10); EXPECT_EQ(1, getCounter("http_4xx")); + EXPECT_EQ(1, getCounter("total_req_sent")); } TEST_F(BenchmarkClientHttpTest, WeirdStatus) { response_code_ = "601"; testBasicFunctionality(0, 1, 10); EXPECT_EQ(1, getCounter("http_xxx")); + EXPECT_EQ(1, getCounter("total_req_sent")); } TEST_F(BenchmarkClientHttpTest, EnableLatencyMeasurement) { setupBenchmarkClient(); EXPECT_EQ(false, client_->shouldMeasureLatencies()); + EXPECT_CALL(mock_store_, deliverHistogramToSinks(_, _)).Times(0); testBasicFunctionality(10, 1, 10); EXPECT_EQ(0, client_->statistics()["benchmark_http_client.queue_to_connect"]->count()); EXPECT_EQ(0, client_->statistics()["benchmark_http_client.request_to_response"]->count()); + EXPECT_EQ(10, getCounter("total_req_sent")); client_->setShouldMeasureLatencies(true); + EXPECT_CALL(mock_store_, deliverHistogramToSinks(_, _)).Times(10); testBasicFunctionality(10, 1, 10); EXPECT_EQ(10, client_->statistics()["benchmark_http_client.queue_to_connect"]->count()); EXPECT_EQ(10, client_->statistics()["benchmark_http_client.request_to_response"]->count()); + EXPECT_EQ(20, getCounter("total_req_sent")); +} + +TEST_F(BenchmarkClientHttpTest, ExportSuccessLatency) { + setupBenchmarkClient(); + uint64_t latency = 10; + EXPECT_CALL(mock_store_, deliverHistogramToSinks(Property(&Envoy::Stats::Metric::name, + "benchmark.latency_on_success_req"), + latency)) + .Times(1); + EXPECT_CALL(mock_store_, + deliverHistogramToSinks( + Property(&Envoy::Stats::Metric::name, "benchmark.latency_on_error_req"), latency)) + .Times(0); + client_->exportLatency(/*response_code=*/200, /*latency_us=*/latency); +} + +TEST_F(BenchmarkClientHttpTest, ExportErrorLatency) { + setupBenchmarkClient(); + uint64_t latency = 10; + EXPECT_CALL(mock_store_, deliverHistogramToSinks(Property(&Envoy::Stats::Metric::name, + "benchmark.latency_on_success_req"), + latency)) + .Times(0); + EXPECT_CALL(mock_store_, + deliverHistogramToSinks( + Property(&Envoy::Stats::Metric::name, "benchmark.latency_on_error_req"), latency)) + .Times(1); + client_->exportLatency(/*response_code=*/500, /*latency_us=*/latency); } TEST_F(BenchmarkClientHttpTest, StatusTrackingInOnComplete) { - auto store = std::make_unique(); - client_ = std::make_unique( - *api_, *dispatcher_, *store, std::make_unique(), - std::make_unique(), std::make_unique(), - std::make_unique(), false, cluster_manager_, http_tracer_, "foo", - request_generator_, true); + setupBenchmarkClient(); Envoy::Http::ResponseHeaderMapPtr header = Envoy::Http::ResponseHeaderMapImpl::create(); header->setStatus(1); diff --git a/test/integration/test_integration_basics.py b/test/integration/test_integration_basics.py index df02ef193..bcd57e093 100644 --- a/test/integration/test_integration_basics.py +++ b/test/integration/test_integration_basics.py @@ -34,6 +34,7 @@ def test_http_h1(http_test_server_fixture): assertCounterEqual(counters, "upstream_rq_pending_total", 1) assertCounterEqual(counters, "upstream_rq_total", 25) assertCounterEqual(counters, "default.total_match_count", 1) + assertCounterEqual(counters, "benchmark.total_req_sent", 25) global_histograms = http_test_server_fixture.getNighthawkGlobalHistogramsbyIdFromJson(parsed_json) assertEqual(int(global_histograms["benchmark_http_client.response_body_size"]["count"]), 25) @@ -47,7 +48,7 @@ def test_http_h1(http_test_server_fixture): assertEqual(int(global_histograms["benchmark_http_client.response_body_size"]["raw_pstdev"]), 0) assertEqual(int(global_histograms["benchmark_http_client.response_header_size"]["raw_pstdev"]), 0) - assertEqual(len(counters), 12) + assertEqual(len(counters), 13) def mini_stress_test(fixture, args): @@ -179,7 +180,8 @@ def test_http_h2(http_test_server_fixture): assertCounterEqual(counters, "upstream_rq_pending_total", 1) assertCounterEqual(counters, "upstream_rq_total", 25) assertCounterEqual(counters, "default.total_match_count", 1) - assertEqual(len(counters), 12) + assertCounterEqual(counters, "benchmark.total_req_sent", 25) + assertEqual(len(counters), 13) def test_http_concurrency(http_test_server_fixture): @@ -224,7 +226,8 @@ def test_https_h1(https_test_server_fixture): assertCounterEqual(counters, "ssl.sigalgs.rsa_pss_rsae_sha256", 1) assertCounterEqual(counters, "ssl.versions.TLSv1.2", 1) assertCounterEqual(counters, "default.total_match_count", 1) - assertEqual(len(counters), 17) + assertCounterEqual(counters, "benchmark.total_req_sent", 25) + assertEqual(len(counters), 18) server_stats = https_test_server_fixture.getTestServerStatisticsJson() assertEqual( @@ -258,7 +261,8 @@ def test_https_h2(https_test_server_fixture): assertCounterEqual(counters, "ssl.sigalgs.rsa_pss_rsae_sha256", 1) assertCounterEqual(counters, "ssl.versions.TLSv1.2", 1) assertCounterEqual(counters, "default.total_match_count", 1) - assertEqual(len(counters), 17) + assertCounterEqual(counters, "benchmark.total_req_sent", 25) + assertEqual(len(counters), 18) def test_https_h2_multiple_connections(https_test_server_fixture): diff --git a/test/stream_decoder_test.cc b/test/stream_decoder_test.cc index 93cb5333f..9327bbecd 100644 --- a/test/stream_decoder_test.cc +++ b/test/stream_decoder_test.cc @@ -37,6 +37,9 @@ class StreamDecoderTest : public Test, public StreamDecoderCompletionCallback { stream_decoder_completion_callbacks_++; } void onPoolFailure(Envoy::Http::ConnectionPool::PoolFailureReason) override { pool_failures_++; } + void exportLatency(const uint32_t, const uint64_t) override { + stream_decoder_export_latency_callbacks_++; + } Envoy::Event::TestRealTimeSystem time_system_; Envoy::Stats::IsolatedStoreImpl store_; @@ -49,6 +52,7 @@ class StreamDecoderTest : public Test, public StreamDecoderCompletionCallback { HeaderMapPtr request_headers_; uint64_t stream_decoder_completion_callbacks_{0}; uint64_t pool_failures_{0}; + uint64_t stream_decoder_export_latency_callbacks_{0}; Envoy::Runtime::RandomGeneratorImpl random_generator_; Envoy::Tracing::HttpTracerSharedPtr http_tracer_; Envoy::Http::ResponseHeaderMapPtr test_header_; @@ -64,6 +68,7 @@ TEST_F(StreamDecoderTest, HeaderOnlyTest) { decoder->decodeHeaders(std::move(test_header_), true); EXPECT_TRUE(is_complete); EXPECT_EQ(1, stream_decoder_completion_callbacks_); + EXPECT_EQ(0, stream_decoder_export_latency_callbacks_); } TEST_F(StreamDecoderTest, HeaderWithBodyTest) { @@ -112,6 +117,7 @@ TEST_F(StreamDecoderTest, LatencyIsNotMeasured) { decoder->decodeHeaders(std::move(test_header_), true); EXPECT_EQ(0, connect_statistic_.count()); EXPECT_EQ(0, latency_statistic_.count()); + EXPECT_EQ(0, stream_decoder_export_latency_callbacks_); } TEST_F(StreamDecoderTest, LatencyIsMeasured) { @@ -146,9 +152,11 @@ TEST_F(StreamDecoderTest, LatencyIsMeasured) { decoder->onPoolReady(stream_encoder, ptr, stream_info); EXPECT_EQ(1, connect_statistic_.count()); decoder->decodeHeaders(std::move(test_header_), false); + EXPECT_EQ(0, stream_decoder_export_latency_callbacks_); decoder->decodeTrailers(std::move(test_trailer_)); EXPECT_EQ(1, connect_statistic_.count()); EXPECT_EQ(1, latency_statistic_.count()); + EXPECT_EQ(1, stream_decoder_export_latency_callbacks_); } TEST_F(StreamDecoderTest, StreamResetTest) { @@ -161,6 +169,7 @@ TEST_F(StreamDecoderTest, StreamResetTest) { decoder->onResetStream(Envoy::Http::StreamResetReason::LocalReset, "fooreason"); EXPECT_TRUE(is_complete); // these do get reported. EXPECT_EQ(1, stream_decoder_completion_callbacks_); + EXPECT_EQ(0, stream_decoder_export_latency_callbacks_); } TEST_F(StreamDecoderTest, PoolFailureTest) { From 37ef0dbc30174407297e291251482ce9ab468910 Mon Sep 17 00:00:00 2001 From: qqustc Date: Tue, 23 Jun 2020 14:14:49 -0400 Subject: [PATCH 02/44] Update stream_decoder.cc Signed-off-by: qqustc Signed-off-by: qqustc --- source/client/stream_decoder.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/client/stream_decoder.cc b/source/client/stream_decoder.cc index 334c2238f..a6574f9c0 100644 --- a/source/client/stream_decoder.cc +++ b/source/client/stream_decoder.cc @@ -48,13 +48,13 @@ void StreamDecoder::decodeTrailers(Envoy::Http::ResponseTrailerMapPtr&& headers) void StreamDecoder::onComplete(bool success) { ASSERT(!success || complete_); if (success && measure_latencies_) { - const auto dur = time_source_.monotonicTime() - request_start_; - latency_statistic_.addValue(dur.count()); + latency_statistic_.addValue((time_source_.monotonicTime() - request_start_).count()); // At this point StreamDecoder::decodeHeaders() should have been called. if (stream_info_.response_code_.has_value()) { + const uint64_t latency_us = + std::chrono::duration_cast(time_source_.monotonicTime() - request_start_).count(); decoder_completion_callback_.exportLatency( - stream_info_.response_code_.value(), - std::chrono::duration_cast(dur).count()); + stream_info_.response_code_.value(), latency_us); } } upstream_timing_.onLastUpstreamRxByteReceived(time_source_); From ecc9f1a463ca5e8f4c172f8bc0b462477077dd56 Mon Sep 17 00:00:00 2001 From: qqustc Date: Wed, 24 Jun 2020 12:09:51 -0400 Subject: [PATCH 03/44] Fix format source/client/stream_decoder.cc Signed-off-by: qqustc --- source/client/stream_decoder.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/client/stream_decoder.cc b/source/client/stream_decoder.cc index a6574f9c0..af25d6aaf 100644 --- a/source/client/stream_decoder.cc +++ b/source/client/stream_decoder.cc @@ -51,10 +51,10 @@ void StreamDecoder::onComplete(bool success) { latency_statistic_.addValue((time_source_.monotonicTime() - request_start_).count()); // At this point StreamDecoder::decodeHeaders() should have been called. if (stream_info_.response_code_.has_value()) { - const uint64_t latency_us = - std::chrono::duration_cast(time_source_.monotonicTime() - request_start_).count(); - decoder_completion_callback_.exportLatency( - stream_info_.response_code_.value(), latency_us); + const uint64_t latency_us = std::chrono::duration_cast( + time_source_.monotonicTime() - request_start_) + .count(); + decoder_completion_callback_.exportLatency(stream_info_.response_code_.value(), latency_us); } } upstream_timing_.onLastUpstreamRxByteReceived(time_source_); From af1ea54b0dd39f57a7e0a828160973d8709eec54 Mon Sep 17 00:00:00 2001 From: qqustc Date: Tue, 30 Jun 2020 22:10:12 -0400 Subject: [PATCH 04/44] Add new NH statistic class CircllhistStatistic and SinkableCircllhistStatistic Signed-off-by: Qin Qin Signed-off-by: qqustc Signed-off-by: Qin Qin --- source/common/BUILD | 2 +- source/common/statistic_impl.cc | 37 +++++++++- source/common/statistic_impl.h | 86 ++++++++++++++++++++++- test/BUILD | 6 +- test/statistic_test.cc | 79 +++++++++++++++++++-- test/test_data/circllhist_proto_json.gold | 60 ++++++++++++++++ 6 files changed, 261 insertions(+), 9 deletions(-) create mode 100644 test/test_data/circllhist_proto_json.gold diff --git a/source/common/BUILD b/source/common/BUILD index f576ced60..c77ede286 100644 --- a/source/common/BUILD +++ b/source/common/BUILD @@ -109,7 +109,7 @@ envoy_cc_library( "@envoy//source/common/http:utility_lib_with_external_headers", "@envoy//source/common/network:utility_lib_with_external_headers", "@envoy//source/common/protobuf:utility_lib_with_external_headers", - "@envoy//source/common/stats:stats_lib_with_external_headers", + "@envoy//source/common/stats:histogram_lib_with_external_headers", "@envoy//source/exe:envoy_common_lib_with_external_headers", "@envoy//source/server/config_validation:server_lib_with_external_headers", ], diff --git a/source/common/statistic_impl.cc b/source/common/statistic_impl.cc index bd5750467..8179babc5 100644 --- a/source/common/statistic_impl.cc +++ b/source/common/statistic_impl.cc @@ -236,4 +236,39 @@ nighthawk::client::Statistic HdrStatistic::toProto(SerializationDomain domain) c return proto; } -} // namespace Nighthawk \ No newline at end of file +StatisticPtr CircllhistStatistic::combine(const Statistic& statistic) const { + auto combined = std::make_unique(); + const auto& b = dynamic_cast(statistic); + hist_accumulate(combined->histogram_, &this->histogram_, 1); + hist_accumulate(combined->histogram_, &b.histogram_, 1); + + combined->min_ = std::min(this->min(), b.min()); + combined->max_ = std::max(this->max(), b.max()); + combined->count_ = this->count() + b.count(); + return combined; +} + +nighthawk::client::Statistic CircllhistStatistic::toProto(SerializationDomain domain) const { + nighthawk::client::Statistic proto = StatisticImpl::toProto(domain); + if (count() == 0) + return proto; + + std::vector quantiles{0, 0.25, 0.5, 0.75, 0.90, 0.95, 0.99, 0.995, 0.999, 1}; + std::vector computed_quantiles(quantiles.size(), 0.0); + hist_approx_quantile(histogram_, quantiles.data(), quantiles.size(), computed_quantiles.data()); + for (size_t i = 0; i < quantiles.size(); i++) { + nighthawk::client::Percentile* percentile = proto.add_percentiles(); + if (domain == Statistic::SerializationDomain::DURATION) { + setDurationFromNanos(*percentile->mutable_duration(), + static_cast(computed_quantiles[i])); + } else { + percentile->set_raw_value(computed_quantiles[i]); + } + percentile->set_percentile(quantiles[i]); + percentile->set_count(hist_approx_count_below(histogram_, computed_quantiles[i])); + } + + return proto; +} + +} // namespace Nighthawk diff --git a/source/common/statistic_impl.h b/source/common/statistic_impl.h index 981133b44..05d0af821 100644 --- a/source/common/statistic_impl.h +++ b/source/common/statistic_impl.h @@ -7,6 +7,7 @@ #include "external/dep_hdrhistogram_c/src/hdr_histogram.h" #include "external/envoy/source/common/common/logger.h" +#include "external/envoy/source/common/stats/histogram_impl.h" #include "common/frequency.h" @@ -150,4 +151,87 @@ class HdrStatistic : public StatisticImpl { struct hdr_histogram* histogram_; }; -} // namespace Nighthawk \ No newline at end of file +/** + * CircllhistStatistic uses Circllhist under the hood to compute statistics. + * Circllhist is used in the implementation of Envoy Histograms, compared to HdrHistogram it trades + * precision for fast performance in merge and insertion. For more info, please see + * https://github.com/circonus-labs/libcircllhist + */ +class CircllhistStatistic : public StatisticImpl { +public: + CircllhistStatistic() { + histogram_ = hist_alloc(); + ASSERT(histogram_ != nullptr); + } + ~CircllhistStatistic() override { hist_free(histogram_); } + + void addValue(uint64_t value) override { + hist_insert_intscale(histogram_, value, 0, 1); + StatisticImpl::addValue(value); + } + double mean() const override { return hist_approx_mean(histogram_); } + double pvariance() const override { return pstdev() * pstdev(); } + double pstdev() const override { + return count() == 0 ? std::nan("") : hist_approx_stddev(histogram_); + } + StatisticPtr combine(const Statistic& statistic) const override; + uint64_t significantDigits() const override { return 1; } + StatisticPtr createNewInstanceOfSameType() const override { + return std::make_unique(); + } + nighthawk::client::Statistic toProto(SerializationDomain domain) const override; + +private: + histogram_t* histogram_; +}; + +/** + * In order to be able to flush histogram value to downstream Envoy stats Sinks, Per worker + * SinkableCircllhistStatistic takes the Scope reference in the constructor and wraps the + * Envoy::Stats::Histogram interface. + */ +class SinkableCircllhistStatistic : public CircllhistStatistic, + public Envoy::Stats::HistogramImplHelper { +public: + // Calling HistogramImplHelper(SymbolTable& symbol_table) constructor to construct an empty + // MetricImpl. This is to bypass the complicated logic of setting up SymbolTable/StatName in + // Envoy. Instead, SinkableCircllhistStatistic overrides name() and tagExtractedName() method to + // return Nighthawk::Statistic::id(). + SinkableCircllhistStatistic(Envoy::Stats::Scope& scope, + const absl::optional worker_id = absl::nullopt) + : CircllhistStatistic(), Envoy::Stats::HistogramImplHelper(scope.symbolTable()), + scope_(scope), worker_id_(worker_id) {} + + ~SinkableCircllhistStatistic() override { + // We must explicitly free the StatName here in order to supply the + // SymbolTable reference. + MetricImpl::clear(symbolTable()); + } + + // Envoy::Stats::Histogram + void recordValue(uint64_t value) override { + addValue(value); + // Currently in Envoy Scope implementation, deliverHistogramToSinks() will flush the histogram + // value directly to stats Sinks. + scope_.deliverHistogramToSinks(*this, value); + } + Envoy::Stats::Histogram::Unit unit() const override { + return Envoy::Stats::Histogram::Unit::Unspecified; + }; + bool used() const override { return count() > 0; } + Envoy::Stats::SymbolTable& symbolTable() override { return scope_.symbolTable(); } + // Overriding name() and tagExtractedName() method in Envoy::Stats::MetricImpl to return + // Statistic::id(). + std::string name() const override { return id(); } + std::string tagExtractedName() const override { return id(); } + + const absl::optional worker_id() { return worker_id_; } + +private: + // This is used for delivering the histogram data to sinks. + Envoy::Stats::Scope& scope_; + // worker_id can be used in downstream stats Sinks as the stats tag. + absl::optional worker_id_; +}; + +} // namespace Nighthawk diff --git a/test/BUILD b/test/BUILD index 868394027..f233f75f8 100644 --- a/test/BUILD +++ b/test/BUILD @@ -195,13 +195,17 @@ envoy_cc_test( envoy_cc_test( name = "statistic_test", srcs = ["statistic_test.cc"], - data = ["test_data/hdr_proto_json.gold"], + data = [ + "test_data/circllhist_proto_json.gold", + "test_data/hdr_proto_json.gold", + ], repository = "@envoy", deps = [ "//source/common:nighthawk_common_lib", "//test/test_common:environment_lib", "@envoy//source/common/protobuf:utility_lib_with_external_headers", "@envoy//source/common/stats:isolated_store_lib_with_external_headers", + "@envoy//test/mocks/stats:stats_mocks", ], ) diff --git a/test/statistic_test.cc b/test/statistic_test.cc index c73e1d6b8..dfee28829 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -2,10 +2,12 @@ #include #include +#include #include // std::bad_cast #include "external/envoy/source/common/protobuf/utility.h" #include "external/envoy/source/common/stats/isolated_store_impl.h" +#include "external/envoy/test/mocks/stats/mocks.h" #include "external/envoy/test/test_common/file_system_for_test.h" #include "external/envoy/test/test_common/utility.h" @@ -20,7 +22,8 @@ using namespace testing; namespace Nighthawk { -using MyTypes = Types; +using MyTypes = Types; template class TypedStatisticTest : public Test {}; @@ -116,15 +119,15 @@ TYPED_TEST(TypedStatisticTest, SingleAndDoubleValue) { a.addValue(1); EXPECT_EQ(1, a.count()); - EXPECT_DOUBLE_EQ(1, a.mean()); + Helper::expectNear(1.0, a.mean(), a.significantDigits()); EXPECT_DOUBLE_EQ(0, a.pvariance()); EXPECT_DOUBLE_EQ(0, a.pstdev()); a.addValue(2); EXPECT_EQ(2, a.count()); - EXPECT_DOUBLE_EQ(1.5, a.mean()); - EXPECT_DOUBLE_EQ(0.25, a.pvariance()); - EXPECT_DOUBLE_EQ(0.5, a.pstdev()); + Helper::expectNear(1.5, a.mean(), a.significantDigits()); + Helper::expectNear(0.25, a.pvariance(), a.significantDigits()); + Helper::expectNear(0.5, a.pstdev(), a.significantDigits()); } TYPED_TEST(TypedStatisticTest, CatastrophicalCancellation) { @@ -269,6 +272,19 @@ TEST(StatisticTest, StreamingStatProtoOutputLargeValues) { EXPECT_EQ(proto.pstdev().nanos(), 0); } +TEST(StatisticTest, CircllhistStatisticProtoOutputLargeValues) { + CircllhistStatistic a; + uint64_t value = 100ul + 0xFFFFFFFF; + a.addValue(value); + a.addValue(value); + const nighthawk::client::Statistic proto = a.toProto(Statistic::SerializationDomain::DURATION); + + EXPECT_EQ(proto.count(), 2); + Helper::expectNear(((1.0 * proto.mean().seconds() * 1000 * 1000 * 1000) + proto.mean().nanos()), + value, a.significantDigits()); + EXPECT_EQ(proto.pstdev().nanos(), 0); +} + TEST(StatisticTest, HdrStatisticPercentilesProto) { nighthawk::client::Statistic parsed_json_proto; HdrStatistic statistic; @@ -289,16 +305,39 @@ TEST(StatisticTest, HdrStatisticPercentilesProto) { EXPECT_EQ(json, golden_json); } +TEST(StatisticTest, CircllhistStatisticPercentilesProto) { + nighthawk::client::Statistic parsed_json_proto; + CircllhistStatistic statistic; + + for (int i = 1; i <= 10; i++) { + statistic.addValue(i); + } + + Envoy::MessageUtil util; + util.loadFromJson(Envoy::Filesystem::fileSystemForTest().fileReadToEnd( + TestEnvironment::runfilesPath("test/test_data/circllhist_proto_json.gold")), + parsed_json_proto, Envoy::ProtobufMessage::getStrictValidationVisitor()); + // Instead of comparing proto's, we perform a string-based comparison, because that emits a + // helpful diff when this fails. + const std::string json = util.getJsonStringFromMessage( + statistic.toProto(Statistic::SerializationDomain::DURATION), true, true); + const std::string golden_json = util.getJsonStringFromMessage(parsed_json_proto, true, true); + EXPECT_EQ(json, golden_json); +} + TEST(StatisticTest, CombineAcrossTypesFails) { HdrStatistic a; InMemoryStatistic b; StreamingStatistic c; + CircllhistStatistic d; EXPECT_THROW(a.combine(b), std::bad_cast); EXPECT_THROW(a.combine(c), std::bad_cast); EXPECT_THROW(b.combine(a), std::bad_cast); EXPECT_THROW(b.combine(c), std::bad_cast); EXPECT_THROW(c.combine(a), std::bad_cast); EXPECT_THROW(c.combine(b), std::bad_cast); + EXPECT_THROW(c.combine(d), std::bad_cast); + EXPECT_THROW(d.combine(a), std::bad_cast); } TEST(StatisticTest, IdFieldWorks) { @@ -328,4 +367,34 @@ TEST(StatisticTest, NullStatistic) { EXPECT_NE(nullptr, stat.createNewInstanceOfSameType()); } +TEST(StatisticTest, EmptySinkableCircllhistStatistic) { + Envoy::Stats::MockIsolatedStatsStore mock_store; + SinkableCircllhistStatistic stat(mock_store); + EXPECT_EQ(Envoy::Stats::Histogram::Unit::Unspecified, stat.unit()); + EXPECT_FALSE(stat.used()); + EXPECT_EQ("", stat.name()); + EXPECT_EQ("", stat.tagExtractedName()); + EXPECT_EQ(absl::nullopt, stat.worker_id()); +} + +TEST(StatisticTest, SimpleSinkableCircllhistStatistic) { + Envoy::Stats::MockIsolatedStatsStore mock_store; + const int worker_id = 0; + SinkableCircllhistStatistic stat(mock_store, worker_id); + + const uint64_t sample_value = 123; + const std::string stat_name = "stat_name"; + + EXPECT_CALL(mock_store, deliverHistogramToSinks(_, sample_value)).Times(1); + stat.recordValue(sample_value); + + stat.setId(stat_name); + EXPECT_EQ(Envoy::Stats::Histogram::Unit::Unspecified, stat.unit()); + EXPECT_TRUE(stat.used()); + EXPECT_EQ(stat_name, stat.name()); + EXPECT_EQ(stat_name, stat.tagExtractedName()); + EXPECT_TRUE(stat.worker_id().has_value()); + EXPECT_EQ(worker_id, stat.worker_id().value()); +} + } // namespace Nighthawk diff --git a/test/test_data/circllhist_proto_json.gold b/test/test_data/circllhist_proto_json.gold new file mode 100644 index 000000000..aeb0bb676 --- /dev/null +++ b/test/test_data/circllhist_proto_json.gold @@ -0,0 +1,60 @@ +{ + "count": "10", + "id": "", + "percentiles": [ + { + "percentile": 0, + "count": "0", + "duration": "0.000000001s" + }, + { + "percentile": 0.25, + "count": "2", + "duration": "0.000000003s" + }, + { + "percentile": 0.5, + "count": "5", + "duration": "0.000000005s" + }, + { + "percentile": 0.75, + "count": "7", + "duration": "0.000000008s" + }, + { + "percentile": 0.9, + "count": "9", + "duration": "0.000000009s" + }, + { + "percentile": 0.95, + "count": "9", + "duration": "0.000000010s" + }, + { + "percentile": 0.99, + "count": "9", + "duration": "0.000000010s" + }, + { + "percentile": 0.995, + "count": "9", + "duration": "0.000000010s" + }, + { + "percentile": 0.999, + "count": "9", + "duration": "0.000000010s" + }, + { + "percentile": 1, + "count": "10", + "duration": "0.000000011s" + } + ], + "mean": "0.000000006s", + "pstdev": "0.000000003s", + "min": "0.000000001s", + "max": "0.000000010s" +} From 43d7ee2487fcaec22991519a011d2d753688cf2d Mon Sep 17 00:00:00 2001 From: qqustc Date: Mon, 6 Jul 2020 17:07:21 -0400 Subject: [PATCH 05/44] Update statistic_impl.h Signed-off-by: Qin Qin --- source/common/statistic_impl.h | 83 +++++++++++++++++++++++----------- 1 file changed, 57 insertions(+), 26 deletions(-) diff --git a/source/common/statistic_impl.h b/source/common/statistic_impl.h index 05d0af821..95a0da7af 100644 --- a/source/common/statistic_impl.h +++ b/source/common/statistic_impl.h @@ -186,28 +186,52 @@ class CircllhistStatistic : public StatisticImpl { }; /** - * In order to be able to flush histogram value to downstream Envoy stats Sinks, Per worker - * SinkableCircllhistStatistic takes the Scope reference in the constructor and wraps the - * Envoy::Stats::Histogram interface. + * In order to be able to flush a histogram value to downstream Envoy stats Sinks, abstract class SinkableStatistic takes the Scope reference in the constructor and wraps the + * Envoy::Stats::HistogramHelper interface. Implementation of sinkable Nighthawk Statistic class will inherit from this class. */ -class SinkableCircllhistStatistic : public CircllhistStatistic, - public Envoy::Stats::HistogramImplHelper { -public: +class SinkableStatistic : public Envoy::Stats::HistogramImplHelper { + public: // Calling HistogramImplHelper(SymbolTable& symbol_table) constructor to construct an empty // MetricImpl. This is to bypass the complicated logic of setting up SymbolTable/StatName in - // Envoy. Instead, SinkableCircllhistStatistic overrides name() and tagExtractedName() method to - // return Nighthawk::Statistic::id(). - SinkableCircllhistStatistic(Envoy::Stats::Scope& scope, - const absl::optional worker_id = absl::nullopt) - : CircllhistStatistic(), Envoy::Stats::HistogramImplHelper(scope.symbolTable()), - scope_(scope), worker_id_(worker_id) {} + // Envoy. + SinkableStatistic(Envoy::Stats::Scope& scope, + const absl::optional worker_id) + : Envoy::Stats::HistogramImplHelper(scope.symbolTable()), + scope_(scope), + worker_id_(worker_id) {} - ~SinkableCircllhistStatistic() override { + ~SinkableStatistic() override { // We must explicitly free the StatName here in order to supply the // SymbolTable reference. - MetricImpl::clear(symbolTable()); + MetricImpl::clear(scope_.symbolTable()); + } + + // Currently Envoy Histogram Unit supports {Unspecified, Bytes, Microseconds, Milliseconds}. By default, Nighthawk::Statistic uses nanosecond as the unit of latency histograms, so Unspecified is returned here to isolate Nighthawk Statistic from Envoy Histogram Unit. + Envoy::Stats::Histogram::Unit unit() const override { + return Envoy::Stats::Histogram::Unit::Unspecified; + }; + Envoy::Stats::SymbolTable& symbolTable() override { + return scope_.symbolTable(); } + const absl::optional worker_id() { return worker_id_; } + + protected: + // This is used for delivering the histogram data to sinks. + Envoy::Stats::Scope& scope_; + + private: + // worker_id can be used in downstream stats Sinks as the stats tag. + absl::optional worker_id_; +}; + +// Implementation of sinkable Nighthawk Statistic with HdrHistogram. +class SinkableHdrStatistic : public SinkableStatistic, public HdrStatistic { + public: + SinkableHdrStatistic(Envoy::Stats::Scope& scope, + const absl::optional worker_id = absl::nullopt) + : SinkableStatistic(scope, worker_id) {} + // Envoy::Stats::Histogram void recordValue(uint64_t value) override { addValue(value); @@ -215,23 +239,30 @@ class SinkableCircllhistStatistic : public CircllhistStatistic, // value directly to stats Sinks. scope_.deliverHistogramToSinks(*this, value); } - Envoy::Stats::Histogram::Unit unit() const override { - return Envoy::Stats::Histogram::Unit::Unspecified; - }; bool used() const override { return count() > 0; } - Envoy::Stats::SymbolTable& symbolTable() override { return scope_.symbolTable(); } - // Overriding name() and tagExtractedName() method in Envoy::Stats::MetricImpl to return - // Statistic::id(). + // Overriding name() and tagExtractedName() method to return Nighthawk::Statistic::id(). std::string name() const override { return id(); } std::string tagExtractedName() const override { return id(); } +}; - const absl::optional worker_id() { return worker_id_; } +// Implementation of sinkable Nighthawk Statistic with Circllhist Histogram. +class SinkableCircllhistStatistic : public SinkableStatistic, public CircllhistStatistic { + public: + SinkableCircllhistStatistic(Envoy::Stats::Scope& scope, + const absl::optional worker_id = absl::nullopt) + : SinkableStatistic(scope, worker_id) {} -private: - // This is used for delivering the histogram data to sinks. - Envoy::Stats::Scope& scope_; - // worker_id can be used in downstream stats Sinks as the stats tag. - absl::optional worker_id_; + // Envoy::Stats::Histogram + void recordValue(uint64_t value) override { + addValue(value); + // Currently in Envoy Scope implementation, deliverHistogramToSinks() will flush the histogram + // value directly to stats Sinks. + scope_.deliverHistogramToSinks(*this, value); + } + bool used() const override { return count() > 0; } + // Overriding name() and tagExtractedName() method to return Nighthawk::Statistic::id(). + std::string name() const override { return id(); } + std::string tagExtractedName() const override { return id(); } }; } // namespace Nighthawk From 77249b978ec1ed5c546fe034f2209f667c8966ed Mon Sep 17 00:00:00 2001 From: qqustc Date: Mon, 6 Jul 2020 17:09:18 -0400 Subject: [PATCH 06/44] Update statistic_impl.cc Signed-off-by: Qin Qin --- source/common/statistic_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/statistic_impl.cc b/source/common/statistic_impl.cc index 8179babc5..41f6be9c6 100644 --- a/source/common/statistic_impl.cc +++ b/source/common/statistic_impl.cc @@ -253,7 +253,7 @@ nighthawk::client::Statistic CircllhistStatistic::toProto(SerializationDomain do if (count() == 0) return proto; - std::vector quantiles{0, 0.25, 0.5, 0.75, 0.90, 0.95, 0.99, 0.995, 0.999, 1}; + std::vector quantiles{0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.55, 0.6, 0.65, 0.7, 0.75, 0.775, 0.8, 0.825, 0.85, 0.875, 0.90, 0.925, 0.95, 0.975, 0.99, 0.995, 0.999, 1}; std::vector computed_quantiles(quantiles.size(), 0.0); hist_approx_quantile(histogram_, quantiles.data(), quantiles.size(), computed_quantiles.data()); for (size_t i = 0; i < quantiles.size(); i++) { From bdecd693d83857f9702acbc51a6d793a835cf859 Mon Sep 17 00:00:00 2001 From: qqustc Date: Mon, 6 Jul 2020 17:31:27 -0400 Subject: [PATCH 07/44] Update statistic_test.cc Signed-off-by: Qin Qin --- test/statistic_test.cc | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/test/statistic_test.cc b/test/statistic_test.cc index dfee28829..1ea9b78b7 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -273,16 +273,15 @@ TEST(StatisticTest, StreamingStatProtoOutputLargeValues) { } TEST(StatisticTest, CircllhistStatisticProtoOutputLargeValues) { - CircllhistStatistic a; + CircllhistStatistic statistic; uint64_t value = 100ul + 0xFFFFFFFF; - a.addValue(value); - a.addValue(value); - const nighthawk::client::Statistic proto = a.toProto(Statistic::SerializationDomain::DURATION); + statistic.addValue(value); + statistic.addValue(value); + const nighthawk::client::Statistic proto = statistic.toProto(Statistic::SerializationDomain::DURATION); EXPECT_EQ(proto.count(), 2); - Helper::expectNear(((1.0 * proto.mean().seconds() * 1000 * 1000 * 1000) + proto.mean().nanos()), - value, a.significantDigits()); - EXPECT_EQ(proto.pstdev().nanos(), 0); + Helper::expectNear(Envoy::Protobuf::util::TimeUtil::DurationToNanoseconds(proto.mean()), value, statistic.significantDigits()); + EXPECT_EQ(Envoy::Protobuf::util::TimeUtil::DurationToNanoseconds(proto.pstdev()), 0); } TEST(StatisticTest, HdrStatisticPercentilesProto) { @@ -367,9 +366,15 @@ TEST(StatisticTest, NullStatistic) { EXPECT_NE(nullptr, stat.createNewInstanceOfSameType()); } -TEST(StatisticTest, EmptySinkableCircllhistStatistic) { +using SinkableTypes = Types; + +template class SinkableStatisticTest : public Test {}; + +TYPED_TEST_SUITE(SinkableStatisticTest, SinkableTypes); + +TYPED_TEST(SinkableStatisticTest, EmptySinkableStatistic) { Envoy::Stats::MockIsolatedStatsStore mock_store; - SinkableCircllhistStatistic stat(mock_store); + TypeParam stat(mock_store); EXPECT_EQ(Envoy::Stats::Histogram::Unit::Unspecified, stat.unit()); EXPECT_FALSE(stat.used()); EXPECT_EQ("", stat.name()); @@ -377,10 +382,10 @@ TEST(StatisticTest, EmptySinkableCircllhistStatistic) { EXPECT_EQ(absl::nullopt, stat.worker_id()); } -TEST(StatisticTest, SimpleSinkableCircllhistStatistic) { +TYPED_TEST(SinkableStatisticTest, SimpleSinkableStatistic) { Envoy::Stats::MockIsolatedStatsStore mock_store; const int worker_id = 0; - SinkableCircllhistStatistic stat(mock_store, worker_id); + TypeParam stat(mock_store, worker_id); const uint64_t sample_value = 123; const std::string stat_name = "stat_name"; From 2c14561fab9402dd92dd652ae335b5b083634d4c Mon Sep 17 00:00:00 2001 From: qqustc Date: Mon, 6 Jul 2020 17:52:57 -0400 Subject: [PATCH 08/44] Update circllhist_proto_json.gold Signed-off-by: Qin Qin --- test/test_data/circllhist_proto_json.gold | 72 ++++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-) diff --git a/test/test_data/circllhist_proto_json.gold b/test/test_data/circllhist_proto_json.gold index aeb0bb676..4262b4fd2 100644 --- a/test/test_data/circllhist_proto_json.gold +++ b/test/test_data/circllhist_proto_json.gold @@ -8,30 +8,100 @@ "duration": "0.000000001s" }, { - "percentile": 0.25, + "percentile": 0.1, + "count": "1", + "duration": "0.000000001s" + }, + { + "percentile": 0.2, "count": "2", + "duration": "0.000000002s" + }, + { + "percentile": 0.3, + "count": "3", "duration": "0.000000003s" }, + { + "percentile": 0.4, + "count": "4", + "duration": "0.000000004s" + }, { "percentile": 0.5, "count": "5", "duration": "0.000000005s" }, + { + "percentile": 0.55, + "count": "5", + "duration": "0.000000006s" + }, + { + "percentile": 0.6, + "count": "6", + "duration": "0.000000006s" + }, + { + "percentile": 0.65, + "count": "6", + "duration": "0.000000007s" + }, + { + "percentile": 0.7, + "count": "7", + "duration": "0.000000007s" + }, { "percentile": 0.75, "count": "7", "duration": "0.000000008s" }, + { + "percentile": 0.775, + "count": "7", + "duration": "0.000000008s" + }, + { + "percentile": 0.8, + "count": "8", + "duration": "0.000000008s" + }, + { + "percentile": 0.825, + "count": "8", + "duration": "0.000000009s" + }, + { + "percentile": 0.85, + "count": "8", + "duration": "0.000000009s" + }, + { + "percentile": 0.875, + "count": "8", + "duration": "0.000000009s" + }, { "percentile": 0.9, "count": "9", "duration": "0.000000009s" }, + { + "percentile": 0.925, + "count": "9", + "duration": "0.000000010s" + }, { "percentile": 0.95, "count": "9", "duration": "0.000000010s" }, + { + "percentile": 0.975, + "count": "9", + "duration": "0.000000010s" + }, { "percentile": 0.99, "count": "9", From 97ccc8abe7f47d7ef4a76e68e270079c57ef5895 Mon Sep 17 00:00:00 2001 From: qqustc Date: Mon, 6 Jul 2020 18:10:25 -0400 Subject: [PATCH 09/44] Update statistic_impl.h Signed-off-by: Qin Qin --- source/common/statistic_impl.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/statistic_impl.h b/source/common/statistic_impl.h index 95a0da7af..91bd6d91b 100644 --- a/source/common/statistic_impl.h +++ b/source/common/statistic_impl.h @@ -240,9 +240,9 @@ class SinkableHdrStatistic : public SinkableStatistic, public HdrStatistic { scope_.deliverHistogramToSinks(*this, value); } bool used() const override { return count() > 0; } - // Overriding name() and tagExtractedName() method to return Nighthawk::Statistic::id(). + // Overriding name() to return Nighthawk::Statistic::id(). std::string name() const override { return id(); } - std::string tagExtractedName() const override { return id(); } + std::string tagExtractedName() const override { ASSERT(false, "tagExtractedName() should not be called in Nighthawk Statistic"); } }; // Implementation of sinkable Nighthawk Statistic with Circllhist Histogram. @@ -260,9 +260,9 @@ class SinkableCircllhistStatistic : public SinkableStatistic, public CircllhistS scope_.deliverHistogramToSinks(*this, value); } bool used() const override { return count() > 0; } - // Overriding name() and tagExtractedName() method to return Nighthawk::Statistic::id(). + // Overriding name() to return Nighthawk::Statistic::id(). std::string name() const override { return id(); } - std::string tagExtractedName() const override { return id(); } + std::string tagExtractedName() const override { ASSERT(false, "tagExtractedName() should not be called in Nighthawk Statistic"); } }; } // namespace Nighthawk From 6876450e6d33023c6155a79ea04fe921327e9cdd Mon Sep 17 00:00:00 2001 From: qqustc Date: Mon, 6 Jul 2020 18:30:46 -0400 Subject: [PATCH 10/44] Update statistic_test.cc Signed-off-by: Qin Qin --- test/statistic_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/statistic_test.cc b/test/statistic_test.cc index 1ea9b78b7..064648cb6 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -378,7 +378,7 @@ TYPED_TEST(SinkableStatisticTest, EmptySinkableStatistic) { EXPECT_EQ(Envoy::Stats::Histogram::Unit::Unspecified, stat.unit()); EXPECT_FALSE(stat.used()); EXPECT_EQ("", stat.name()); - EXPECT_EQ("", stat.tagExtractedName()); + ASSERT_DEATH(stat.tagExtractedName(), ".*should not be called in Nighthawk Statistic"); EXPECT_EQ(absl::nullopt, stat.worker_id()); } @@ -397,7 +397,7 @@ TYPED_TEST(SinkableStatisticTest, SimpleSinkableStatistic) { EXPECT_EQ(Envoy::Stats::Histogram::Unit::Unspecified, stat.unit()); EXPECT_TRUE(stat.used()); EXPECT_EQ(stat_name, stat.name()); - EXPECT_EQ(stat_name, stat.tagExtractedName()); + ASSERT_DEATH(stat.tagExtractedName(), ".*should not be called in Nighthawk Statistic"); EXPECT_TRUE(stat.worker_id().has_value()); EXPECT_EQ(worker_id, stat.worker_id().value()); } From 8fd0a0505ba84c4c879904ce34f98ceea4ab2aef Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Mon, 6 Jul 2020 18:39:50 -0400 Subject: [PATCH 11/44] Refactor SinkableCircllhistStatistic Signed-off-by: qqustc Signed-off-by: Qin Qin --- source/common/statistic_impl.cc | 4 +++- source/common/statistic_impl.h | 42 ++++++++++++++++++--------------- test/statistic_test.cc | 6 +++-- 3 files changed, 30 insertions(+), 22 deletions(-) diff --git a/source/common/statistic_impl.cc b/source/common/statistic_impl.cc index 41f6be9c6..2e372a4c9 100644 --- a/source/common/statistic_impl.cc +++ b/source/common/statistic_impl.cc @@ -253,7 +253,9 @@ nighthawk::client::Statistic CircllhistStatistic::toProto(SerializationDomain do if (count() == 0) return proto; - std::vector quantiles{0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.55, 0.6, 0.65, 0.7, 0.75, 0.775, 0.8, 0.825, 0.85, 0.875, 0.90, 0.925, 0.95, 0.975, 0.99, 0.995, 0.999, 1}; + std::vector quantiles{0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.55, 0.6, + 0.65, 0.7, 0.75, 0.775, 0.8, 0.825, 0.85, 0.875, + 0.90, 0.925, 0.95, 0.975, 0.99, 0.995, 0.999, 1}; std::vector computed_quantiles(quantiles.size(), 0.0); hist_approx_quantile(histogram_, quantiles.data(), quantiles.size(), computed_quantiles.data()); for (size_t i = 0; i < quantiles.size(); i++) { diff --git a/source/common/statistic_impl.h b/source/common/statistic_impl.h index 91bd6d91b..4f4b5b0ab 100644 --- a/source/common/statistic_impl.h +++ b/source/common/statistic_impl.h @@ -186,18 +186,18 @@ class CircllhistStatistic : public StatisticImpl { }; /** - * In order to be able to flush a histogram value to downstream Envoy stats Sinks, abstract class SinkableStatistic takes the Scope reference in the constructor and wraps the - * Envoy::Stats::HistogramHelper interface. Implementation of sinkable Nighthawk Statistic class will inherit from this class. + * In order to be able to flush a histogram value to downstream Envoy stats Sinks, abstract class + * SinkableStatistic takes the Scope reference in the constructor and wraps the + * Envoy::Stats::HistogramHelper interface. Implementation of sinkable Nighthawk Statistic class + * will inherit from this class. */ class SinkableStatistic : public Envoy::Stats::HistogramImplHelper { - public: +public: // Calling HistogramImplHelper(SymbolTable& symbol_table) constructor to construct an empty // MetricImpl. This is to bypass the complicated logic of setting up SymbolTable/StatName in // Envoy. - SinkableStatistic(Envoy::Stats::Scope& scope, - const absl::optional worker_id) - : Envoy::Stats::HistogramImplHelper(scope.symbolTable()), - scope_(scope), + SinkableStatistic(Envoy::Stats::Scope& scope, const absl::optional worker_id) + : Envoy::Stats::HistogramImplHelper(scope.symbolTable()), scope_(scope), worker_id_(worker_id) {} ~SinkableStatistic() override { @@ -206,30 +206,30 @@ class SinkableStatistic : public Envoy::Stats::HistogramImplHelper { MetricImpl::clear(scope_.symbolTable()); } - // Currently Envoy Histogram Unit supports {Unspecified, Bytes, Microseconds, Milliseconds}. By default, Nighthawk::Statistic uses nanosecond as the unit of latency histograms, so Unspecified is returned here to isolate Nighthawk Statistic from Envoy Histogram Unit. + // Currently Envoy Histogram Unit supports {Unspecified, Bytes, Microseconds, Milliseconds}. By + // default, Nighthawk::Statistic uses nanosecond as the unit of latency histograms, so Unspecified + // is returned here to isolate Nighthawk Statistic from Envoy Histogram Unit. Envoy::Stats::Histogram::Unit unit() const override { return Envoy::Stats::Histogram::Unit::Unspecified; }; - Envoy::Stats::SymbolTable& symbolTable() override { - return scope_.symbolTable(); - } + Envoy::Stats::SymbolTable& symbolTable() override { return scope_.symbolTable(); } const absl::optional worker_id() { return worker_id_; } - protected: +protected: // This is used for delivering the histogram data to sinks. Envoy::Stats::Scope& scope_; - private: +private: // worker_id can be used in downstream stats Sinks as the stats tag. absl::optional worker_id_; }; // Implementation of sinkable Nighthawk Statistic with HdrHistogram. class SinkableHdrStatistic : public SinkableStatistic, public HdrStatistic { - public: +public: SinkableHdrStatistic(Envoy::Stats::Scope& scope, - const absl::optional worker_id = absl::nullopt) + const absl::optional worker_id = absl::nullopt) : SinkableStatistic(scope, worker_id) {} // Envoy::Stats::Histogram @@ -242,14 +242,16 @@ class SinkableHdrStatistic : public SinkableStatistic, public HdrStatistic { bool used() const override { return count() > 0; } // Overriding name() to return Nighthawk::Statistic::id(). std::string name() const override { return id(); } - std::string tagExtractedName() const override { ASSERT(false, "tagExtractedName() should not be called in Nighthawk Statistic"); } + std::string tagExtractedName() const override { + ASSERT(false, "tagExtractedName() should not be called in Nighthawk Statistic"); + } }; // Implementation of sinkable Nighthawk Statistic with Circllhist Histogram. class SinkableCircllhistStatistic : public SinkableStatistic, public CircllhistStatistic { - public: +public: SinkableCircllhistStatistic(Envoy::Stats::Scope& scope, - const absl::optional worker_id = absl::nullopt) + const absl::optional worker_id = absl::nullopt) : SinkableStatistic(scope, worker_id) {} // Envoy::Stats::Histogram @@ -262,7 +264,9 @@ class SinkableCircllhistStatistic : public SinkableStatistic, public CircllhistS bool used() const override { return count() > 0; } // Overriding name() to return Nighthawk::Statistic::id(). std::string name() const override { return id(); } - std::string tagExtractedName() const override { ASSERT(false, "tagExtractedName() should not be called in Nighthawk Statistic"); } + std::string tagExtractedName() const override { + ASSERT(false, "tagExtractedName() should not be called in Nighthawk Statistic"); + } }; } // namespace Nighthawk diff --git a/test/statistic_test.cc b/test/statistic_test.cc index 064648cb6..41fbda0ff 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -277,10 +277,12 @@ TEST(StatisticTest, CircllhistStatisticProtoOutputLargeValues) { uint64_t value = 100ul + 0xFFFFFFFF; statistic.addValue(value); statistic.addValue(value); - const nighthawk::client::Statistic proto = statistic.toProto(Statistic::SerializationDomain::DURATION); + const nighthawk::client::Statistic proto = + statistic.toProto(Statistic::SerializationDomain::DURATION); EXPECT_EQ(proto.count(), 2); - Helper::expectNear(Envoy::Protobuf::util::TimeUtil::DurationToNanoseconds(proto.mean()), value, statistic.significantDigits()); + Helper::expectNear(Envoy::Protobuf::util::TimeUtil::DurationToNanoseconds(proto.mean()), value, + statistic.significantDigits()); EXPECT_EQ(Envoy::Protobuf::util::TimeUtil::DurationToNanoseconds(proto.pstdev()), 0); } From cd6b29e49f4208f475a6b07353c08875f4993967 Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Mon, 6 Jul 2020 20:45:22 -0400 Subject: [PATCH 12/44] fix clang_tidy Signed-off-by: Qin Qin --- source/common/statistic_impl.cc | 3 ++- source/common/statistic_impl.h | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/source/common/statistic_impl.cc b/source/common/statistic_impl.cc index 2e372a4c9..2e0f238c7 100644 --- a/source/common/statistic_impl.cc +++ b/source/common/statistic_impl.cc @@ -250,8 +250,9 @@ StatisticPtr CircllhistStatistic::combine(const Statistic& statistic) const { nighthawk::client::Statistic CircllhistStatistic::toProto(SerializationDomain domain) const { nighthawk::client::Statistic proto = StatisticImpl::toProto(domain); - if (count() == 0) + if (count() == 0) { return proto; + } std::vector quantiles{0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.55, 0.6, 0.65, 0.7, 0.75, 0.775, 0.8, 0.825, 0.85, 0.875, diff --git a/source/common/statistic_impl.h b/source/common/statistic_impl.h index 4f4b5b0ab..381ee937d 100644 --- a/source/common/statistic_impl.h +++ b/source/common/statistic_impl.h @@ -244,6 +244,7 @@ class SinkableHdrStatistic : public SinkableStatistic, public HdrStatistic { std::string name() const override { return id(); } std::string tagExtractedName() const override { ASSERT(false, "tagExtractedName() should not be called in Nighthawk Statistic"); + return id(); } }; @@ -266,6 +267,7 @@ class SinkableCircllhistStatistic : public SinkableStatistic, public CircllhistS std::string name() const override { return id(); } std::string tagExtractedName() const override { ASSERT(false, "tagExtractedName() should not be called in Nighthawk Statistic"); + return id(); } }; From 684fe0b433fe9ba9b73a7b6659bb8ed47fc9966d Mon Sep 17 00:00:00 2001 From: qqustc Date: Tue, 7 Jul 2020 10:18:12 -0400 Subject: [PATCH 13/44] Update statistic_test.cc Signed-off-by: Qin Qin Signed-off-by: qqustc --- test/statistic_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/statistic_test.cc b/test/statistic_test.cc index 41fbda0ff..5076b1e2c 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -380,7 +380,6 @@ TYPED_TEST(SinkableStatisticTest, EmptySinkableStatistic) { EXPECT_EQ(Envoy::Stats::Histogram::Unit::Unspecified, stat.unit()); EXPECT_FALSE(stat.used()); EXPECT_EQ("", stat.name()); - ASSERT_DEATH(stat.tagExtractedName(), ".*should not be called in Nighthawk Statistic"); EXPECT_EQ(absl::nullopt, stat.worker_id()); } @@ -399,7 +398,6 @@ TYPED_TEST(SinkableStatisticTest, SimpleSinkableStatistic) { EXPECT_EQ(Envoy::Stats::Histogram::Unit::Unspecified, stat.unit()); EXPECT_TRUE(stat.used()); EXPECT_EQ(stat_name, stat.name()); - ASSERT_DEATH(stat.tagExtractedName(), ".*should not be called in Nighthawk Statistic"); EXPECT_TRUE(stat.worker_id().has_value()); EXPECT_EQ(worker_id, stat.worker_id().value()); } From dc655a40c112e8e8c46f463e268082c5233185ba Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Tue, 7 Jul 2020 12:40:56 -0400 Subject: [PATCH 14/44] replace ASSERT with RELEASE_ASSERT Signed-off-by: Qin Qin --- source/common/statistic_impl.h | 4 ++-- test/statistic_test.cc | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/source/common/statistic_impl.h b/source/common/statistic_impl.h index 381ee937d..33e0a7055 100644 --- a/source/common/statistic_impl.h +++ b/source/common/statistic_impl.h @@ -243,7 +243,7 @@ class SinkableHdrStatistic : public SinkableStatistic, public HdrStatistic { // Overriding name() to return Nighthawk::Statistic::id(). std::string name() const override { return id(); } std::string tagExtractedName() const override { - ASSERT(false, "tagExtractedName() should not be called in Nighthawk Statistic"); + RELEASE_ASSERT(false, "tagExtractedName() should not be called in Nighthawk Statistic"); return id(); } }; @@ -266,7 +266,7 @@ class SinkableCircllhistStatistic : public SinkableStatistic, public CircllhistS // Overriding name() to return Nighthawk::Statistic::id(). std::string name() const override { return id(); } std::string tagExtractedName() const override { - ASSERT(false, "tagExtractedName() should not be called in Nighthawk Statistic"); + RELEASE_ASSERT(false, "tagExtractedName() should not be called in Nighthawk Statistic"); return id(); } }; diff --git a/test/statistic_test.cc b/test/statistic_test.cc index 5076b1e2c..12dfb7089 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -380,6 +380,7 @@ TYPED_TEST(SinkableStatisticTest, EmptySinkableStatistic) { EXPECT_EQ(Envoy::Stats::Histogram::Unit::Unspecified, stat.unit()); EXPECT_FALSE(stat.used()); EXPECT_EQ("", stat.name()); + EXPECT_DEATH(stat.tagExtractedName(), ".*should not be called in Nighthawk Statistic"); EXPECT_EQ(absl::nullopt, stat.worker_id()); } @@ -398,6 +399,7 @@ TYPED_TEST(SinkableStatisticTest, SimpleSinkableStatistic) { EXPECT_EQ(Envoy::Stats::Histogram::Unit::Unspecified, stat.unit()); EXPECT_TRUE(stat.used()); EXPECT_EQ(stat_name, stat.name()); + EXPECT_DEATH(stat.tagExtractedName(), ".*should not be called in Nighthawk Statistic"); EXPECT_TRUE(stat.worker_id().has_value()); EXPECT_EQ(worker_id, stat.worker_id().value()); } From fc298c5a2a001ec8acc8d70c8191601f6163c32b Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Tue, 7 Jul 2020 16:01:13 -0400 Subject: [PATCH 15/44] update statistic_test.cc Signed-off-by: Qin Qin --- test/statistic_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/statistic_test.cc b/test/statistic_test.cc index 12dfb7089..a770b1ee8 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -380,7 +380,7 @@ TYPED_TEST(SinkableStatisticTest, EmptySinkableStatistic) { EXPECT_EQ(Envoy::Stats::Histogram::Unit::Unspecified, stat.unit()); EXPECT_FALSE(stat.used()); EXPECT_EQ("", stat.name()); - EXPECT_DEATH(stat.tagExtractedName(), ".*should not be called in Nighthawk Statistic"); + EXPECT_DEATH(stat.tagExtractedName(), ".*"); EXPECT_EQ(absl::nullopt, stat.worker_id()); } @@ -399,7 +399,7 @@ TYPED_TEST(SinkableStatisticTest, SimpleSinkableStatistic) { EXPECT_EQ(Envoy::Stats::Histogram::Unit::Unspecified, stat.unit()); EXPECT_TRUE(stat.used()); EXPECT_EQ(stat_name, stat.name()); - EXPECT_DEATH(stat.tagExtractedName(), ".*should not be called in Nighthawk Statistic"); + EXPECT_DEATH(stat.tagExtractedName(), ".*"); EXPECT_TRUE(stat.worker_id().has_value()); EXPECT_EQ(worker_id, stat.worker_id().value()); } From e1356eca04fe0a99aa85dc895e29609c652905af Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Tue, 7 Jul 2020 20:34:20 -0400 Subject: [PATCH 16/44] update statistic_test.cc Signed-off-by: Qin Qin --- test/statistic_test.cc | 46 ++++++++++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/test/statistic_test.cc b/test/statistic_test.cc index a770b1ee8..ab5567227 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -223,6 +223,15 @@ TYPED_TEST(TypedStatisticTest, StringOutput) { } } +TYPED_TEST(TypedStatisticTest, IdFieldWorks) { + TypeParam statistic; + std::string id = "fooid"; + + EXPECT_EQ("", statistic.id()); + statistic.setId(id); + EXPECT_EQ(id, statistic.id()); +} + class StatisticTest : public Test {}; // Note that we explicitly subject SimpleStatistic to the large @@ -341,15 +350,6 @@ TEST(StatisticTest, CombineAcrossTypesFails) { EXPECT_THROW(d.combine(a), std::bad_cast); } -TEST(StatisticTest, IdFieldWorks) { - StreamingStatistic c; - std::string id = "fooid"; - - EXPECT_EQ("", c.id()); - c.setId(id); - EXPECT_EQ(id, c.id()); -} - TEST(StatisticTest, HdrStatisticOutOfRange) { HdrStatistic a; a.addValue(INT64_MAX); @@ -359,13 +359,26 @@ TEST(StatisticTest, HdrStatisticOutOfRange) { TEST(StatisticTest, NullStatistic) { NullStatistic stat; EXPECT_EQ(0, stat.count()); + std::string id = "fooid"; + stat.setId(id); + EXPECT_EQ(id, stat.id()); stat.addValue(1); + EXPECT_EQ(0, stat.count()); + EXPECT_EQ(0, stat.max()); + EXPECT_EQ(UINT64_MAX, stat.min()); EXPECT_EQ(0, stat.mean()); EXPECT_EQ(0, stat.pvariance()); EXPECT_EQ(0, stat.pstdev()); EXPECT_NE(nullptr, stat.combine(stat)); EXPECT_EQ(0, stat.significantDigits()); EXPECT_NE(nullptr, stat.createNewInstanceOfSameType()); + const nighthawk::client::Statistic proto = stat.toProto(Statistic::SerializationDomain::RAW); + EXPECT_EQ(id, proto.id()); + EXPECT_EQ(0, proto.count()); + EXPECT_EQ(0, proto.raw_mean()); + EXPECT_EQ(0, proto.raw_pstdev()); + EXPECT_EQ(0, proto.raw_max()); + EXPECT_EQ(UINT64_MAX, proto.raw_min()); } using SinkableTypes = Types; @@ -377,6 +390,12 @@ TYPED_TEST_SUITE(SinkableStatisticTest, SinkableTypes); TYPED_TEST(SinkableStatisticTest, EmptySinkableStatistic) { Envoy::Stats::MockIsolatedStatsStore mock_store; TypeParam stat(mock_store); + EXPECT_EQ(0, stat.count()); + EXPECT_TRUE(std::isnan(stat.mean())); + EXPECT_TRUE(std::isnan(stat.pvariance())); + EXPECT_TRUE(std::isnan(stat.pstdev())); + EXPECT_EQ(stat.min(), UINT64_MAX); + EXPECT_EQ(stat.max(), 0); EXPECT_EQ(Envoy::Stats::Histogram::Unit::Unspecified, stat.unit()); EXPECT_FALSE(stat.used()); EXPECT_EQ("", stat.name()); @@ -388,14 +407,19 @@ TYPED_TEST(SinkableStatisticTest, SimpleSinkableStatistic) { Envoy::Stats::MockIsolatedStatsStore mock_store; const int worker_id = 0; TypeParam stat(mock_store, worker_id); - const uint64_t sample_value = 123; const std::string stat_name = "stat_name"; EXPECT_CALL(mock_store, deliverHistogramToSinks(_, sample_value)).Times(1); stat.recordValue(sample_value); - stat.setId(stat_name); + + EXPECT_EQ(1, stat.count()); + Helper::expectNear(123.0, stat.mean(), stat.significantDigits()); + EXPECT_DOUBLE_EQ(0, stat.pvariance()); + EXPECT_DOUBLE_EQ(0, stat.pstdev()); + EXPECT_EQ(123, stat.min()); + EXPECT_EQ(123, stat.max()); EXPECT_EQ(Envoy::Stats::Histogram::Unit::Unspecified, stat.unit()); EXPECT_TRUE(stat.used()); EXPECT_EQ(stat_name, stat.name()); From a9d4d5dcd1c1b0094305b53c8a1f42a0d0a420bd Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Wed, 8 Jul 2020 10:10:12 -0400 Subject: [PATCH 17/44] update statistic_impl.h and change coverage threshold Signed-off-by: Qin Qin --- source/common/statistic_impl.h | 6 ++---- test/run_nighthawk_bazel_coverage.sh | 4 ++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/source/common/statistic_impl.h b/source/common/statistic_impl.h index 33e0a7055..c1505b80f 100644 --- a/source/common/statistic_impl.h +++ b/source/common/statistic_impl.h @@ -243,8 +243,7 @@ class SinkableHdrStatistic : public SinkableStatistic, public HdrStatistic { // Overriding name() to return Nighthawk::Statistic::id(). std::string name() const override { return id(); } std::string tagExtractedName() const override { - RELEASE_ASSERT(false, "tagExtractedName() should not be called in Nighthawk Statistic"); - return id(); + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } }; @@ -266,8 +265,7 @@ class SinkableCircllhistStatistic : public SinkableStatistic, public CircllhistS // Overriding name() to return Nighthawk::Statistic::id(). std::string name() const override { return id(); } std::string tagExtractedName() const override { - RELEASE_ASSERT(false, "tagExtractedName() should not be called in Nighthawk Statistic"); - return id(); + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } }; diff --git a/test/run_nighthawk_bazel_coverage.sh b/test/run_nighthawk_bazel_coverage.sh index 8b48f4eec..0512e3b23 100755 --- a/test/run_nighthawk_bazel_coverage.sh +++ b/test/run_nighthawk_bazel_coverage.sh @@ -43,7 +43,7 @@ COVERAGE_VALUE=${COVERAGE_VALUE%?} if [ "$VALIDATE_COVERAGE" == "true" ] then - COVERAGE_THRESHOLD=98.6 + COVERAGE_THRESHOLD=98.4 COVERAGE_FAILED=$(echo "${COVERAGE_VALUE}<${COVERAGE_THRESHOLD}" | bc) if test ${COVERAGE_FAILED} -eq 1; then echo Code coverage ${COVERAGE_VALUE} is lower than limit of ${COVERAGE_THRESHOLD} @@ -52,4 +52,4 @@ then echo Code coverage ${COVERAGE_VALUE} is good and higher than limit of ${COVERAGE_THRESHOLD} fi fi -echo "HTML coverage report is in ${COVERAGE_DIR}/index.html" \ No newline at end of file +echo "HTML coverage report is in ${COVERAGE_DIR}/index.html" From e4d66721f8d33b7582e121713869f4aa9245ae5b Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Wed, 8 Jul 2020 10:15:32 -0400 Subject: [PATCH 18/44] format Signed-off-by: Qin Qin --- source/common/statistic_impl.h | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/source/common/statistic_impl.h b/source/common/statistic_impl.h index c1505b80f..e07c9cf66 100644 --- a/source/common/statistic_impl.h +++ b/source/common/statistic_impl.h @@ -242,9 +242,7 @@ class SinkableHdrStatistic : public SinkableStatistic, public HdrStatistic { bool used() const override { return count() > 0; } // Overriding name() to return Nighthawk::Statistic::id(). std::string name() const override { return id(); } - std::string tagExtractedName() const override { - NOT_IMPLEMENTED_GCOVR_EXCL_LINE; - } + std::string tagExtractedName() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } }; // Implementation of sinkable Nighthawk Statistic with Circllhist Histogram. @@ -264,9 +262,7 @@ class SinkableCircllhistStatistic : public SinkableStatistic, public CircllhistS bool used() const override { return count() > 0; } // Overriding name() to return Nighthawk::Statistic::id(). std::string name() const override { return id(); } - std::string tagExtractedName() const override { - NOT_IMPLEMENTED_GCOVR_EXCL_LINE; - } + std::string tagExtractedName() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } }; } // namespace Nighthawk From db7889263539fdaab94a4cf38f79e11a0980606e Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Fri, 10 Jul 2020 12:05:35 -0400 Subject: [PATCH 19/44] update statistic_impl Signed-off-by: Qin Qin --- source/common/statistic_impl.cc | 79 +++++++++++++++++++++++++++++---- source/common/statistic_impl.h | 71 +++++++++-------------------- test/statistic_test.cc | 16 ++++--- 3 files changed, 102 insertions(+), 64 deletions(-) diff --git a/source/common/statistic_impl.cc b/source/common/statistic_impl.cc index 2e0f238c7..0bf5998f9 100644 --- a/source/common/statistic_impl.cc +++ b/source/common/statistic_impl.cc @@ -236,27 +236,50 @@ nighthawk::client::Statistic HdrStatistic::toProto(SerializationDomain domain) c return proto; } +CircllhistStatistic::CircllhistStatistic() { + histogram_ = hist_alloc(); + ASSERT(histogram_ != nullptr); +} + +CircllhistStatistic::~CircllhistStatistic() { hist_free(histogram_); } + +void CircllhistStatistic::addValue(uint64_t value) { + hist_insert_intscale(histogram_, value, 0, 1); + StatisticImpl::addValue(value); +} +double CircllhistStatistic::mean() const { return hist_approx_mean(histogram_); } +double CircllhistStatistic::pvariance() const { return pstdev() * pstdev(); } +double CircllhistStatistic::pstdev() const { + return count() == 0 ? std::nan("") : hist_approx_stddev(histogram_); +} + StatisticPtr CircllhistStatistic::combine(const Statistic& statistic) const { auto combined = std::make_unique(); - const auto& b = dynamic_cast(statistic); - hist_accumulate(combined->histogram_, &this->histogram_, 1); - hist_accumulate(combined->histogram_, &b.histogram_, 1); + const auto& stat = dynamic_cast(statistic); + hist_accumulate(combined->histogram_, &this->histogram_, /*histogram_count=*/1); + hist_accumulate(combined->histogram_, &stat.histogram_, /*histogram_count=*/1); - combined->min_ = std::min(this->min(), b.min()); - combined->max_ = std::max(this->max(), b.max()); - combined->count_ = this->count() + b.count(); + combined->min_ = std::min(this->min(), stat.min()); + combined->max_ = std::max(this->max(), stat.max()); + combined->count_ = this->count() + stat.count(); return combined; } +StatisticPtr CircllhistStatistic::createNewInstanceOfSameType() const { + return std::make_unique(); +} + nighthawk::client::Statistic CircllhistStatistic::toProto(SerializationDomain domain) const { nighthawk::client::Statistic proto = StatisticImpl::toProto(domain); if (count() == 0) { return proto; } - std::vector quantiles{0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.55, 0.6, - 0.65, 0.7, 0.75, 0.775, 0.8, 0.825, 0.85, 0.875, - 0.90, 0.925, 0.95, 0.975, 0.99, 0.995, 0.999, 1}; + // List of quantiles is based on hdr_proto_json.gold which lists the quantiles provided by + // HdrHistogram. + const std::vector quantiles{0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.55, 0.6, + 0.65, 0.7, 0.75, 0.775, 0.8, 0.825, 0.85, 0.875, + 0.90, 0.925, 0.95, 0.975, 0.99, 0.995, 0.999, 1}; std::vector computed_quantiles(quantiles.size(), 0.0); hist_approx_quantile(histogram_, quantiles.data(), quantiles.size(), computed_quantiles.data()); for (size_t i = 0; i < quantiles.size(); i++) { @@ -274,4 +297,42 @@ nighthawk::client::Statistic CircllhistStatistic::toProto(SerializationDomain do return proto; } +SinkableStatistic::SinkableStatistic(Envoy::Stats::Scope& scope, absl::optional worker_id) + : Envoy::Stats::HistogramImplHelper(scope.symbolTable()), scope_(scope), worker_id_(worker_id) { +} + +SinkableStatistic::~SinkableStatistic() { + // We must explicitly free the StatName here in order to supply the + // SymbolTable reference. + MetricImpl::clear(scope_.symbolTable()); +} + +Envoy::Stats::Histogram::Unit SinkableStatistic::unit() const { + return Envoy::Stats::Histogram::Unit::Unspecified; +} + +Envoy::Stats::SymbolTable& SinkableStatistic::symbolTable() { return scope_.symbolTable(); } + +SinkableHdrStatistic::SinkableHdrStatistic(Envoy::Stats::Scope& scope, + absl::optional worker_id) + : SinkableStatistic(scope, worker_id) {} + +void SinkableHdrStatistic::recordValue(uint64_t value) { + addValue(value); + // Currently in Envoy Scope implementation, deliverHistogramToSinks() will flush the histogram + // value directly to stats Sinks. + scope_.deliverHistogramToSinks(*this, value); +} + +SinkableCircllhistStatistic::SinkableCircllhistStatistic(Envoy::Stats::Scope& scope, + absl::optional worker_id) + : SinkableStatistic(scope, worker_id) {} + +void SinkableCircllhistStatistic::recordValue(uint64_t value) { + addValue(value); + // Currently in Envoy Scope implementation, deliverHistogramToSinks() will flush the histogram + // value directly to stats Sinks. + scope_.deliverHistogramToSinks(*this, value); +} + } // namespace Nighthawk diff --git a/source/common/statistic_impl.h b/source/common/statistic_impl.h index e07c9cf66..1239d5881 100644 --- a/source/common/statistic_impl.h +++ b/source/common/statistic_impl.h @@ -159,26 +159,16 @@ class HdrStatistic : public StatisticImpl { */ class CircllhistStatistic : public StatisticImpl { public: - CircllhistStatistic() { - histogram_ = hist_alloc(); - ASSERT(histogram_ != nullptr); - } - ~CircllhistStatistic() override { hist_free(histogram_); } + CircllhistStatistic(); + ~CircllhistStatistic() override; - void addValue(uint64_t value) override { - hist_insert_intscale(histogram_, value, 0, 1); - StatisticImpl::addValue(value); - } - double mean() const override { return hist_approx_mean(histogram_); } - double pvariance() const override { return pstdev() * pstdev(); } - double pstdev() const override { - return count() == 0 ? std::nan("") : hist_approx_stddev(histogram_); - } + void addValue(uint64_t value) override; + double mean() const override; + double pvariance() const override; + double pstdev() const override; StatisticPtr combine(const Statistic& statistic) const override; uint64_t significantDigits() const override { return 1; } - StatisticPtr createNewInstanceOfSameType() const override { - return std::make_unique(); - } + StatisticPtr createNewInstanceOfSameType() const override; nighthawk::client::Statistic toProto(SerializationDomain domain) const override; private: @@ -196,27 +186,19 @@ class SinkableStatistic : public Envoy::Stats::HistogramImplHelper { // Calling HistogramImplHelper(SymbolTable& symbol_table) constructor to construct an empty // MetricImpl. This is to bypass the complicated logic of setting up SymbolTable/StatName in // Envoy. - SinkableStatistic(Envoy::Stats::Scope& scope, const absl::optional worker_id) - : Envoy::Stats::HistogramImplHelper(scope.symbolTable()), scope_(scope), - worker_id_(worker_id) {} - - ~SinkableStatistic() override { - // We must explicitly free the StatName here in order to supply the - // SymbolTable reference. - MetricImpl::clear(scope_.symbolTable()); - } + SinkableStatistic(Envoy::Stats::Scope& scope, absl::optional worker_id); + ~SinkableStatistic() override; // Currently Envoy Histogram Unit supports {Unspecified, Bytes, Microseconds, Milliseconds}. By // default, Nighthawk::Statistic uses nanosecond as the unit of latency histograms, so Unspecified // is returned here to isolate Nighthawk Statistic from Envoy Histogram Unit. - Envoy::Stats::Histogram::Unit unit() const override { - return Envoy::Stats::Histogram::Unit::Unspecified; - }; - Envoy::Stats::SymbolTable& symbolTable() override { return scope_.symbolTable(); } - + Envoy::Stats::Histogram::Unit unit() const override; + Envoy::Stats::SymbolTable& symbolTable() override; + // Return the id of the worker where this statistic is defined. Per worker + // statistic should always set worker_id. Return absl::nullopt when the + // statistic is not defined per worker. const absl::optional worker_id() { return worker_id_; } -protected: // This is used for delivering the histogram data to sinks. Envoy::Stats::Scope& scope_; @@ -228,17 +210,12 @@ class SinkableStatistic : public Envoy::Stats::HistogramImplHelper { // Implementation of sinkable Nighthawk Statistic with HdrHistogram. class SinkableHdrStatistic : public SinkableStatistic, public HdrStatistic { public: - SinkableHdrStatistic(Envoy::Stats::Scope& scope, - const absl::optional worker_id = absl::nullopt) - : SinkableStatistic(scope, worker_id) {} + // The constructor takes the Scope reference which is used to flush a histogram value to + // downstream stats Sinks through deliverHistogramToSinks(). + SinkableHdrStatistic(Envoy::Stats::Scope& scope, absl::optional worker_id = absl::nullopt); // Envoy::Stats::Histogram - void recordValue(uint64_t value) override { - addValue(value); - // Currently in Envoy Scope implementation, deliverHistogramToSinks() will flush the histogram - // value directly to stats Sinks. - scope_.deliverHistogramToSinks(*this, value); - } + void recordValue(uint64_t value) override; bool used() const override { return count() > 0; } // Overriding name() to return Nighthawk::Statistic::id(). std::string name() const override { return id(); } @@ -248,17 +225,13 @@ class SinkableHdrStatistic : public SinkableStatistic, public HdrStatistic { // Implementation of sinkable Nighthawk Statistic with Circllhist Histogram. class SinkableCircllhistStatistic : public SinkableStatistic, public CircllhistStatistic { public: + // The constructor takes the Scope reference which is used to flush a histogram value to + // downstream stats Sinks through deliverHistogramToSinks(). SinkableCircllhistStatistic(Envoy::Stats::Scope& scope, - const absl::optional worker_id = absl::nullopt) - : SinkableStatistic(scope, worker_id) {} + absl::optional worker_id = absl::nullopt); // Envoy::Stats::Histogram - void recordValue(uint64_t value) override { - addValue(value); - // Currently in Envoy Scope implementation, deliverHistogramToSinks() will flush the histogram - // value directly to stats Sinks. - scope_.deliverHistogramToSinks(*this, value); - } + void recordValue(uint64_t value) override; bool used() const override { return count() > 0; } // Overriding name() to return Nighthawk::Statistic::id(). std::string name() const override { return id(); } diff --git a/test/statistic_test.cc b/test/statistic_test.cc index ab5567227..b72d76043 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -307,12 +307,14 @@ TEST(StatisticTest, HdrStatisticPercentilesProto) { util.loadFromJson(Envoy::Filesystem::fileSystemForTest().fileReadToEnd( TestEnvironment::runfilesPath("test/test_data/hdr_proto_json.gold")), parsed_json_proto, Envoy::ProtobufMessage::getStrictValidationVisitor()); - // Instead of comparing proto's, we perform a string-based comparison, because that emits a - // helpful diff when this fails. const std::string json = util.getJsonStringFromMessage( statistic.toProto(Statistic::SerializationDomain::DURATION), true, true); const std::string golden_json = util.getJsonStringFromMessage(parsed_json_proto, true, true); - EXPECT_EQ(json, golden_json); + EXPECT_THAT(statistic.toProto(Statistic::SerializationDomain::DURATION), + Envoy::ProtoEq(parsed_json_proto)) + << json << "\n" + << "is not equal to golden file:\n" + << golden_json; } TEST(StatisticTest, CircllhistStatisticPercentilesProto) { @@ -327,12 +329,14 @@ TEST(StatisticTest, CircllhistStatisticPercentilesProto) { util.loadFromJson(Envoy::Filesystem::fileSystemForTest().fileReadToEnd( TestEnvironment::runfilesPath("test/test_data/circllhist_proto_json.gold")), parsed_json_proto, Envoy::ProtobufMessage::getStrictValidationVisitor()); - // Instead of comparing proto's, we perform a string-based comparison, because that emits a - // helpful diff when this fails. const std::string json = util.getJsonStringFromMessage( statistic.toProto(Statistic::SerializationDomain::DURATION), true, true); const std::string golden_json = util.getJsonStringFromMessage(parsed_json_proto, true, true); - EXPECT_EQ(json, golden_json); + EXPECT_THAT(statistic.toProto(Statistic::SerializationDomain::DURATION), + Envoy::ProtoEq(parsed_json_proto)) + << json << "\n" + << "is not equal to golden file:\n" + << golden_json; } TEST(StatisticTest, CombineAcrossTypesFails) { From 8903b6090507bb475686e6f530368084e3c8f6ce Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Fri, 10 Jul 2020 14:20:18 -0400 Subject: [PATCH 20/44] update statistic_impl.cc Signed-off-by: Qin Qin --- source/common/statistic_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/statistic_impl.cc b/source/common/statistic_impl.cc index 0bf5998f9..2886608ef 100644 --- a/source/common/statistic_impl.cc +++ b/source/common/statistic_impl.cc @@ -256,8 +256,8 @@ double CircllhistStatistic::pstdev() const { StatisticPtr CircllhistStatistic::combine(const Statistic& statistic) const { auto combined = std::make_unique(); const auto& stat = dynamic_cast(statistic); - hist_accumulate(combined->histogram_, &this->histogram_, /*histogram_count=*/1); - hist_accumulate(combined->histogram_, &stat.histogram_, /*histogram_count=*/1); + hist_accumulate(combined->histogram_, &this->histogram_, /*cnt=*/1); + hist_accumulate(combined->histogram_, &stat.histogram_, /*cnt=*/1); combined->min_ = std::min(this->min(), stat.min()); combined->max_ = std::max(this->max(), stat.max()); From 0fb2d129fc491823e6a3d625d069a170f8b3d531 Mon Sep 17 00:00:00 2001 From: qqustc Date: Mon, 13 Jul 2020 10:28:41 -0400 Subject: [PATCH 21/44] update latency histogram to use nh statistics Signed-off-by: qqustc --- include/nighthawk/client/factories.h | 4 +- source/client/benchmark_client_impl.cc | 21 +++++--- source/client/benchmark_client_impl.h | 17 +++--- source/client/client_worker_impl.cc | 2 +- source/client/factories_impl.cc | 9 ++-- source/client/factories_impl.h | 2 +- source/client/stream_decoder.cc | 10 ++-- source/client/stream_decoder.h | 2 +- test/benchmark_http_client_test.cc | 58 ++++++++++++--------- test/integration/test_integration_basics.py | 12 ++--- 10 files changed, 79 insertions(+), 58 deletions(-) diff --git a/include/nighthawk/client/factories.h b/include/nighthawk/client/factories.h index dc393d8a3..0fa6a3803 100644 --- a/include/nighthawk/client/factories.h +++ b/include/nighthawk/client/factories.h @@ -33,7 +33,7 @@ class BenchmarkClientFactory { * @param scope stats scope for any stats tracked by the benchmark client. * @param cluster_manager Cluster manager preconfigured with our target cluster. * @param http_tracer Shared pointer to an http tracer implementation (e.g. Zipkin). - * @param cluster_name Name of the cluster that this benchmark client will use. In conjunction + * @param worker_number Worker id which is also the name of the cluster that this benchmark client will use. In conjunction * with cluster_manager this will allow the this BenchmarkClient to access the target connection * pool. * @param request_source Source of request-specifiers. Will be queries every time the @@ -45,7 +45,7 @@ class BenchmarkClientFactory { Envoy::Stats::Scope& scope, Envoy::Upstream::ClusterManagerPtr& cluster_manager, Envoy::Tracing::HttpTracerSharedPtr& http_tracer, - absl::string_view cluster_name, + const int worker_number, RequestSource& request_source) const PURE; }; diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index cf23def9a..a122cb2a4 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -51,6 +51,8 @@ BenchmarkClientHttpImpl::BenchmarkClientHttpImpl( Envoy::Api::Api& api, Envoy::Event::Dispatcher& dispatcher, Envoy::Stats::Scope& scope, StatisticPtr&& connect_statistic, StatisticPtr&& response_statistic, StatisticPtr&& response_header_size_statistic, StatisticPtr&& response_body_size_statistic, + std::unique_ptr&& latency_2xx_statistic, + std::unique_ptr&& latency_xxx_statistic, bool use_h2, Envoy::Upstream::ClusterManagerPtr& cluster_manager, Envoy::Tracing::HttpTracerSharedPtr& http_tracer, absl::string_view cluster_name, RequestGenerator request_generator, const bool provide_resource_backpressure) @@ -58,9 +60,11 @@ BenchmarkClientHttpImpl::BenchmarkClientHttpImpl( connect_statistic_(std::move(connect_statistic)), response_statistic_(std::move(response_statistic)), response_header_size_statistic_(std::move(response_header_size_statistic)), - response_body_size_statistic_(std::move(response_body_size_statistic)), use_h2_(use_h2), - benchmark_client_stats_( - {ALL_BENCHMARK_CLIENT_STATS(POOL_COUNTER(*scope_), POOL_HISTOGRAM(*scope_))}), + response_body_size_statistic_(std::move(response_body_size_statistic)), + latency_2xx_statistic_(std::move(latency_2xx_statistic)), + latency_xxx_statistic_(std::move(latency_xxx_statistic)), + use_h2_(use_h2), + benchmark_client_stats_({ALL_BENCHMARK_CLIENT_STATS(POOL_COUNTER(*scope_))}), cluster_manager_(cluster_manager), http_tracer_(http_tracer), cluster_name_(std::string(cluster_name)), request_generator_(std::move(request_generator)), provide_resource_backpressure_(provide_resource_backpressure) { @@ -68,6 +72,8 @@ BenchmarkClientHttpImpl::BenchmarkClientHttpImpl( response_statistic_->setId("benchmark_http_client.request_to_response"); response_header_size_statistic_->setId("benchmark_http_client.response_header_size"); response_body_size_statistic_->setId("benchmark_http_client.response_body_size"); + latency_2xx_statistic_->setId("benchmark_http_client.latency_2xx"); + latency_xxx_statistic_->setId("benchmark_http_client.latency_xxx"); } void BenchmarkClientHttpImpl::terminate() { @@ -84,6 +90,8 @@ StatisticPtrMap BenchmarkClientHttpImpl::statistics() const { statistics[response_statistic_->id()] = response_statistic_.get(); statistics[response_header_size_statistic_->id()] = response_header_size_statistic_.get(); statistics[response_body_size_statistic_->id()] = response_body_size_statistic_.get(); + statistics[latency_2xx_statistic_->id()] = latency_2xx_statistic_.get(); + statistics[latency_xxx_statistic_->id()] = latency_xxx_statistic_.get(); return statistics; }; @@ -125,7 +133,6 @@ bool BenchmarkClientHttpImpl::tryStartRequest(CompletionCallback caller_completi *response_body_size_statistic_, request->header(), shouldMeasureLatencies(), content_length, generator_, http_tracer_); requests_initiated_++; - benchmark_client_stats_.total_req_sent_.inc(); pool_ptr->newStream(*stream_decoder, *stream_decoder); return true; } @@ -172,11 +179,11 @@ void BenchmarkClientHttpImpl::onPoolFailure(Envoy::Http::ConnectionPool::PoolFai } void BenchmarkClientHttpImpl::exportLatency(const uint32_t response_code, - const uint64_t latency_us) { + const uint64_t latency_ns) { if (response_code > 199 && response_code <= 299) { - benchmark_client_stats_.latency_on_success_req_.recordValue(latency_us); + latency_2xx_statistic_->recordValue(latency_ns); } else { - benchmark_client_stats_.latency_on_error_req_.recordValue(latency_us); + latency_xxx_statistic_->recordValue(latency_ns); } } diff --git a/source/client/benchmark_client_impl.h b/source/client/benchmark_client_impl.h index 2903c935c..996ae77f0 100644 --- a/source/client/benchmark_client_impl.h +++ b/source/client/benchmark_client_impl.h @@ -13,6 +13,7 @@ #include "nighthawk/common/request_source.h" #include "nighthawk/common/sequencer.h" #include "nighthawk/common/statistic.h" +#include "nighthawk/source/common/statistic_impl.h" #include "external/envoy/source/common/common/logger.h" #include "external/envoy/source/common/http/http1/conn_pool.h" @@ -39,13 +40,10 @@ using namespace Envoy; // We need this because of macro expectations. COUNTER(http_5xx) \ COUNTER(http_xxx) \ COUNTER(pool_overflow) \ - COUNTER(pool_connection_failure) \ - COUNTER(total_req_sent) \ - HISTOGRAM(latency_on_success_req, Microseconds) \ - HISTOGRAM(latency_on_error_req, Microseconds) + COUNTER(pool_connection_failure) struct BenchmarkClientStats { - ALL_BENCHMARK_CLIENT_STATS(GENERATE_COUNTER_STRUCT, GENERATE_HISTOGRAM_STRUCT) + ALL_BENCHMARK_CLIENT_STATS(GENERATE_COUNTER_STRUCT) }; class Http1PoolImpl : public Envoy::Http::Http1::ProdConnPoolImpl { @@ -78,7 +76,10 @@ class BenchmarkClientHttpImpl : public BenchmarkClient, Envoy::Stats::Scope& scope, StatisticPtr&& connect_statistic, StatisticPtr&& response_statistic, StatisticPtr&& response_header_size_statistic, - StatisticPtr&& response_body_size_statistic, bool use_h2, + StatisticPtr&& response_body_size_statistic, + std::unique_ptr&& latency_2xx_statistic, + std::unique_ptr&& latency_xxx_statistic, + bool use_h2, Envoy::Upstream::ClusterManagerPtr& cluster_manager, Envoy::Tracing::HttpTracerSharedPtr& http_tracer, absl::string_view cluster_name, RequestGenerator request_generator, @@ -107,7 +108,7 @@ class BenchmarkClientHttpImpl : public BenchmarkClient, // StreamDecoderCompletionCallback void onComplete(bool success, const Envoy::Http::ResponseHeaderMap& headers) override; void onPoolFailure(Envoy::Http::ConnectionPool::PoolFailureReason reason) override; - void exportLatency(const uint32_t response_code, const uint64_t latency_us) override; + void exportLatency(const uint32_t response_code, const uint64_t latency_ns) override; // Helpers Envoy::Http::ConnectionPool::Instance* pool() { @@ -126,6 +127,8 @@ class BenchmarkClientHttpImpl : public BenchmarkClient, StatisticPtr response_statistic_; StatisticPtr response_header_size_statistic_; StatisticPtr response_body_size_statistic_; + std::unique_ptr latency_2xx_statistic_; + std::unique_ptr latency_xxx_statistic_; const bool use_h2_; std::chrono::seconds timeout_{5s}; uint32_t connection_limit_{1}; diff --git a/source/client/client_worker_impl.cc b/source/client/client_worker_impl.cc index 09aaba66a..69e3e2f28 100644 --- a/source/client/client_worker_impl.cc +++ b/source/client/client_worker_impl.cc @@ -33,7 +33,7 @@ ClientWorkerImpl::ClientWorkerImpl(Envoy::Api::Api& api, Envoy::ThreadLocal::Ins fmt::format("{}.requestsource", worker_number))), benchmark_client_(benchmark_client_factory.create( api, *dispatcher_, *worker_number_scope_, cluster_manager, http_tracer_, - fmt::format("{}", worker_number), *request_generator_)), + worker_number, *request_generator_)), phase_( std::make_unique("main", sequencer_factory_.create( diff --git a/source/client/factories_impl.cc b/source/client/factories_impl.cc index a2e3eb611..798c2a966 100644 --- a/source/client/factories_impl.cc +++ b/source/client/factories_impl.cc @@ -30,7 +30,7 @@ BenchmarkClientFactoryImpl::BenchmarkClientFactoryImpl(const Options& options) BenchmarkClientPtr BenchmarkClientFactoryImpl::create( Envoy::Api::Api& api, Envoy::Event::Dispatcher& dispatcher, Envoy::Stats::Scope& scope, Envoy::Upstream::ClusterManagerPtr& cluster_manager, - Envoy::Tracing::HttpTracerSharedPtr& http_tracer, absl::string_view cluster_name, + Envoy::Tracing::HttpTracerSharedPtr& http_tracer, const int worker_number, RequestSource& request_generator) const { StatisticFactoryImpl statistic_factory(options_); // While we lack options to configure which statistic backend goes where, we directly pass @@ -41,8 +41,11 @@ BenchmarkClientPtr BenchmarkClientFactoryImpl::create( // statistics. auto benchmark_client = std::make_unique( api, dispatcher, scope, statistic_factory.create(), statistic_factory.create(), - std::make_unique(), std::make_unique(), options_.h2(), - cluster_manager, http_tracer, cluster_name, request_generator.get(), !options_.openLoop()); + std::make_unique(), std::make_unique(), + std::make_unique(scope, worker_number), + std::make_unique(scope, worker_number), + options_.h2(), + cluster_manager, http_tracer, fmt::format("{}", worker_number), request_generator.get(), !options_.openLoop()); auto request_options = options_.toCommandLineOptions()->request_options(); benchmark_client->setConnectionLimit(options_.connections()); benchmark_client->setMaxPendingRequests(options_.maxPendingRequests()); diff --git a/source/client/factories_impl.h b/source/client/factories_impl.h index 928b63f99..fc45bf740 100644 --- a/source/client/factories_impl.h +++ b/source/client/factories_impl.h @@ -31,7 +31,7 @@ class BenchmarkClientFactoryImpl : public OptionBasedFactoryImpl, public Benchma Envoy::Stats::Scope& scope, Envoy::Upstream::ClusterManagerPtr& cluster_manager, Envoy::Tracing::HttpTracerSharedPtr& http_tracer, - absl::string_view cluster_name, + const int worker_number, RequestSource& request_generator) const override; }; diff --git a/source/client/stream_decoder.cc b/source/client/stream_decoder.cc index af25d6aaf..c36bd39c3 100644 --- a/source/client/stream_decoder.cc +++ b/source/client/stream_decoder.cc @@ -51,10 +51,12 @@ void StreamDecoder::onComplete(bool success) { latency_statistic_.addValue((time_source_.monotonicTime() - request_start_).count()); // At this point StreamDecoder::decodeHeaders() should have been called. if (stream_info_.response_code_.has_value()) { - const uint64_t latency_us = std::chrono::duration_cast( - time_source_.monotonicTime() - request_start_) - .count(); - decoder_completion_callback_.exportLatency(stream_info_.response_code_.value(), latency_us); + decoder_completion_callback_.exportLatency( + stream_info_.response_code_.value(), + (time_source_.monotonicTime() - request_start_).count()); + } + } else { + ENVOY_LOG(warn, "response_code is not available in onComplete()"); } } upstream_timing_.onLastUpstreamRxByteReceived(time_source_); diff --git a/source/client/stream_decoder.h b/source/client/stream_decoder.h index 6edb1b0e0..a9a110f74 100644 --- a/source/client/stream_decoder.h +++ b/source/client/stream_decoder.h @@ -24,7 +24,7 @@ class StreamDecoderCompletionCallback { virtual ~StreamDecoderCompletionCallback() = default; virtual void onComplete(bool success, const Envoy::Http::ResponseHeaderMap& headers) PURE; virtual void onPoolFailure(Envoy::Http::ConnectionPool::PoolFailureReason reason) PURE; - virtual void exportLatency(const uint32_t response_code, const uint64_t latency_us) PURE; + virtual void exportLatency(const uint32_t response_code, const uint64_t latency_ns) PURE; }; // TODO(oschaaf): create a StreamDecoderPool? diff --git a/test/benchmark_http_client_test.cc b/test/benchmark_http_client_test.cc index f535eed2a..774b54df4 100644 --- a/test/benchmark_http_client_test.cc +++ b/test/benchmark_http_client_test.cc @@ -126,7 +126,10 @@ class BenchmarkClientHttpTest : public Test { client_ = std::make_unique( *api_, *dispatcher_, mock_store_, std::make_unique(), std::make_unique(), std::make_unique(), - std::make_unique(), false, cluster_manager_, http_tracer_, "benchmark", + std::make_unique(), + std::make_unique(mock_store_, worker_number_), + std::make_unique(mock_store_, worker_number_), + false, cluster_manager_, http_tracer_, "benchmark", request_generator_, true); } @@ -161,34 +164,31 @@ class BenchmarkClientHttpTest : public Test { Envoy::Tracing::HttpTracerSharedPtr http_tracer_; std::string response_code_; RequestGenerator request_generator_; + int worker_number_{0}; }; TEST_F(BenchmarkClientHttpTest, BasicTestH1200) { response_code_ = "200"; testBasicFunctionality(2, 3, 10); EXPECT_EQ(5, getCounter("http_2xx")); - EXPECT_EQ(5, getCounter("total_req_sent")); } TEST_F(BenchmarkClientHttpTest, BasicTestH1300) { response_code_ = "300"; testBasicFunctionality(0, 11, 10); EXPECT_EQ(10, getCounter("http_3xx")); - EXPECT_EQ(10, getCounter("total_req_sent")); } TEST_F(BenchmarkClientHttpTest, BasicTestH1404) { response_code_ = "404"; testBasicFunctionality(0, 1, 10); EXPECT_EQ(1, getCounter("http_4xx")); - EXPECT_EQ(1, getCounter("total_req_sent")); } TEST_F(BenchmarkClientHttpTest, WeirdStatus) { response_code_ = "601"; testBasicFunctionality(0, 1, 10); EXPECT_EQ(1, getCounter("http_xxx")); - EXPECT_EQ(1, getCounter("total_req_sent")); } TEST_F(BenchmarkClientHttpTest, EnableLatencyMeasurement) { @@ -198,41 +198,51 @@ TEST_F(BenchmarkClientHttpTest, EnableLatencyMeasurement) { testBasicFunctionality(10, 1, 10); EXPECT_EQ(0, client_->statistics()["benchmark_http_client.queue_to_connect"]->count()); EXPECT_EQ(0, client_->statistics()["benchmark_http_client.request_to_response"]->count()); - EXPECT_EQ(10, getCounter("total_req_sent")); + EXPECT_EQ(0, client_->statistics()["benchmark_http_client.latency_2xx"]->count()); client_->setShouldMeasureLatencies(true); EXPECT_CALL(mock_store_, deliverHistogramToSinks(_, _)).Times(10); testBasicFunctionality(10, 1, 10); EXPECT_EQ(10, client_->statistics()["benchmark_http_client.queue_to_connect"]->count()); EXPECT_EQ(10, client_->statistics()["benchmark_http_client.request_to_response"]->count()); - EXPECT_EQ(20, getCounter("total_req_sent")); + EXPECT_EQ(10, client_->statistics()["benchmark_http_client.latency_2xx"]->count()); } TEST_F(BenchmarkClientHttpTest, ExportSuccessLatency) { setupBenchmarkClient(); uint64_t latency = 10; - EXPECT_CALL(mock_store_, deliverHistogramToSinks(Property(&Envoy::Stats::Metric::name, - "benchmark.latency_on_success_req"), - latency)) - .Times(1); - EXPECT_CALL(mock_store_, - deliverHistogramToSinks( - Property(&Envoy::Stats::Metric::name, "benchmark.latency_on_error_req"), latency)) + EXPECT_CALL(mock_store_, deliverHistogramToSinks( + Property(&Envoy::Stats::Metric::name, + "benchmark_http_client.latency_2xx"), + latency)) + .Times(2); + EXPECT_CALL(mock_store_, deliverHistogramToSinks( + Property(&Envoy::Stats::Metric::name, + "benchmark_http_client.latency_xxx"), + latency)) .Times(0); - client_->exportLatency(/*response_code=*/200, /*latency_us=*/latency); + client_->exportLatency(/*response_code=*/200, /*latency_ns=*/latency); + client_->exportLatency(/*response_code=*/200, /*latency_ns=*/latency); + EXPECT_EQ(2, client_->statistics()["benchmark_http_client.latency_2xx"]->count()); + EXPECT_DOUBLE_EQ(10, client_->statistics()["benchmark_http_client.latency_2xx"]->mean()); } TEST_F(BenchmarkClientHttpTest, ExportErrorLatency) { setupBenchmarkClient(); - uint64_t latency = 10; - EXPECT_CALL(mock_store_, deliverHistogramToSinks(Property(&Envoy::Stats::Metric::name, - "benchmark.latency_on_success_req"), - latency)) + EXPECT_CALL(mock_store_, deliverHistogramToSinks( + Property(&Envoy::Stats::Metric::name, + "benchmark_http_client.latency_xxx"), + _)) + .Times(3); + EXPECT_CALL(mock_store_, deliverHistogramToSinks( + Property(&Envoy::Stats::Metric::name, + "benchmark_http_client.latency_2xx"), + _)) .Times(0); - EXPECT_CALL(mock_store_, - deliverHistogramToSinks( - Property(&Envoy::Stats::Metric::name, "benchmark.latency_on_error_req"), latency)) - .Times(1); - client_->exportLatency(/*response_code=*/500, /*latency_us=*/latency); + client_->exportLatency(/*response_code=*/500, /*latency_ns=*/1); + client_->exportLatency(/*response_code=*/100, /*latency_ns=*/2); + client_->exportLatency(/*response_code=*/600, /*latency_ns=*/3); + EXPECT_EQ(3, client_->statistics()["benchmark_http_client.latency_xxx"]->count()); + EXPECT_DOUBLE_EQ(2, client_->statistics()["benchmark_http_client.latency_xxx"]->mean()); } TEST_F(BenchmarkClientHttpTest, StatusTrackingInOnComplete) { diff --git a/test/integration/test_integration_basics.py b/test/integration/test_integration_basics.py index f7ba007dd..8cc7e2fbd 100644 --- a/test/integration/test_integration_basics.py +++ b/test/integration/test_integration_basics.py @@ -38,7 +38,6 @@ def test_http_h1(http_test_server_fixture): assertCounterEqual(counters, "upstream_rq_pending_total", 1) assertCounterEqual(counters, "upstream_rq_total", 25) assertCounterEqual(counters, "default.total_match_count", 1) - assertCounterEqual(counters, "benchmark.total_req_sent", 25) global_histograms = http_test_server_fixture.getNighthawkGlobalHistogramsbyIdFromJson(parsed_json) assertEqual(int(global_histograms["benchmark_http_client.response_body_size"]["count"]), 25) @@ -52,7 +51,7 @@ def test_http_h1(http_test_server_fixture): assertEqual(int(global_histograms["benchmark_http_client.response_body_size"]["raw_pstdev"]), 0) assertEqual(int(global_histograms["benchmark_http_client.response_header_size"]["raw_pstdev"]), 0) - assertEqual(len(counters), 13) + assertEqual(len(counters), 12) def mini_stress_test(fixture, args): @@ -184,8 +183,7 @@ def test_http_h2(http_test_server_fixture): assertCounterEqual(counters, "upstream_rq_pending_total", 1) assertCounterEqual(counters, "upstream_rq_total", 25) assertCounterEqual(counters, "default.total_match_count", 1) - assertCounterEqual(counters, "benchmark.total_req_sent", 25) - assertEqual(len(counters), 13) + assertEqual(len(counters), 12) def test_http_concurrency(http_test_server_fixture): @@ -232,8 +230,7 @@ def test_https_h1(https_test_server_fixture): assertCounterEqual(counters, "ssl.sigalgs.rsa_pss_rsae_sha256", 1) assertCounterEqual(counters, "ssl.versions.TLSv1.2", 1) assertCounterEqual(counters, "default.total_match_count", 1) - assertCounterEqual(counters, "benchmark.total_req_sent", 25) - assertEqual(len(counters), 18) + assertEqual(len(counters), 17) server_stats = https_test_server_fixture.getTestServerStatisticsJson() assertEqual( @@ -269,8 +266,7 @@ def test_https_h2(https_test_server_fixture): assertCounterEqual(counters, "ssl.sigalgs.rsa_pss_rsae_sha256", 1) assertCounterEqual(counters, "ssl.versions.TLSv1.2", 1) assertCounterEqual(counters, "default.total_match_count", 1) - assertCounterEqual(counters, "benchmark.total_req_sent", 25) - assertEqual(len(counters), 18) + assertEqual(len(counters), 17) @pytest.mark.parametrize('server_config', From 4ce94f293381182901a32a6cc2981b059dcc1388 Mon Sep 17 00:00:00 2001 From: qqustc Date: Mon, 13 Jul 2020 20:49:23 -0400 Subject: [PATCH 22/44] delete stat.doc Signed-off-by: qqustc --- docs/root/statistics.md | 63 --------------------------- source/client/benchmark_client_impl.h | 2 +- 2 files changed, 1 insertion(+), 64 deletions(-) delete mode 100644 docs/root/statistics.md diff --git a/docs/root/statistics.md b/docs/root/statistics.md deleted file mode 100644 index a8926f672..000000000 --- a/docs/root/statistics.md +++ /dev/null @@ -1,63 +0,0 @@ -# Nighthawk Statistics - -## Background -Currently Nighthawk only outputs metrics at the end of a test run. There are no metrics streamed during a test run. In order to collect all the useful Nighthawk metrics, we plan to instrument Nighthawk with the functionality to stream its metrics. - - -## Statistics in BenchMarkClient -Per worker Statistics are defined in [benchmark_client_impl.h](https://github.com/envoyproxy/nighthawk/blob/master/source/client/benchmark_client_impl.h) - -Name | Type | Description ------| ----- | ---------------- -total_req_sent | Counter | Total number of requests sent from Nighthawk -http_xxx | Counter | Total number of response with code xxx -stream_resets | Counter | Total number of sream reset -pool_overflow | Counter | Total number of times connection pool overflowed -pool_connection_failure | Counter | Total number of times pool connection failed -latency_on_success_req | Histogram | Latency (in Microseconds) histogram of successful request with code 2xx -latency_on_error_req | Histogram | Latency (in Microseconds) histogram of error request with code other than 2xx - -## Envoy Metrics Model -Since Nighthawk is built on top of [Envoy](https://github.com/envoyproxy/envoy) and similar metrics have been exported from Envoy, it is natural to follow the example in Envoy. Furthermore *Envoy typed metrics are already being used in Nighthawk* ([example](https://github.com/envoyproxy/nighthawk/blob/master/source/client/benchmark_client_impl.h#L33-L46)). - - -Envoy has 3 types of metrics -- Counters: Unsigned integers (can only increase) represent how many times an event happens, e.g. total number of requests. -- Gauges: Unsigned integers (can increase or decrease), e.g. number of active connections. -- Histograms: Unsigned integers that will yield summarized percentile values. E.g. latency distributions. - - -In Envoy, the stat [Store](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/include/envoy/stats/store.h#L29) is a singleton and provides a simple interface by which the rest of the code can obtain handles to scopes, counters, gauges, and histograms. Envoy counters and gauges are periodically (configured at ~5 sec interval) flushed to the sinks. Note that currently histogram values are sent directly to the sinks. A stat [Sink](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/include/envoy/stats/sink.h#L48) is an interface that takes generic stat data and translates it into a backend-specific wire format. Currently Envoy supports the TCP and UDP [statsd](https://github.com/b/statsd_spec) protocol (implemented in [statsd.h](https://github.com/envoyproxy/envoy/blob/master/source/extensions/stat_sinks/common/statsd/statsd.h)). Users can create their own Sink subclass to translate Envoy metrics into backend-specific format. - -Envoy metrics can be defined using a macro, e.g. -``` -#define ALL_CLUSTER_STATS(COUNTER, GAUGE, HISTOGRAM) - COUNTER(upstream_cx_total) - GAUGE(upstream_cx_active, NeverImport) - HISTOGRAM(upstream_cx_length, Milliseconds) -struct ClusterStats { - ALL_CLUSTER_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT) -}; -``` - -## Envoy Metrics Limitation -Currently Envoy metrics don't support key-value map. As a result, for metrics to be broken down by certain dimensions, we need to define a separate metric for each dimension. For example, currently Nighthawk defines separate [counters](https://github.com/envoyproxy/nighthawk/blob/master/source/client/benchmark_client_impl.h#L35-L40) to monitor the number of responses with corresponding response code. - -Due to this limitation, for the latency metric we define 2 metrics: *latency_on_success_req* (to capture latency of successful requests with 2xx) and *latency_on_error_req* (to capture latency of error requests with code other than 2xx). - - -## Envoy Metrics Flush -Envoy uses a flush timer to periodically flush metrics into stat sinks ([here](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/source/server/server.cc#L479-L480)) at a configured interval (default to 5 sec). For every metric flush, Envoy will call the function [flushMetricsToSinks()](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/source/server/server.cc#L175) to create a metric snapshot from Envoy stat store and flush the snapshot to all sinks through `sink->flush(snapshot)`. - - -## Metrics Export in Nighthawk -Currently a single Nighthawk can run with multiple workers. In the future, Nighthawk will be extended to be able to run multiple instances together. Since each Nighthawk worker sends requests independently, we decide to export per worker level metrics since it provides several advantages over global level metrics (aggregated across all workers). Notice that *there are existing Nighthawk metrics already defined at per worker level* ([example](https://github.com/envoyproxy/nighthawk/blob/bc72a52efdc489beaa0844b34f136e03394bd355/source/client/benchmark_client_impl.cc#L61)). -- Per worker level metrics provide information about the performance of each worker which will be hidden by the global level metrics. -- Keep the workers independent which makes it easier/efficient to scale up to multiple Nighthawks with large numbers of workers. - -Metrics can be defined at per worker level using [Scope](https://github.com/envoyproxy/envoy/blob/e9c2c8c4a0141c9634316e8283f98f412d0dd207/include/envoy/stats/scope.h#L35) ( e.g. `cluster..total_request_sent`). The dynamic portions of metric (e.g. `worker_id`) can be embedded into the metric name. A [TagSpecifier](https://github.com/envoyproxy/envoy/blob/7a652daf35d7d4a6a6bad5a010fe65947ee4411a/api/envoy/config/metrics/v3/stats.proto#L182) can be specified in the bootstrap configuration, which will transform dynamic portions into tags. When per worker level metrics are exported from Nighthawk, multiple per worker level metrics can be converted into a single metric with a `worker_id` label in the stat Sink if the corresponding backend metric supports key-value map. - -## Reference -- [Nighthawk: architecture and key concepts](https://github.com/envoyproxy/nighthawk/blob/master/docs/root/overview.md) -- [Envoy Stats System](https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md) -- [Envoy Stats blog](https://blog.envoyproxy.io/envoy-stats-b65c7f363342) diff --git a/source/client/benchmark_client_impl.h b/source/client/benchmark_client_impl.h index 996ae77f0..75f860370 100644 --- a/source/client/benchmark_client_impl.h +++ b/source/client/benchmark_client_impl.h @@ -31,7 +31,7 @@ using namespace std::chrono_literals; using namespace Envoy; // We need this because of macro expectations. -#define ALL_BENCHMARK_CLIENT_STATS(COUNTER, HISTOGRAM) \ +#define ALL_BENCHMARK_CLIENT_STATS(COUNTER) \ COUNTER(stream_resets) \ COUNTER(http_1xx) \ COUNTER(http_2xx) \ From 0f4fc953dcbf0d85605309714ef99cd46889cd95 Mon Sep 17 00:00:00 2001 From: qqustc Date: Tue, 14 Jul 2020 11:55:13 -0400 Subject: [PATCH 23/44] add latency stat Signed-off-by: qqustc --- source/client/benchmark_client_impl.cc | 41 ++++++++++----- source/client/benchmark_client_impl.h | 28 ++++++++--- source/client/factories_impl.cc | 17 +++++-- source/common/statistic_impl.h | 1 + test/benchmark_http_client_test.cc | 69 ++++++++++++++++++++------ 5 files changed, 116 insertions(+), 40 deletions(-) diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index a122cb2a4..a0304a02c 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -49,20 +49,21 @@ Http1PoolImpl::newStream(Envoy::Http::ResponseDecoder& response_decoder, BenchmarkClientHttpImpl::BenchmarkClientHttpImpl( Envoy::Api::Api& api, Envoy::Event::Dispatcher& dispatcher, Envoy::Stats::Scope& scope, - StatisticPtr&& connect_statistic, StatisticPtr&& response_statistic, - StatisticPtr&& response_header_size_statistic, StatisticPtr&& response_body_size_statistic, - std::unique_ptr&& latency_2xx_statistic, - std::unique_ptr&& latency_xxx_statistic, - bool use_h2, Envoy::Upstream::ClusterManagerPtr& cluster_manager, + BenchmarkClientStatistic& statistic, bool use_h2, + Envoy::Upstream::ClusterManagerPtr& cluster_manager, Envoy::Tracing::HttpTracerSharedPtr& http_tracer, absl::string_view cluster_name, RequestGenerator request_generator, const bool provide_resource_backpressure) : api_(api), dispatcher_(dispatcher), scope_(scope.createScope("benchmark.")), - connect_statistic_(std::move(connect_statistic)), - response_statistic_(std::move(response_statistic)), - response_header_size_statistic_(std::move(response_header_size_statistic)), - response_body_size_statistic_(std::move(response_body_size_statistic)), - latency_2xx_statistic_(std::move(latency_2xx_statistic)), - latency_xxx_statistic_(std::move(latency_xxx_statistic)), + connect_statistic_(std::move(statistic.connect_statistic)), + response_statistic_(std::move(statistic.response_statistic)), + response_header_size_statistic_(std::move(statistic.response_header_size_statistic)), + response_body_size_statistic_(std::move(statistic.response_body_size_statistic)), + latency_1xx_statistic_(std::move(statistic.latency_1xx_statistic)), + latency_2xx_statistic_(std::move(statistic.latency_2xx_statistic)), + latency_3xx_statistic_(std::move(statistic.latency_3xx_statistic)), + latency_4xx_statistic_(std::move(statistic.latency_4xx_statistic)), + latency_5xx_statistic_(std::move(statistic.latency_5xx_statistic)), + latency_xxx_statistic_(std::move(statistic.latency_xxx_statistic)), use_h2_(use_h2), benchmark_client_stats_({ALL_BENCHMARK_CLIENT_STATS(POOL_COUNTER(*scope_))}), cluster_manager_(cluster_manager), http_tracer_(http_tracer), @@ -72,7 +73,11 @@ BenchmarkClientHttpImpl::BenchmarkClientHttpImpl( response_statistic_->setId("benchmark_http_client.request_to_response"); response_header_size_statistic_->setId("benchmark_http_client.response_header_size"); response_body_size_statistic_->setId("benchmark_http_client.response_body_size"); + latency_1xx_statistic_->setId("benchmark_http_client.latency_1xx"); latency_2xx_statistic_->setId("benchmark_http_client.latency_2xx"); + latency_3xx_statistic_->setId("benchmark_http_client.latency_3xx"); + latency_4xx_statistic_->setId("benchmark_http_client.latency_4xx"); + latency_5xx_statistic_->setId("benchmark_http_client.latency_5xx"); latency_xxx_statistic_->setId("benchmark_http_client.latency_xxx"); } @@ -90,7 +95,11 @@ StatisticPtrMap BenchmarkClientHttpImpl::statistics() const { statistics[response_statistic_->id()] = response_statistic_.get(); statistics[response_header_size_statistic_->id()] = response_header_size_statistic_.get(); statistics[response_body_size_statistic_->id()] = response_body_size_statistic_.get(); + statistics[latency_1xx_statistic_->id()] = latency_1xx_statistic_.get(); statistics[latency_2xx_statistic_->id()] = latency_2xx_statistic_.get(); + statistics[latency_3xx_statistic_->id()] = latency_3xx_statistic_.get(); + statistics[latency_4xx_statistic_->id()] = latency_4xx_statistic_.get(); + statistics[latency_5xx_statistic_->id()] = latency_5xx_statistic_.get(); statistics[latency_xxx_statistic_->id()] = latency_xxx_statistic_.get(); return statistics; }; @@ -180,8 +189,16 @@ void BenchmarkClientHttpImpl::onPoolFailure(Envoy::Http::ConnectionPool::PoolFai void BenchmarkClientHttpImpl::exportLatency(const uint32_t response_code, const uint64_t latency_ns) { - if (response_code > 199 && response_code <= 299) { + if (response_code > 99 && response_code <= 199) { + latency_1xx_statistic_->recordValue(latency_ns); + } else if (response_code > 199 && response_code <= 299) { latency_2xx_statistic_->recordValue(latency_ns); + } else if (response_code > 299 && response_code <= 399) { + latency_3xx_statistic_->recordValue(latency_ns); + } else if (response_code > 399 && response_code <= 499) { + latency_4xx_statistic_->recordValue(latency_ns); + } else if (response_code > 499 && response_code <= 599) { + latency_5xx_statistic_->recordValue(latency_ns); } else { latency_xxx_statistic_->recordValue(latency_ns); } diff --git a/source/client/benchmark_client_impl.h b/source/client/benchmark_client_impl.h index 75f860370..0457bbe41 100644 --- a/source/client/benchmark_client_impl.h +++ b/source/client/benchmark_client_impl.h @@ -46,6 +46,20 @@ struct BenchmarkClientStats { ALL_BENCHMARK_CLIENT_STATS(GENERATE_COUNTER_STRUCT) }; +// For histogram metrics, Nighthawk Statistic is used instead of Envoy Histogram. +struct BenchmarkClientStatistic { + StatisticPtr connect_statistic; + StatisticPtr response_statistic; + StatisticPtr response_header_size_statistic; + StatisticPtr response_body_size_statistic; + std::unique_ptr latency_1xx_statistic; + std::unique_ptr latency_2xx_statistic; + std::unique_ptr latency_3xx_statistic; + std::unique_ptr latency_4xx_statistic; + std::unique_ptr latency_5xx_statistic; + std::unique_ptr latency_xxx_statistic; +}; + class Http1PoolImpl : public Envoy::Http::Http1::ProdConnPoolImpl { public: enum class ConnectionReuseStrategy { @@ -73,14 +87,8 @@ class BenchmarkClientHttpImpl : public BenchmarkClient, public Envoy::Logger::Loggable { public: BenchmarkClientHttpImpl(Envoy::Api::Api& api, Envoy::Event::Dispatcher& dispatcher, - Envoy::Stats::Scope& scope, StatisticPtr&& connect_statistic, - StatisticPtr&& response_statistic, - StatisticPtr&& response_header_size_statistic, - StatisticPtr&& response_body_size_statistic, - std::unique_ptr&& latency_2xx_statistic, - std::unique_ptr&& latency_xxx_statistic, - bool use_h2, - Envoy::Upstream::ClusterManagerPtr& cluster_manager, + Envoy::Stats::Scope& scope, BenchmarkClientStatistic& statistic, + bool use_h2, Envoy::Upstream::ClusterManagerPtr& cluster_manager, Envoy::Tracing::HttpTracerSharedPtr& http_tracer, absl::string_view cluster_name, RequestGenerator request_generator, const bool provide_resource_backpressure); @@ -127,7 +135,11 @@ class BenchmarkClientHttpImpl : public BenchmarkClient, StatisticPtr response_statistic_; StatisticPtr response_header_size_statistic_; StatisticPtr response_body_size_statistic_; + std::unique_ptr latency_1xx_statistic_; std::unique_ptr latency_2xx_statistic_; + std::unique_ptr latency_3xx_statistic_; + std::unique_ptr latency_4xx_statistic_; + std::unique_ptr latency_5xx_statistic_; std::unique_ptr latency_xxx_statistic_; const bool use_h2_; std::chrono::seconds timeout_{5s}; diff --git a/source/client/factories_impl.cc b/source/client/factories_impl.cc index 798c2a966..494dae556 100644 --- a/source/client/factories_impl.cc +++ b/source/client/factories_impl.cc @@ -39,12 +39,19 @@ BenchmarkClientPtr BenchmarkClientFactoryImpl::create( // NullStatistic). // TODO(#292): Create options and have the StatisticFactory consider those when instantiating // statistics. + BenchmarkClientStatistic statistic; + statistic.connect_statistic = statistic_factory.create(); + statistic.response_statistic = statistic_factory.create(); + statistic.response_header_size_statistic = std::make_unique(); + statistic.response_body_size_statistic = std::make_unique(); + statistic.latency_1xx_statistic = std::make_unique(scope, worker_number); + statistic.latency_2xx_statistic = std::make_unique(scope, worker_number); + statistic.latency_3xx_statistic = std::make_unique(scope, worker_number); + statistic.latency_4xx_statistic = std::make_unique(scope, worker_number); + statistic.latency_5xx_statistic = std::make_unique(scope, worker_number); + statistic.latency_xxx_statistic = std::make_unique(scope, worker_number); auto benchmark_client = std::make_unique( - api, dispatcher, scope, statistic_factory.create(), statistic_factory.create(), - std::make_unique(), std::make_unique(), - std::make_unique(scope, worker_number), - std::make_unique(scope, worker_number), - options_.h2(), + api, dispatcher, scope, statistic, options_.h2(), cluster_manager, http_tracer, fmt::format("{}", worker_number), request_generator.get(), !options_.openLoop()); auto request_options = options_.toCommandLineOptions()->request_options(); benchmark_client->setConnectionLimit(options_.connections()); diff --git a/source/common/statistic_impl.h b/source/common/statistic_impl.h index 1239d5881..20ba3de01 100644 --- a/source/common/statistic_impl.h +++ b/source/common/statistic_impl.h @@ -199,6 +199,7 @@ class SinkableStatistic : public Envoy::Stats::HistogramImplHelper { // statistic is not defined per worker. const absl::optional worker_id() { return worker_id_; } +protected: // This is used for delivering the histogram data to sinks. Envoy::Stats::Scope& scope_; diff --git a/test/benchmark_http_client_test.cc b/test/benchmark_http_client_test.cc index 774b54df4..4c253db8a 100644 --- a/test/benchmark_http_client_test.cc +++ b/test/benchmark_http_client_test.cc @@ -54,6 +54,22 @@ class BenchmarkClientHttpTest : public Test { {{":scheme", "http"}, {":method", "GET"}, {":path", "/"}, {":host", "localhost"}})); return std::make_unique(header); }; + statistic_.connect_statistic = std::make_unique(); + statistic_.response_statistic = std::make_unique(); + statistic_.response_header_size_statistic = std::make_unique(); + statistic_.response_body_size_statistic = std::make_unique(); + statistic_.latency_1xx_statistic = + std::make_unique(mock_store_, worker_number_); + statistic_.latency_2xx_statistic = + std::make_unique(mock_store_, worker_number_); + statistic_.latency_3xx_statistic = + std::make_unique(mock_store_, worker_number_); + statistic_.latency_4xx_statistic = + std::make_unique(mock_store_, worker_number_); + statistic_.latency_5xx_statistic = + std::make_unique(mock_store_, worker_number_); + statistic_.latency_xxx_statistic = + std::make_unique(mock_store_, worker_number_); } void testBasicFunctionality(const uint64_t max_pending, const uint64_t connection_limit, @@ -124,11 +140,7 @@ class BenchmarkClientHttpTest : public Test { void setupBenchmarkClient() { client_ = std::make_unique( - *api_, *dispatcher_, mock_store_, std::make_unique(), - std::make_unique(), std::make_unique(), - std::make_unique(), - std::make_unique(mock_store_, worker_number_), - std::make_unique(mock_store_, worker_number_), + *api_, *dispatcher_, mock_store_, statistic_, false, cluster_manager_, http_tracer_, "benchmark", request_generator_, true); } @@ -165,6 +177,7 @@ class BenchmarkClientHttpTest : public Test { std::string response_code_; RequestGenerator request_generator_; int worker_number_{0}; + Client::BenchmarkClientStatistic statistic_; }; TEST_F(BenchmarkClientHttpTest, BasicTestH1200) { @@ -223,28 +236,54 @@ TEST_F(BenchmarkClientHttpTest, ExportSuccessLatency) { client_->exportLatency(/*response_code=*/200, /*latency_ns=*/latency); client_->exportLatency(/*response_code=*/200, /*latency_ns=*/latency); EXPECT_EQ(2, client_->statistics()["benchmark_http_client.latency_2xx"]->count()); - EXPECT_DOUBLE_EQ(10, client_->statistics()["benchmark_http_client.latency_2xx"]->mean()); + EXPECT_DOUBLE_EQ(latency, client_->statistics()["benchmark_http_client.latency_2xx"]->mean()); } TEST_F(BenchmarkClientHttpTest, ExportErrorLatency) { setupBenchmarkClient(); EXPECT_CALL(mock_store_, deliverHistogramToSinks( Property(&Envoy::Stats::Metric::name, - "benchmark_http_client.latency_xxx"), + "benchmark_http_client.latency_1xx"), _)) - .Times(3); + .Times(1); EXPECT_CALL(mock_store_, deliverHistogramToSinks( Property(&Envoy::Stats::Metric::name, - "benchmark_http_client.latency_2xx"), + "benchmark_http_client.latency_3xx"), _)) - .Times(0); - client_->exportLatency(/*response_code=*/500, /*latency_ns=*/1); - client_->exportLatency(/*response_code=*/100, /*latency_ns=*/2); - client_->exportLatency(/*response_code=*/600, /*latency_ns=*/3); - EXPECT_EQ(3, client_->statistics()["benchmark_http_client.latency_xxx"]->count()); - EXPECT_DOUBLE_EQ(2, client_->statistics()["benchmark_http_client.latency_xxx"]->mean()); + .Times(1); + EXPECT_CALL(mock_store_, deliverHistogramToSinks( + Property(&Envoy::Stats::Metric::name, + "benchmark_http_client.latency_4xx"), + _)) + .Times(1); + EXPECT_CALL(mock_store_, deliverHistogramToSinks( + Property(&Envoy::Stats::Metric::name, + "benchmark_http_client.latency_5xx"), + _)) + .Times(1); + EXPECT_CALL(mock_store_, deliverHistogramToSinks( + Property(&Envoy::Stats::Metric::name, + "benchmark_http_client.latency_xxx"), + _)) + .Times(1); + client_->exportLatency(/*response_code=*/100, /*latency_ns=*/1); + client_->exportLatency(/*response_code=*/300, /*latency_ns=*/3); + client_->exportLatency(/*response_code=*/400, /*latency_ns=*/4); + client_->exportLatency(/*response_code=*/500, /*latency_ns=*/5); + client_->exportLatency(/*response_code=*/600, /*latency_ns=*/6); + EXPECT_EQ(1, client_->statistics()["benchmark_http_client.latency_1xx"]->count()); + EXPECT_DOUBLE_EQ(1, client_->statistics()["benchmark_http_client.latency_1xx"]->mean()); + EXPECT_EQ(1, client_->statistics()["benchmark_http_client.latency_xxx"]->count()); + EXPECT_DOUBLE_EQ(3, client_->statistics()["benchmark_http_client.latency_3xx"]->mean()); + EXPECT_EQ(1, client_->statistics()["benchmark_http_client.latency_xxx"]->count()); + EXPECT_DOUBLE_EQ(4, client_->statistics()["benchmark_http_client.latency_4xx"]->mean()); + EXPECT_EQ(1, client_->statistics()["benchmark_http_client.latency_xxx"]->count()); + EXPECT_DOUBLE_EQ(5, client_->statistics()["benchmark_http_client.latency_5xx"]->mean()); + EXPECT_EQ(1, client_->statistics()["benchmark_http_client.latency_xxx"]->count()); + EXPECT_DOUBLE_EQ(6, client_->statistics()["benchmark_http_client.latency_xxx"]->mean()); } + TEST_F(BenchmarkClientHttpTest, StatusTrackingInOnComplete) { setupBenchmarkClient(); Envoy::Http::ResponseHeaderMapPtr header = Envoy::Http::ResponseHeaderMapImpl::create(); From 190299c4ebf608ce35ace7c4b4d57cdc5e387935 Mon Sep 17 00:00:00 2001 From: qqustc Date: Tue, 14 Jul 2020 12:23:28 -0400 Subject: [PATCH 24/44] fix include Signed-off-by: qqustc --- nighthawk | 1 + source/client/benchmark_client_impl.h | 2 +- test/mocks/client/mock_benchmark_client_factory.h | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) create mode 160000 nighthawk diff --git a/nighthawk b/nighthawk new file mode 160000 index 000000000..0f4fc953d --- /dev/null +++ b/nighthawk @@ -0,0 +1 @@ +Subproject commit 0f4fc953dcbf0d85605309714ef99cd46889cd95 diff --git a/source/client/benchmark_client_impl.h b/source/client/benchmark_client_impl.h index 0457bbe41..a8b0c6cee 100644 --- a/source/client/benchmark_client_impl.h +++ b/source/client/benchmark_client_impl.h @@ -13,7 +13,6 @@ #include "nighthawk/common/request_source.h" #include "nighthawk/common/sequencer.h" #include "nighthawk/common/statistic.h" -#include "nighthawk/source/common/statistic_impl.h" #include "external/envoy/source/common/common/logger.h" #include "external/envoy/source/common/http/http1/conn_pool.h" @@ -23,6 +22,7 @@ #include "api/client/options.pb.h" #include "client/stream_decoder.h" +#include "common/statistic_impl.h" namespace Nighthawk { namespace Client { diff --git a/test/mocks/client/mock_benchmark_client_factory.h b/test/mocks/client/mock_benchmark_client_factory.h index d67db595c..0c8d0ef9f 100644 --- a/test/mocks/client/mock_benchmark_client_factory.h +++ b/test/mocks/client/mock_benchmark_client_factory.h @@ -17,7 +17,7 @@ class MockBenchmarkClientFactory : public BenchmarkClientFactory { MOCK_CONST_METHOD7(create, BenchmarkClientPtr(Envoy::Api::Api&, Envoy::Event::Dispatcher&, Envoy::Stats::Scope&, Envoy::Upstream::ClusterManagerPtr&, - Envoy::Tracing::HttpTracerSharedPtr&, absl::string_view, + Envoy::Tracing::HttpTracerSharedPtr&, const int, RequestSource& request_generator)); }; From 4ef322d687c8142becddf9b5763e90766b583716 Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Tue, 14 Jul 2020 12:59:01 -0400 Subject: [PATCH 25/44] fix stream_decoder.cc Signed-off-by: Qin Qin --- source/client/stream_decoder.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/source/client/stream_decoder.cc b/source/client/stream_decoder.cc index c36bd39c3..f442c1b31 100644 --- a/source/client/stream_decoder.cc +++ b/source/client/stream_decoder.cc @@ -55,9 +55,6 @@ void StreamDecoder::onComplete(bool success) { stream_info_.response_code_.value(), (time_source_.monotonicTime() - request_start_).count()); } - } else { - ENVOY_LOG(warn, "response_code is not available in onComplete()"); - } } upstream_timing_.onLastUpstreamRxByteReceived(time_source_); response_body_sizes_statistic_.addValue(stream_info_.bytesSent()); From 66e1e89be24ca41ba39cbfba2b374d8364c810ed Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Tue, 14 Jul 2020 14:30:34 -0400 Subject: [PATCH 26/44] fix factories_test.cc Signed-off-by: Qin Qin --- test/factories_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/factories_test.cc b/test/factories_test.cc index 2f82f96e5..68428c53b 100644 --- a/test/factories_test.cc +++ b/test/factories_test.cc @@ -46,7 +46,7 @@ TEST_F(FactoriesTest, CreateBenchmarkClient) { StaticRequestSourceImpl request_generator( std::make_unique()); auto benchmark_client = factory.create(*api_, dispatcher_, stats_store_, cluster_manager, - http_tracer_, "foocluster", request_generator); + http_tracer_, /*worker_number=*/0, request_generator); EXPECT_NE(nullptr, benchmark_client.get()); } From a0cba74afbd5257d210df4ace83dd61834e08358 Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Tue, 14 Jul 2020 17:50:06 -0400 Subject: [PATCH 27/44] format file Signed-off-by: Qin Qin --- include/nighthawk/client/factories.h | 6 +-- source/client/benchmark_client_impl.cc | 3 +- source/client/benchmark_client_impl.h | 3 +- source/client/client_worker_impl.cc | 6 +-- source/client/factories_impl.cc | 4 +- test/benchmark_http_client_test.cc | 55 +++++++++------------ test/integration/test_integration_basics.py | 24 +++++---- 7 files changed, 48 insertions(+), 53 deletions(-) diff --git a/include/nighthawk/client/factories.h b/include/nighthawk/client/factories.h index 0fa6a3803..d7c7796b9 100644 --- a/include/nighthawk/client/factories.h +++ b/include/nighthawk/client/factories.h @@ -33,9 +33,9 @@ class BenchmarkClientFactory { * @param scope stats scope for any stats tracked by the benchmark client. * @param cluster_manager Cluster manager preconfigured with our target cluster. * @param http_tracer Shared pointer to an http tracer implementation (e.g. Zipkin). - * @param worker_number Worker id which is also the name of the cluster that this benchmark client will use. In conjunction - * with cluster_manager this will allow the this BenchmarkClient to access the target connection - * pool. + * @param worker_number Worker id which is also the name of the cluster that this benchmark client + * will use. In conjunction with cluster_manager this will allow the this BenchmarkClient to + * access the target connection pool. * @param request_source Source of request-specifiers. Will be queries every time the * BenchmarkClient is asked to issue a request. * diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index a0304a02c..67da9ddb5 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -63,8 +63,7 @@ BenchmarkClientHttpImpl::BenchmarkClientHttpImpl( latency_3xx_statistic_(std::move(statistic.latency_3xx_statistic)), latency_4xx_statistic_(std::move(statistic.latency_4xx_statistic)), latency_5xx_statistic_(std::move(statistic.latency_5xx_statistic)), - latency_xxx_statistic_(std::move(statistic.latency_xxx_statistic)), - use_h2_(use_h2), + latency_xxx_statistic_(std::move(statistic.latency_xxx_statistic)), use_h2_(use_h2), benchmark_client_stats_({ALL_BENCHMARK_CLIENT_STATS(POOL_COUNTER(*scope_))}), cluster_manager_(cluster_manager), http_tracer_(http_tracer), cluster_name_(std::string(cluster_name)), request_generator_(std::move(request_generator)), diff --git a/source/client/benchmark_client_impl.h b/source/client/benchmark_client_impl.h index a8b0c6cee..7a18e5af4 100644 --- a/source/client/benchmark_client_impl.h +++ b/source/client/benchmark_client_impl.h @@ -21,9 +21,10 @@ #include "api/client/options.pb.h" -#include "client/stream_decoder.h" #include "common/statistic_impl.h" +#include "client/stream_decoder.h" + namespace Nighthawk { namespace Client { diff --git a/source/client/client_worker_impl.cc b/source/client/client_worker_impl.cc index 69e3e2f28..d9ebb158a 100644 --- a/source/client/client_worker_impl.cc +++ b/source/client/client_worker_impl.cc @@ -31,9 +31,9 @@ ClientWorkerImpl::ClientWorkerImpl(Envoy::Api::Api& api, Envoy::ThreadLocal::Ins request_generator_( request_generator_factory.create(cluster_manager, *dispatcher_, *worker_number_scope_, fmt::format("{}.requestsource", worker_number))), - benchmark_client_(benchmark_client_factory.create( - api, *dispatcher_, *worker_number_scope_, cluster_manager, http_tracer_, - worker_number, *request_generator_)), + benchmark_client_(benchmark_client_factory.create(api, *dispatcher_, *worker_number_scope_, + cluster_manager, http_tracer_, + worker_number, *request_generator_)), phase_( std::make_unique("main", sequencer_factory_.create( diff --git a/source/client/factories_impl.cc b/source/client/factories_impl.cc index 494dae556..2a5d193eb 100644 --- a/source/client/factories_impl.cc +++ b/source/client/factories_impl.cc @@ -51,8 +51,8 @@ BenchmarkClientPtr BenchmarkClientFactoryImpl::create( statistic.latency_5xx_statistic = std::make_unique(scope, worker_number); statistic.latency_xxx_statistic = std::make_unique(scope, worker_number); auto benchmark_client = std::make_unique( - api, dispatcher, scope, statistic, options_.h2(), - cluster_manager, http_tracer, fmt::format("{}", worker_number), request_generator.get(), !options_.openLoop()); + api, dispatcher, scope, statistic, options_.h2(), cluster_manager, http_tracer, + fmt::format("{}", worker_number), request_generator.get(), !options_.openLoop()); auto request_options = options_.toCommandLineOptions()->request_options(); benchmark_client->setConnectionLimit(options_.connections()); benchmark_client->setMaxPendingRequests(options_.maxPendingRequests()); diff --git a/test/benchmark_http_client_test.cc b/test/benchmark_http_client_test.cc index 4c253db8a..64b5a5f1e 100644 --- a/test/benchmark_http_client_test.cc +++ b/test/benchmark_http_client_test.cc @@ -140,9 +140,8 @@ class BenchmarkClientHttpTest : public Test { void setupBenchmarkClient() { client_ = std::make_unique( - *api_, *dispatcher_, mock_store_, statistic_, - false, cluster_manager_, http_tracer_, "benchmark", - request_generator_, true); + *api_, *dispatcher_, mock_store_, statistic_, false, cluster_manager_, http_tracer_, + "benchmark", request_generator_, true); } uint64_t getCounter(absl::string_view name) { @@ -223,15 +222,13 @@ TEST_F(BenchmarkClientHttpTest, EnableLatencyMeasurement) { TEST_F(BenchmarkClientHttpTest, ExportSuccessLatency) { setupBenchmarkClient(); uint64_t latency = 10; - EXPECT_CALL(mock_store_, deliverHistogramToSinks( - Property(&Envoy::Stats::Metric::name, - "benchmark_http_client.latency_2xx"), - latency)) + EXPECT_CALL(mock_store_, deliverHistogramToSinks(Property(&Envoy::Stats::Metric::name, + "benchmark_http_client.latency_2xx"), + latency)) .Times(2); - EXPECT_CALL(mock_store_, deliverHistogramToSinks( - Property(&Envoy::Stats::Metric::name, - "benchmark_http_client.latency_xxx"), - latency)) + EXPECT_CALL(mock_store_, deliverHistogramToSinks(Property(&Envoy::Stats::Metric::name, + "benchmark_http_client.latency_xxx"), + latency)) .Times(0); client_->exportLatency(/*response_code=*/200, /*latency_ns=*/latency); client_->exportLatency(/*response_code=*/200, /*latency_ns=*/latency); @@ -241,30 +238,25 @@ TEST_F(BenchmarkClientHttpTest, ExportSuccessLatency) { TEST_F(BenchmarkClientHttpTest, ExportErrorLatency) { setupBenchmarkClient(); - EXPECT_CALL(mock_store_, deliverHistogramToSinks( - Property(&Envoy::Stats::Metric::name, - "benchmark_http_client.latency_1xx"), - _)) + EXPECT_CALL(mock_store_, + deliverHistogramToSinks( + Property(&Envoy::Stats::Metric::name, "benchmark_http_client.latency_1xx"), _)) .Times(1); - EXPECT_CALL(mock_store_, deliverHistogramToSinks( - Property(&Envoy::Stats::Metric::name, - "benchmark_http_client.latency_3xx"), - _)) + EXPECT_CALL(mock_store_, + deliverHistogramToSinks( + Property(&Envoy::Stats::Metric::name, "benchmark_http_client.latency_3xx"), _)) .Times(1); - EXPECT_CALL(mock_store_, deliverHistogramToSinks( - Property(&Envoy::Stats::Metric::name, - "benchmark_http_client.latency_4xx"), - _)) + EXPECT_CALL(mock_store_, + deliverHistogramToSinks( + Property(&Envoy::Stats::Metric::name, "benchmark_http_client.latency_4xx"), _)) .Times(1); - EXPECT_CALL(mock_store_, deliverHistogramToSinks( - Property(&Envoy::Stats::Metric::name, - "benchmark_http_client.latency_5xx"), - _)) + EXPECT_CALL(mock_store_, + deliverHistogramToSinks( + Property(&Envoy::Stats::Metric::name, "benchmark_http_client.latency_5xx"), _)) .Times(1); - EXPECT_CALL(mock_store_, deliverHistogramToSinks( - Property(&Envoy::Stats::Metric::name, - "benchmark_http_client.latency_xxx"), - _)) + EXPECT_CALL(mock_store_, + deliverHistogramToSinks( + Property(&Envoy::Stats::Metric::name, "benchmark_http_client.latency_xxx"), _)) .Times(1); client_->exportLatency(/*response_code=*/100, /*latency_ns=*/1); client_->exportLatency(/*response_code=*/300, /*latency_ns=*/3); @@ -283,7 +275,6 @@ TEST_F(BenchmarkClientHttpTest, ExportErrorLatency) { EXPECT_DOUBLE_EQ(6, client_->statistics()["benchmark_http_client.latency_xxx"]->mean()); } - TEST_F(BenchmarkClientHttpTest, StatusTrackingInOnComplete) { setupBenchmarkClient(); Envoy::Http::ResponseHeaderMapPtr header = Envoy::Http::ResponseHeaderMapImpl::create(); diff --git a/test/integration/test_integration_basics.py b/test/integration/test_integration_basics.py index 8cc7e2fbd..ed8e49e4c 100644 --- a/test/integration/test_integration_basics.py +++ b/test/integration/test_integration_basics.py @@ -10,9 +10,11 @@ from threading import Thread from test.integration.common import IpVersion -from test.integration.integration_test_fixtures import ( - http_test_server_fixture, https_test_server_fixture, multi_http_test_server_fixture, - multi_https_test_server_fixture, server_config) +from test.integration.integration_test_fixtures import (http_test_server_fixture, + https_test_server_fixture, + multi_http_test_server_fixture, + multi_https_test_server_fixture, + server_config) from test.integration.utility import * # TODO(oschaaf): we mostly verify stats observed from the client-side. Add expectations @@ -74,8 +76,9 @@ def mini_stress_test(fixture, args): else: assertGreaterEqual(int(global_histograms["sequencer.blocking"]["count"]), 1) - assertGreaterEqual( - int(global_histograms["benchmark_http_client.request_to_response"]["count"]), 1) + assertGreaterEqual(int(global_histograms["benchmark_http_client.request_to_response"]["count"]), + 1) + assertGreaterEqual(int(global_histograms["benchmark_http_client.latency_2xx"]["count"]), 1) return counters @@ -674,11 +677,12 @@ def test_http_request_release_timing(http_test_server_fixture, qps_parameterizat global_histograms = http_test_server_fixture.getNighthawkGlobalHistogramsbyIdFromJson( parsed_json) counters = http_test_server_fixture.getNighthawkCounterMapFromJson(parsed_json) - assertEqual( - int(global_histograms["benchmark_http_client.request_to_response"]["count"]), - total_requests) - assertEqual( - int(global_histograms["benchmark_http_client.queue_to_connect"]["count"]), total_requests) + assertEqual(int(global_histograms["benchmark_http_client.request_to_response"]["count"]), + total_requests) + assertEqual(int(global_histograms["benchmark_http_client.queue_to_connect"]["count"]), + total_requests) + assertEqual(int(global_histograms["benchmark_http_client.latency_2xx"]["count"]), + total_requests) assertCounterEqual(counters, "benchmark.http_2xx", (total_requests)) From ccc055f73e955a4490f8a660daf9d318be05c114 Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Tue, 14 Jul 2020 18:12:20 -0400 Subject: [PATCH 28/44] fix format Signed-off-by: Qin Qin --- test/integration/test_integration_basics.py | 25 ++++++++++----------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/test/integration/test_integration_basics.py b/test/integration/test_integration_basics.py index ed8e49e4c..4cd4019fa 100644 --- a/test/integration/test_integration_basics.py +++ b/test/integration/test_integration_basics.py @@ -10,11 +10,9 @@ from threading import Thread from test.integration.common import IpVersion -from test.integration.integration_test_fixtures import (http_test_server_fixture, - https_test_server_fixture, - multi_http_test_server_fixture, - multi_https_test_server_fixture, - server_config) +from test.integration.integration_test_fixtures import ( + http_test_server_fixture, https_test_server_fixture, multi_http_test_server_fixture, + multi_https_test_server_fixture, server_config) from test.integration.utility import * # TODO(oschaaf): we mostly verify stats observed from the client-side. Add expectations @@ -76,8 +74,8 @@ def mini_stress_test(fixture, args): else: assertGreaterEqual(int(global_histograms["sequencer.blocking"]["count"]), 1) - assertGreaterEqual(int(global_histograms["benchmark_http_client.request_to_response"]["count"]), - 1) + assertGreaterEqual( + int(global_histograms["benchmark_http_client.request_to_response"]["count"]), 1) assertGreaterEqual(int(global_histograms["benchmark_http_client.latency_2xx"]["count"]), 1) return counters @@ -677,12 +675,13 @@ def test_http_request_release_timing(http_test_server_fixture, qps_parameterizat global_histograms = http_test_server_fixture.getNighthawkGlobalHistogramsbyIdFromJson( parsed_json) counters = http_test_server_fixture.getNighthawkCounterMapFromJson(parsed_json) - assertEqual(int(global_histograms["benchmark_http_client.request_to_response"]["count"]), - total_requests) - assertEqual(int(global_histograms["benchmark_http_client.queue_to_connect"]["count"]), - total_requests) - assertEqual(int(global_histograms["benchmark_http_client.latency_2xx"]["count"]), - total_requests) + assertEqual( + int(global_histograms["benchmark_http_client.request_to_response"]["count"]), + total_requests) + assertEqual( + int(global_histograms["benchmark_http_client.queue_to_connect"]["count"]), total_requests) + assertEqual( + int(global_histograms["benchmark_http_client.latency_2xx"]["count"]), total_requests) assertCounterEqual(counters, "benchmark.http_2xx", (total_requests)) From d6e2a475937c9d5de202f33bb7da1b0b3d240a1b Mon Sep 17 00:00:00 2001 From: qqustc Date: Tue, 14 Jul 2020 23:02:23 -0400 Subject: [PATCH 29/44] Create statistics.md Signed-off-by: Qin Qin Signed-off-by: qqustc --- docs/root/statistics.md | 87 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 docs/root/statistics.md diff --git a/docs/root/statistics.md b/docs/root/statistics.md new file mode 100644 index 000000000..9ace8f12d --- /dev/null +++ b/docs/root/statistics.md @@ -0,0 +1,87 @@ +# Nighthawk Statistics + +## Background +Currently Nighthawk only outputs metrics at the end of a test run and there are no metrics streamed during a test run. +The work to stream its metrics is in progress. + + +## Statistics in Nighthawk +All statistics below defined in Nighthawk are per worker. + +For counter metric, Nighthawk use Envoy's Counter directly. +For histogram metric, Nighthawk wraps Envoy's Histogram into its own Statistic concept (see [#391](https://github.com/envoyproxy/nighthawk/pull/391)). + +Name | Type | Description +-----| ----- | ---------------- +upstream_rq_total | Counter | Total number of requests sent from Nighthawk +http_1xx | Counter | Total number of response with code 1xx +http_2xx | Counter | Total number of response with code 2xx +http_3xx | Counter | Total number of response with code 3xx +http_4xx | Counter | Total number of response with code 4xx +http_5xx | Counter | Total number of response with code 5xx +http_xxx | Counter | Total number of response with code <100 or >=600 +stream_resets | Counter | Total number of stream reset +pool_overflow | Counter | Total number of times connection pool overflowed +pool_connection_failure | Counter | Total number of times pool connection failed +benchmark_http_client.latency_1xx | HdrStatistic | Latency (in Nanosecond) histogram of request with code 1xx +benchmark_http_client.latency_2xx | HdrStatistic | Latency (in Nanosecond) histogram of request with code 2xx +benchmark_http_client.latency_3xx | HdrStatistic | Latency (in Nanosecond) histogram of request with code 3xx +benchmark_http_client.latency_4xx | HdrStatistic | Latency (in Nanosecond) histogram of request with code 4xx +benchmark_http_client.latency_5xx | HdrStatistic | Latency (in Nanosecond) histogram of request with code 5xx +benchmark_http_client.latency_xxx | HdrStatistic | Latency (in Nanosecond) histogram of request with code <100 or >=600 +benchmark_http_client.queue_to_connect | HdrStatistic | Histogram of request connection time (in Nanosecond) +benchmark_http_client.request_to_response | HdrStatistic | Latency (in Nanosecond) histogram of all requests (include requests with stream reset or pool failure) +benchmark_http_client.response_header_size | StreamingStatistic | Statistic of response header size (min, max, mean, pstdev values in bytes) +benchmark_http_client.response_body_size | StreamingStatistic | Statistic of response body size (min, max, mean, pstdev values in bytes) +sequencer.callback | HdrStatistic | Latency (in Nanosecond) histogram of unblocked requests +sequencer.blocking | HdrStatistic | Latency (in Nanosecond) histogram of blocked requests + + +## Envoy Metrics Model + +[Envoy](https://github.com/envoyproxy/envoy) has 3 types of metrics +- Counters: Unsigned integers (can only increase) represent how many times an event happens, e.g. total number of requests. +- Gauges: Unsigned integers (can increase or decrease), e.g. number of active connections. +- Histograms: Unsigned integers that will yield summarized percentile values. E.g. latency distributions. + +In Envoy, the stat [Store](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/include/envoy/stats/store.h#L29) is a singleton +and provides a simple interface by which the rest of the code can obtain handles to scopes, counters, gauges, and histograms. +Envoy counters and gauges are periodically (configured at ~5 sec interval) flushed to the sinks. Note that currently histogram values are sent directly to the sinks. +A stat [Sink](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/include/envoy/stats/sink.h#L48) is an interface that takes generic stat data and translates it into a backend-specific wire format. +Currently Envoy supports the TCP and UDP [statsd](https://github.com/b/statsd_spec) protocol (implemented in [statsd.h](https://github.com/envoyproxy/envoy/blob/master/source/extensions/stat_sinks/common/statsd/statsd.h)). +Users can create their own Sink subclass to translate Envoy metrics into backend-specific format. + +Envoy metrics can be defined using a macro, e.g. +``` +#define ALL_CLUSTER_STATS(COUNTER, GAUGE, HISTOGRAM) + COUNTER(upstream_cx_total) + GAUGE(upstream_cx_active, NeverImport) + HISTOGRAM(upstream_cx_length, Milliseconds) +struct ClusterStats { + ALL_CLUSTER_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT) +}; +``` + +## Envoy Metrics Limitation +Currently Envoy metrics don't support key-value map. As a result, for metrics to be broken down by certain dimensions, we need to define a separate metric for each dimension. +For example, currently Nighthawk defines separate [counters](https://github.com/envoyproxy/nighthawk/blob/master/source/client/benchmark_client_impl.h#L35-L40) to monitor the number of responses with corresponding response code. + +## Envoy Metrics Flush +Envoy uses a flush timer to periodically flush metrics into stat sinks ([here](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/source/server/server.cc#L479-L480)) at a configured interval (default to 5 sec). +For every metric flush, Envoy will call the function [flushMetricsToSinks()](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/source/server/server.cc#L175) to create a metric snapshot from Envoy stat store and flush the snapshot to all sinks through `sink->flush(snapshot)`. + + +## Metrics Export in Nighthawk +Currently a single Nighthawk can run with multiple workers. In the future, Nighthawk will be extended to be able to run multiple instances together. +Since each Nighthawk worker sends requests independently, we decided to export per worker level metrics since it provides several advantages over global level metrics (aggregated across all workers). +- Per worker level metrics provide information about the performance of each worker which will be hidden by the global level metrics. +- Keep the workers independent which makes it easier/efficient to scale up to multiple Nighthawks with large numbers of workers. + +Envoy metrics can be defined at per worker level using [Scope](https://github.com/envoyproxy/envoy/blob/e9c2c8c4a0141c9634316e8283f98f412d0dd207/include/envoy/stats/scope.h#L35) ( e.g. `cluster..total_request_sent`). +The dynamic portions of metric (e.g. `worker_id`) can be embedded into the metric name. A [TagSpecifier](https://github.com/envoyproxy/envoy/blob/7a652daf35d7d4a6a6bad5a010fe65947ee4411a/api/envoy/config/metrics/v3/stats.proto#L182) can be specified in the bootstrap configuration, which will transform dynamic portions into tags. +When per worker level metrics are exported from Nighthawk, multiple per worker level metrics can be converted into a single metric with a `worker_id` label in the stat Sink if the corresponding backend metric supports key-value map. + +## Reference +- [Nighthawk: architecture and key concepts](https://github.com/envoyproxy/nighthawk/blob/master/docs/root/overview.md) +- [Envoy Stats System](https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md) +- [Envoy Stats blog](https://blog.envoyproxy.io/envoy-stats-b65c7f363342) From 2e3762b62d2a498a9fec088192cdf4ae3ededaf5 Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Wed, 15 Jul 2020 09:13:26 -0400 Subject: [PATCH 30/44] update doc Signed-off-by: Qin Qin --- docs/root/statistics.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/root/statistics.md b/docs/root/statistics.md index 9ace8f12d..3012802bb 100644 --- a/docs/root/statistics.md +++ b/docs/root/statistics.md @@ -1,6 +1,6 @@ -# Nighthawk Statistics +# Nighthawk Statistics -## Background +## Background Currently Nighthawk only outputs metrics at the end of a test run and there are no metrics streamed during a test run. The work to stream its metrics is in progress. From a71dad8bb9755339c5159255478e6e93164162bf Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Wed, 15 Jul 2020 17:46:20 -0400 Subject: [PATCH 31/44] update stat Signed-off-by: Qin Qin --- docs/root/statistics.md | 92 +++++++++++++------ include/nighthawk/client/factories.h | 5 +- source/client/benchmark_client_impl.cc | 12 +-- source/client/benchmark_client_impl.h | 24 ++--- source/client/client_worker_impl.cc | 8 +- source/client/factories_impl.cc | 20 ++-- source/client/factories_impl.h | 2 +- source/client/stream_decoder.cc | 2 + source/client/stream_decoder.h | 3 +- source/common/statistic_impl.cc | 4 +- source/common/statistic_impl.h | 6 ++ test/BUILD | 2 +- test/benchmark_http_client_test.cc | 63 +++---------- .../client/mock_benchmark_client_factory.h | 4 +- test/statistic_test.cc | 5 +- test/stream_decoder_test.cc | 2 +- 16 files changed, 131 insertions(+), 123 deletions(-) diff --git a/docs/root/statistics.md b/docs/root/statistics.md index 3012802bb..6887f235b 100644 --- a/docs/root/statistics.md +++ b/docs/root/statistics.md @@ -1,15 +1,17 @@ # Nighthawk Statistics ## Background -Currently Nighthawk only outputs metrics at the end of a test run and there are no metrics streamed during a test run. -The work to stream its metrics is in progress. +Currently Nighthawk only outputs metrics at the end of a test run and there are +no metrics streamed during a test run. The work to stream its metrics is in +progress. ## Statistics in Nighthawk All statistics below defined in Nighthawk are per worker. -For counter metric, Nighthawk use Envoy's Counter directly. -For histogram metric, Nighthawk wraps Envoy's Histogram into its own Statistic concept (see [#391](https://github.com/envoyproxy/nighthawk/pull/391)). +For counter metric, Nighthawk use Envoy's Counter directly. For histogram +metric, Nighthawk wraps Envoy's Histogram into its own Statistic concept (see +[#391](https://github.com/envoyproxy/nighthawk/pull/391)). Name | Type | Description -----| ----- | ---------------- @@ -40,19 +42,29 @@ sequencer.blocking | HdrStatistic | Latency (in Nanosecond) histogram of blocked ## Envoy Metrics Model [Envoy](https://github.com/envoyproxy/envoy) has 3 types of metrics -- Counters: Unsigned integers (can only increase) represent how many times an event happens, e.g. total number of requests. -- Gauges: Unsigned integers (can increase or decrease), e.g. number of active connections. -- Histograms: Unsigned integers that will yield summarized percentile values. E.g. latency distributions. - -In Envoy, the stat [Store](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/include/envoy/stats/store.h#L29) is a singleton -and provides a simple interface by which the rest of the code can obtain handles to scopes, counters, gauges, and histograms. -Envoy counters and gauges are periodically (configured at ~5 sec interval) flushed to the sinks. Note that currently histogram values are sent directly to the sinks. -A stat [Sink](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/include/envoy/stats/sink.h#L48) is an interface that takes generic stat data and translates it into a backend-specific wire format. -Currently Envoy supports the TCP and UDP [statsd](https://github.com/b/statsd_spec) protocol (implemented in [statsd.h](https://github.com/envoyproxy/envoy/blob/master/source/extensions/stat_sinks/common/statsd/statsd.h)). -Users can create their own Sink subclass to translate Envoy metrics into backend-specific format. +- Counters: Unsigned integers (can only increase) represent how many times an + event happens, e.g. total number of requests. +- Gauges: Unsigned integers (can increase or decrease), e.g. number of active + connections. +- Histograms: Unsigned integers that will yield summarized percentile values. + E.g. latency distributions. + +In Envoy, the stat +[Store](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/include/envoy/stats/store.h#L29) +is a singleton and provides a simple interface by which the rest of the code can +obtain handles to scopes, counters, gauges, and histograms. Envoy counters and +gauges are periodically (configured at ~5 sec interval) flushed to the sinks. +Note that currently histogram values are sent directly to the sinks. A stat +[Sink](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/include/envoy/stats/sink.h#L48) +is an interface that takes generic stat data and translates it into a +backend-specific wire format. Currently Envoy supports the TCP and UDP +[statsd](https://github.com/b/statsd_spec) protocol (implemented in +[statsd.h](https://github.com/envoyproxy/envoy/blob/master/source/extensions/stat_sinks/common/statsd/statsd.h)). +Users can create their own Sink subclass to translate Envoy metrics into +backend-specific format. Envoy metrics can be defined using a macro, e.g. -``` +```cc #define ALL_CLUSTER_STATS(COUNTER, GAUGE, HISTOGRAM) COUNTER(upstream_cx_total) GAUGE(upstream_cx_active, NeverImport) @@ -63,25 +75,47 @@ struct ClusterStats { ``` ## Envoy Metrics Limitation -Currently Envoy metrics don't support key-value map. As a result, for metrics to be broken down by certain dimensions, we need to define a separate metric for each dimension. -For example, currently Nighthawk defines separate [counters](https://github.com/envoyproxy/nighthawk/blob/master/source/client/benchmark_client_impl.h#L35-L40) to monitor the number of responses with corresponding response code. +Currently Envoy metrics don't support key-value map. As a result, for metrics to +be broken down by certain dimensions, we need to define a separate metric for +each dimension. For example, currently Nighthawk defines separate +[counters](https://github.com/envoyproxy/nighthawk/blob/master/source/client/benchmark_client_impl.h#L35-L40) +to monitor the number of responses with corresponding response code. ## Envoy Metrics Flush -Envoy uses a flush timer to periodically flush metrics into stat sinks ([here](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/source/server/server.cc#L479-L480)) at a configured interval (default to 5 sec). -For every metric flush, Envoy will call the function [flushMetricsToSinks()](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/source/server/server.cc#L175) to create a metric snapshot from Envoy stat store and flush the snapshot to all sinks through `sink->flush(snapshot)`. +Envoy uses a flush timer to periodically flush metrics into stat sinks +([here](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/source/server/server.cc#L479-L480)) +at a configured interval (default to 5 sec). For every metric flush, Envoy will +call the function +[flushMetricsToSinks()](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/source/server/server.cc#L175) +to create a metric snapshot from Envoy stat store and flush the snapshot to all +sinks through `sink->flush(snapshot)`. ## Metrics Export in Nighthawk -Currently a single Nighthawk can run with multiple workers. In the future, Nighthawk will be extended to be able to run multiple instances together. -Since each Nighthawk worker sends requests independently, we decided to export per worker level metrics since it provides several advantages over global level metrics (aggregated across all workers). -- Per worker level metrics provide information about the performance of each worker which will be hidden by the global level metrics. -- Keep the workers independent which makes it easier/efficient to scale up to multiple Nighthawks with large numbers of workers. - -Envoy metrics can be defined at per worker level using [Scope](https://github.com/envoyproxy/envoy/blob/e9c2c8c4a0141c9634316e8283f98f412d0dd207/include/envoy/stats/scope.h#L35) ( e.g. `cluster..total_request_sent`). -The dynamic portions of metric (e.g. `worker_id`) can be embedded into the metric name. A [TagSpecifier](https://github.com/envoyproxy/envoy/blob/7a652daf35d7d4a6a6bad5a010fe65947ee4411a/api/envoy/config/metrics/v3/stats.proto#L182) can be specified in the bootstrap configuration, which will transform dynamic portions into tags. -When per worker level metrics are exported from Nighthawk, multiple per worker level metrics can be converted into a single metric with a `worker_id` label in the stat Sink if the corresponding backend metric supports key-value map. +Currently a single Nighthawk can run with multiple workers. In the future, +Nighthawk will be extended to be able to run multiple instances together. Since +each Nighthawk worker sends requests independently, we decided to export per +worker level metrics since it provides several advantages over global level +metrics (aggregated across all workers). +- Per worker level metrics provide information about the performance of each + worker which will be hidden by the global level metrics. +- Keep the workers independent which makes it easier/efficient to scale up to + multiple Nighthawks with large numbers of workers. + +Envoy metrics can be defined at per worker level using +[Scope](https://github.com/envoyproxy/envoy/blob/e9c2c8c4a0141c9634316e8283f98f412d0dd207/include/envoy/stats/scope.h#L35) +( e.g. `cluster..total_request_sent`). The dynamic portions of +metric (e.g. `worker_id`) can be embedded into the metric name. A +[TagSpecifier](https://github.com/envoyproxy/envoy/blob/7a652daf35d7d4a6a6bad5a010fe65947ee4411a/api/envoy/config/metrics/v3/stats.proto#L182) +can be specified in the bootstrap configuration, which will transform dynamic +portions into tags. When per worker level metrics are exported from Nighthawk, +multiple per worker level metrics can be converted into a single metric with a +`worker_id` label in the stat Sink if the corresponding backend metric supports +key-value map. ## Reference -- [Nighthawk: architecture and key concepts](https://github.com/envoyproxy/nighthawk/blob/master/docs/root/overview.md) -- [Envoy Stats System](https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md) +- [Nighthawk: architecture and key + concepts](https://github.com/envoyproxy/nighthawk/blob/master/docs/root/overview.md) +- [Envoy Stats + System](https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md) - [Envoy Stats blog](https://blog.envoyproxy.io/envoy-stats-b65c7f363342) diff --git a/include/nighthawk/client/factories.h b/include/nighthawk/client/factories.h index d7c7796b9..44b90b487 100644 --- a/include/nighthawk/client/factories.h +++ b/include/nighthawk/client/factories.h @@ -33,9 +33,10 @@ class BenchmarkClientFactory { * @param scope stats scope for any stats tracked by the benchmark client. * @param cluster_manager Cluster manager preconfigured with our target cluster. * @param http_tracer Shared pointer to an http tracer implementation (e.g. Zipkin). - * @param worker_number Worker id which is also the name of the cluster that this benchmark client + * @param cluster_name Name of the cluster that this benchmark client * will use. In conjunction with cluster_manager this will allow the this BenchmarkClient to * access the target connection pool. + * @param sink_stat_prefix Prefix of the stat (e.g. worker id). * @param request_source Source of request-specifiers. Will be queries every time the * BenchmarkClient is asked to issue a request. * @@ -45,7 +46,7 @@ class BenchmarkClientFactory { Envoy::Stats::Scope& scope, Envoy::Upstream::ClusterManagerPtr& cluster_manager, Envoy::Tracing::HttpTracerSharedPtr& http_tracer, - const int worker_number, + absl::string_view cluster_name, const int sink_stat_prefix, RequestSource& request_source) const PURE; }; diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index 7499647cb..753ca7328 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -189,17 +189,17 @@ void BenchmarkClientHttpImpl::onPoolFailure(Envoy::Http::ConnectionPool::PoolFai void BenchmarkClientHttpImpl::exportLatency(const uint32_t response_code, const uint64_t latency_ns) { if (response_code > 99 && response_code <= 199) { - latency_1xx_statistic_->recordValue(latency_ns); + latency_1xx_statistic_->addValue(latency_ns); } else if (response_code > 199 && response_code <= 299) { - latency_2xx_statistic_->recordValue(latency_ns); + latency_2xx_statistic_->addValue(latency_ns); } else if (response_code > 299 && response_code <= 399) { - latency_3xx_statistic_->recordValue(latency_ns); + latency_3xx_statistic_->addValue(latency_ns); } else if (response_code > 399 && response_code <= 499) { - latency_4xx_statistic_->recordValue(latency_ns); + latency_4xx_statistic_->addValue(latency_ns); } else if (response_code > 499 && response_code <= 599) { - latency_5xx_statistic_->recordValue(latency_ns); + latency_5xx_statistic_->addValue(latency_ns); } else { - latency_xxx_statistic_->recordValue(latency_ns); + latency_xxx_statistic_->addValue(latency_ns); } } diff --git a/source/client/benchmark_client_impl.h b/source/client/benchmark_client_impl.h index a13f4dd2a..a03a338d3 100644 --- a/source/client/benchmark_client_impl.h +++ b/source/client/benchmark_client_impl.h @@ -54,12 +54,12 @@ struct BenchmarkClientStatistic { StatisticPtr response_statistic; StatisticPtr response_header_size_statistic; StatisticPtr response_body_size_statistic; - std::unique_ptr latency_1xx_statistic; - std::unique_ptr latency_2xx_statistic; - std::unique_ptr latency_3xx_statistic; - std::unique_ptr latency_4xx_statistic; - std::unique_ptr latency_5xx_statistic; - std::unique_ptr latency_xxx_statistic; + StatisticPtr latency_1xx_statistic; + StatisticPtr latency_2xx_statistic; + StatisticPtr latency_3xx_statistic; + StatisticPtr latency_4xx_statistic; + StatisticPtr latency_5xx_statistic; + StatisticPtr latency_xxx_statistic; }; class Http1PoolImpl : public Envoy::Http::Http1::ProdConnPoolImpl { @@ -137,12 +137,12 @@ class BenchmarkClientHttpImpl : public BenchmarkClient, StatisticPtr response_statistic_; StatisticPtr response_header_size_statistic_; StatisticPtr response_body_size_statistic_; - std::unique_ptr latency_1xx_statistic_; - std::unique_ptr latency_2xx_statistic_; - std::unique_ptr latency_3xx_statistic_; - std::unique_ptr latency_4xx_statistic_; - std::unique_ptr latency_5xx_statistic_; - std::unique_ptr latency_xxx_statistic_; + StatisticPtr latency_1xx_statistic_; + StatisticPtr latency_2xx_statistic_; + StatisticPtr latency_3xx_statistic_; + StatisticPtr latency_4xx_statistic_; + StatisticPtr latency_5xx_statistic_; + StatisticPtr latency_xxx_statistic_; const bool use_h2_; std::chrono::seconds timeout_{5s}; uint32_t connection_limit_{1}; diff --git a/source/client/client_worker_impl.cc b/source/client/client_worker_impl.cc index d9ebb158a..a5d456763 100644 --- a/source/client/client_worker_impl.cc +++ b/source/client/client_worker_impl.cc @@ -31,9 +31,9 @@ ClientWorkerImpl::ClientWorkerImpl(Envoy::Api::Api& api, Envoy::ThreadLocal::Ins request_generator_( request_generator_factory.create(cluster_manager, *dispatcher_, *worker_number_scope_, fmt::format("{}.requestsource", worker_number))), - benchmark_client_(benchmark_client_factory.create(api, *dispatcher_, *worker_number_scope_, - cluster_manager, http_tracer_, - worker_number, *request_generator_)), + benchmark_client_(benchmark_client_factory.create( + api, *dispatcher_, *worker_number_scope_, cluster_manager, http_tracer_, + fmt::format("{}", worker_number), worker_number, *request_generator_)), phase_( std::make_unique("main", sequencer_factory_.create( @@ -106,4 +106,4 @@ StatisticPtrMap ClientWorkerImpl::statistics() const { } } // namespace Client -} // namespace Nighthawk \ No newline at end of file +} // namespace Nighthawk diff --git a/source/client/factories_impl.cc b/source/client/factories_impl.cc index 2a5d193eb..6f488edf5 100644 --- a/source/client/factories_impl.cc +++ b/source/client/factories_impl.cc @@ -30,8 +30,8 @@ BenchmarkClientFactoryImpl::BenchmarkClientFactoryImpl(const Options& options) BenchmarkClientPtr BenchmarkClientFactoryImpl::create( Envoy::Api::Api& api, Envoy::Event::Dispatcher& dispatcher, Envoy::Stats::Scope& scope, Envoy::Upstream::ClusterManagerPtr& cluster_manager, - Envoy::Tracing::HttpTracerSharedPtr& http_tracer, const int worker_number, - RequestSource& request_generator) const { + Envoy::Tracing::HttpTracerSharedPtr& http_tracer, absl::string_view cluster_name, + const int sink_stat_prefix, RequestSource& request_generator) const { StatisticFactoryImpl statistic_factory(options_); // While we lack options to configure which statistic backend goes where, we directly pass // StreamingStatistic for the stats that track response sizes. Ideally we would have options @@ -44,15 +44,15 @@ BenchmarkClientPtr BenchmarkClientFactoryImpl::create( statistic.response_statistic = statistic_factory.create(); statistic.response_header_size_statistic = std::make_unique(); statistic.response_body_size_statistic = std::make_unique(); - statistic.latency_1xx_statistic = std::make_unique(scope, worker_number); - statistic.latency_2xx_statistic = std::make_unique(scope, worker_number); - statistic.latency_3xx_statistic = std::make_unique(scope, worker_number); - statistic.latency_4xx_statistic = std::make_unique(scope, worker_number); - statistic.latency_5xx_statistic = std::make_unique(scope, worker_number); - statistic.latency_xxx_statistic = std::make_unique(scope, worker_number); + statistic.latency_1xx_statistic = std::make_unique(scope, sink_stat_prefix); + statistic.latency_2xx_statistic = std::make_unique(scope, sink_stat_prefix); + statistic.latency_3xx_statistic = std::make_unique(scope, sink_stat_prefix); + statistic.latency_4xx_statistic = std::make_unique(scope, sink_stat_prefix); + statistic.latency_5xx_statistic = std::make_unique(scope, sink_stat_prefix); + statistic.latency_xxx_statistic = std::make_unique(scope, sink_stat_prefix); auto benchmark_client = std::make_unique( - api, dispatcher, scope, statistic, options_.h2(), cluster_manager, http_tracer, - fmt::format("{}", worker_number), request_generator.get(), !options_.openLoop()); + api, dispatcher, scope, statistic, options_.h2(), cluster_manager, http_tracer, cluster_name, + request_generator.get(), !options_.openLoop()); auto request_options = options_.toCommandLineOptions()->request_options(); benchmark_client->setConnectionLimit(options_.connections()); benchmark_client->setMaxPendingRequests(options_.maxPendingRequests()); diff --git a/source/client/factories_impl.h b/source/client/factories_impl.h index fc45bf740..2be43ee7a 100644 --- a/source/client/factories_impl.h +++ b/source/client/factories_impl.h @@ -31,7 +31,7 @@ class BenchmarkClientFactoryImpl : public OptionBasedFactoryImpl, public Benchma Envoy::Stats::Scope& scope, Envoy::Upstream::ClusterManagerPtr& cluster_manager, Envoy::Tracing::HttpTracerSharedPtr& http_tracer, - const int worker_number, + absl::string_view cluster_name, const int sink_stat_prefix, RequestSource& request_generator) const override; }; diff --git a/source/client/stream_decoder.cc b/source/client/stream_decoder.cc index f442c1b31..a22ec30c2 100644 --- a/source/client/stream_decoder.cc +++ b/source/client/stream_decoder.cc @@ -54,6 +54,8 @@ void StreamDecoder::onComplete(bool success) { decoder_completion_callback_.exportLatency( stream_info_.response_code_.value(), (time_source_.monotonicTime() - request_start_).count()); + } else { + ENVOY_LOG(warn, "response_code is not available in onComplete"); } } upstream_timing_.onLastUpstreamRxByteReceived(time_source_); diff --git a/source/client/stream_decoder.h b/source/client/stream_decoder.h index b8f9d917c..0f6d4aeae 100644 --- a/source/client/stream_decoder.h +++ b/source/client/stream_decoder.h @@ -36,7 +36,8 @@ class StreamDecoderCompletionCallback { class StreamDecoder : public Envoy::Http::ResponseDecoder, public Envoy::Http::StreamCallbacks, public Envoy::Http::ConnectionPool::Callbacks, - public Envoy::Event::DeferredDeletable { + public Envoy::Event::DeferredDeletable, + public Envoy::Logger::Loggable { public: StreamDecoder(Envoy::Event::Dispatcher& dispatcher, Envoy::TimeSource& time_source, StreamDecoderCompletionCallback& decoder_completion_callback, diff --git a/source/common/statistic_impl.cc b/source/common/statistic_impl.cc index b4a697a66..672c0020f 100644 --- a/source/common/statistic_impl.cc +++ b/source/common/statistic_impl.cc @@ -317,7 +317,7 @@ SinkableHdrStatistic::SinkableHdrStatistic(Envoy::Stats::Scope& scope, : SinkableStatistic(scope, worker_id) {} void SinkableHdrStatistic::recordValue(uint64_t value) { - addValue(value); + HdrStatistic::addValue(value); // Currently in Envoy Scope implementation, deliverHistogramToSinks() will flush the histogram // value directly to stats Sinks. scope_.deliverHistogramToSinks(*this, value); @@ -328,7 +328,7 @@ SinkableCircllhistStatistic::SinkableCircllhistStatistic(Envoy::Stats::Scope& sc : SinkableStatistic(scope, worker_id) {} void SinkableCircllhistStatistic::recordValue(uint64_t value) { - addValue(value); + CircllhistStatistic::addValue(value); // Currently in Envoy Scope implementation, deliverHistogramToSinks() will flush the histogram // value directly to stats Sinks. scope_.deliverHistogramToSinks(*this, value); diff --git a/source/common/statistic_impl.h b/source/common/statistic_impl.h index d7e1f240f..f3d83b4de 100644 --- a/source/common/statistic_impl.h +++ b/source/common/statistic_impl.h @@ -223,6 +223,9 @@ class SinkableHdrStatistic : public SinkableStatistic, public HdrStatistic { // Overriding name() to return Nighthawk::Statistic::id(). std::string name() const override { return id(); } std::string tagExtractedName() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } + + // Nighthawk::Statistic + void addValue(uint64_t value) override { recordValue(value); } }; // Implementation of sinkable Nighthawk Statistic with Circllhist Histogram. @@ -239,6 +242,9 @@ class SinkableCircllhistStatistic : public SinkableStatistic, public CircllhistS // Overriding name() to return Nighthawk::Statistic::id(). std::string name() const override { return id(); } std::string tagExtractedName() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } + + // Nighthawk::Statistic + void addValue(uint64_t value) override { recordValue(value); } }; } // namespace Nighthawk diff --git a/test/BUILD b/test/BUILD index 81946a4f0..ac7cdcacc 100644 --- a/test/BUILD +++ b/test/BUILD @@ -18,9 +18,9 @@ envoy_cc_test( "//test/test_common:environment_lib", "@envoy//source/common/http:header_map_lib_with_external_headers", "@envoy//source/common/network:utility_lib_with_external_headers", + "@envoy//source/common/stats:isolated_store_lib_with_external_headers", "@envoy//source/exe:process_wide_lib_with_external_headers", "@envoy//test/mocks/runtime:runtime_mocks", - "@envoy//test/mocks/stats:stats_mocks", "@envoy//test/mocks/stream_info:stream_info_mocks", "@envoy//test/mocks/thread_local:thread_local_mocks", "@envoy//test/mocks/upstream:upstream_mocks", diff --git a/test/benchmark_http_client_test.cc b/test/benchmark_http_client_test.cc index 4bf639069..eb4af2015 100644 --- a/test/benchmark_http_client_test.cc +++ b/test/benchmark_http_client_test.cc @@ -4,10 +4,10 @@ #include "external/envoy/source/common/http/header_map_impl.h" #include "external/envoy/source/common/network/utility.h" #include "external/envoy/source/common/runtime/runtime_impl.h" +#include "external/envoy/source/common/stats/isolated_store_impl.h" #include "external/envoy/source/exe/process_wide.h" #include "external/envoy/test/mocks/common.h" #include "external/envoy/test/mocks/runtime/mocks.h" -#include "external/envoy/test/mocks/stats/mocks.h" #include "external/envoy/test/mocks/stream_info/mocks.h" #include "external/envoy/test/mocks/thread_local/mocks.h" #include "external/envoy/test/mocks/upstream/mocks.h" @@ -59,18 +59,12 @@ class BenchmarkClientHttpTest : public Test { statistic_.response_statistic = std::make_unique(); statistic_.response_header_size_statistic = std::make_unique(); statistic_.response_body_size_statistic = std::make_unique(); - statistic_.latency_1xx_statistic = - std::make_unique(mock_store_, worker_number_); - statistic_.latency_2xx_statistic = - std::make_unique(mock_store_, worker_number_); - statistic_.latency_3xx_statistic = - std::make_unique(mock_store_, worker_number_); - statistic_.latency_4xx_statistic = - std::make_unique(mock_store_, worker_number_); - statistic_.latency_5xx_statistic = - std::make_unique(mock_store_, worker_number_); - statistic_.latency_xxx_statistic = - std::make_unique(mock_store_, worker_number_); + statistic_.latency_1xx_statistic = std::make_unique(); + statistic_.latency_2xx_statistic = std::make_unique(); + statistic_.latency_3xx_statistic = std::make_unique(); + statistic_.latency_4xx_statistic = std::make_unique(); + statistic_.latency_5xx_statistic = std::make_unique(); + statistic_.latency_xxx_statistic = std::make_unique(); } void testBasicFunctionality(const uint64_t max_pending, const uint64_t connection_limit, @@ -141,8 +135,8 @@ class BenchmarkClientHttpTest : public Test { void setupBenchmarkClient() { client_ = std::make_unique( - *api_, *dispatcher_, mock_store_, statistic_, false, cluster_manager_, http_tracer_, - "benchmark", request_generator_, true); + *api_, *dispatcher_, store_, statistic_, false, cluster_manager_, http_tracer_, "benchmark", + request_generator_, true); } uint64_t getCounter(absl::string_view name) { @@ -158,8 +152,7 @@ class BenchmarkClientHttpTest : public Test { } Envoy::Event::TestRealTimeSystem time_system_; - // deliverHistogramToSinks() is not implemented in IsolatedStoreImpl so test with a mock store. - Envoy::Stats::MockIsolatedStatsStore mock_store_; + Envoy::Stats::IsolatedStoreImpl store_; Envoy::Api::ApiPtr api_; Envoy::Event::DispatcherPtr dispatcher_; Envoy::Random::RandomGeneratorImpl generator_; @@ -207,13 +200,11 @@ TEST_F(BenchmarkClientHttpTest, WeirdStatus) { TEST_F(BenchmarkClientHttpTest, EnableLatencyMeasurement) { setupBenchmarkClient(); EXPECT_EQ(false, client_->shouldMeasureLatencies()); - EXPECT_CALL(mock_store_, deliverHistogramToSinks(_, _)).Times(0); testBasicFunctionality(10, 1, 10); EXPECT_EQ(0, client_->statistics()["benchmark_http_client.queue_to_connect"]->count()); EXPECT_EQ(0, client_->statistics()["benchmark_http_client.request_to_response"]->count()); EXPECT_EQ(0, client_->statistics()["benchmark_http_client.latency_2xx"]->count()); client_->setShouldMeasureLatencies(true); - EXPECT_CALL(mock_store_, deliverHistogramToSinks(_, _)).Times(10); testBasicFunctionality(10, 1, 10); EXPECT_EQ(10, client_->statistics()["benchmark_http_client.queue_to_connect"]->count()); EXPECT_EQ(10, client_->statistics()["benchmark_http_client.request_to_response"]->count()); @@ -222,43 +213,15 @@ TEST_F(BenchmarkClientHttpTest, EnableLatencyMeasurement) { TEST_F(BenchmarkClientHttpTest, ExportSuccessLatency) { setupBenchmarkClient(); - uint64_t latency = 10; - EXPECT_CALL(mock_store_, deliverHistogramToSinks(Property(&Envoy::Stats::Metric::name, - "benchmark_http_client.latency_2xx"), - latency)) - .Times(2); - EXPECT_CALL(mock_store_, deliverHistogramToSinks(Property(&Envoy::Stats::Metric::name, - "benchmark_http_client.latency_xxx"), - latency)) - .Times(0); - client_->exportLatency(/*response_code=*/200, /*latency_ns=*/latency); - client_->exportLatency(/*response_code=*/200, /*latency_ns=*/latency); + uint64_t latency_ns = 10; + client_->exportLatency(/*response_code=*/200, latency_ns); + client_->exportLatency(/*response_code=*/200, latency_ns); EXPECT_EQ(2, client_->statistics()["benchmark_http_client.latency_2xx"]->count()); EXPECT_DOUBLE_EQ(latency, client_->statistics()["benchmark_http_client.latency_2xx"]->mean()); } TEST_F(BenchmarkClientHttpTest, ExportErrorLatency) { setupBenchmarkClient(); - EXPECT_CALL(mock_store_, - deliverHistogramToSinks( - Property(&Envoy::Stats::Metric::name, "benchmark_http_client.latency_1xx"), _)) - .Times(1); - EXPECT_CALL(mock_store_, - deliverHistogramToSinks( - Property(&Envoy::Stats::Metric::name, "benchmark_http_client.latency_3xx"), _)) - .Times(1); - EXPECT_CALL(mock_store_, - deliverHistogramToSinks( - Property(&Envoy::Stats::Metric::name, "benchmark_http_client.latency_4xx"), _)) - .Times(1); - EXPECT_CALL(mock_store_, - deliverHistogramToSinks( - Property(&Envoy::Stats::Metric::name, "benchmark_http_client.latency_5xx"), _)) - .Times(1); - EXPECT_CALL(mock_store_, - deliverHistogramToSinks( - Property(&Envoy::Stats::Metric::name, "benchmark_http_client.latency_xxx"), _)) - .Times(1); client_->exportLatency(/*response_code=*/100, /*latency_ns=*/1); client_->exportLatency(/*response_code=*/300, /*latency_ns=*/3); client_->exportLatency(/*response_code=*/400, /*latency_ns=*/4); diff --git a/test/mocks/client/mock_benchmark_client_factory.h b/test/mocks/client/mock_benchmark_client_factory.h index 0c8d0ef9f..8e7dd78b2 100644 --- a/test/mocks/client/mock_benchmark_client_factory.h +++ b/test/mocks/client/mock_benchmark_client_factory.h @@ -17,8 +17,8 @@ class MockBenchmarkClientFactory : public BenchmarkClientFactory { MOCK_CONST_METHOD7(create, BenchmarkClientPtr(Envoy::Api::Api&, Envoy::Event::Dispatcher&, Envoy::Stats::Scope&, Envoy::Upstream::ClusterManagerPtr&, - Envoy::Tracing::HttpTracerSharedPtr&, const int, - RequestSource& request_generator)); + Envoy::Tracing::HttpTracerSharedPtr&, absl::string_view, + const int, RequestSource& request_generator)); }; } // namespace Client diff --git a/test/statistic_test.cc b/test/statistic_test.cc index b72d76043..a1fd5d1a2 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -414,11 +414,12 @@ TYPED_TEST(SinkableStatisticTest, SimpleSinkableStatistic) { const uint64_t sample_value = 123; const std::string stat_name = "stat_name"; - EXPECT_CALL(mock_store, deliverHistogramToSinks(_, sample_value)).Times(1); + EXPECT_CALL(mock_store, deliverHistogramToSinks(_, sample_value)).Times(2); stat.recordValue(sample_value); + stat.addValue(sample_value); stat.setId(stat_name); - EXPECT_EQ(1, stat.count()); + EXPECT_EQ(2, stat.count()); Helper::expectNear(123.0, stat.mean(), stat.significantDigits()); EXPECT_DOUBLE_EQ(0, stat.pvariance()); EXPECT_DOUBLE_EQ(0, stat.pstdev()); diff --git a/test/stream_decoder_test.cc b/test/stream_decoder_test.cc index ab8cad2dd..ca6556906 100644 --- a/test/stream_decoder_test.cc +++ b/test/stream_decoder_test.cc @@ -53,7 +53,7 @@ class StreamDecoderTest : public Test, public StreamDecoderCompletionCallback { uint64_t stream_decoder_completion_callbacks_{0}; uint64_t pool_failures_{0}; uint64_t stream_decoder_export_latency_callbacks_{0}; - Envoy::Runtime::RandomGeneratorImpl random_generator_; + Envoy::Random::RandomGeneratorImpl random_generator_; Envoy::Tracing::HttpTracerSharedPtr http_tracer_; Envoy::Http::ResponseHeaderMapPtr test_header_; Envoy::Http::ResponseTrailerMapPtr test_trailer_; From be4533c32956432ba7e50ac71c10a057b678106d Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Wed, 15 Jul 2020 17:58:36 -0400 Subject: [PATCH 32/44] fix mock_benchmark_client_factory.h Signed-off-by: Qin Qin --- test/mocks/client/mock_benchmark_client_factory.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/mocks/client/mock_benchmark_client_factory.h b/test/mocks/client/mock_benchmark_client_factory.h index 8e7dd78b2..097440656 100644 --- a/test/mocks/client/mock_benchmark_client_factory.h +++ b/test/mocks/client/mock_benchmark_client_factory.h @@ -14,7 +14,7 @@ namespace Client { class MockBenchmarkClientFactory : public BenchmarkClientFactory { public: MockBenchmarkClientFactory(); - MOCK_CONST_METHOD7(create, + MOCK_CONST_METHOD8(create, BenchmarkClientPtr(Envoy::Api::Api&, Envoy::Event::Dispatcher&, Envoy::Stats::Scope&, Envoy::Upstream::ClusterManagerPtr&, Envoy::Tracing::HttpTracerSharedPtr&, absl::string_view, From f8d5696600a8a32f196a3627187927e092cd9ea4 Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Thu, 16 Jul 2020 09:50:20 -0400 Subject: [PATCH 33/44] fix tests Signed-off-by: Qin Qin --- test/benchmark_http_client_test.cc | 2 +- test/client_worker_test.cc | 2 +- test/factories_test.cc | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/benchmark_http_client_test.cc b/test/benchmark_http_client_test.cc index eb4af2015..a34dd983a 100644 --- a/test/benchmark_http_client_test.cc +++ b/test/benchmark_http_client_test.cc @@ -217,7 +217,7 @@ TEST_F(BenchmarkClientHttpTest, ExportSuccessLatency) { client_->exportLatency(/*response_code=*/200, latency_ns); client_->exportLatency(/*response_code=*/200, latency_ns); EXPECT_EQ(2, client_->statistics()["benchmark_http_client.latency_2xx"]->count()); - EXPECT_DOUBLE_EQ(latency, client_->statistics()["benchmark_http_client.latency_2xx"]->mean()); + EXPECT_DOUBLE_EQ(latency_ns, client_->statistics()["benchmark_http_client.latency_2xx"]->mean()); } TEST_F(BenchmarkClientHttpTest, ExportErrorLatency) { diff --git a/test/client_worker_test.cc b/test/client_worker_test.cc index c2a7a9dae..8ffdf6680 100644 --- a/test/client_worker_test.cc +++ b/test/client_worker_test.cc @@ -43,7 +43,7 @@ class ClientWorkerTest : public Test { sequencer_ = new MockSequencer(); request_generator_ = new MockRequestSource(); - EXPECT_CALL(benchmark_client_factory_, create(_, _, _, _, _, _, _)) + EXPECT_CALL(benchmark_client_factory_, create(_, _, _, _, _, _, _, _)) .Times(1) .WillOnce(Return(ByMove(std::unique_ptr(benchmark_client_)))); diff --git a/test/factories_test.cc b/test/factories_test.cc index 68428c53b..690c21acf 100644 --- a/test/factories_test.cc +++ b/test/factories_test.cc @@ -46,7 +46,7 @@ TEST_F(FactoriesTest, CreateBenchmarkClient) { StaticRequestSourceImpl request_generator( std::make_unique()); auto benchmark_client = factory.create(*api_, dispatcher_, stats_store_, cluster_manager, - http_tracer_, /*worker_number=*/0, request_generator); + http_tracer_, "foocluster", /*sink_stat_prefix=*/0, request_generator); EXPECT_NE(nullptr, benchmark_client.get()); } From 6b36a64a52c8583a615890f3d64238191703d0ed Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Thu, 16 Jul 2020 10:01:22 -0400 Subject: [PATCH 34/44] fix format Signed-off-by: Qin Qin --- test/factories_test.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/factories_test.cc b/test/factories_test.cc index 690c21acf..b3cdbf0e7 100644 --- a/test/factories_test.cc +++ b/test/factories_test.cc @@ -45,8 +45,9 @@ TEST_F(FactoriesTest, CreateBenchmarkClient) { EXPECT_CALL(options_, toCommandLineOptions()).Times(1).WillOnce(Return(ByMove(std::move(cmd)))); StaticRequestSourceImpl request_generator( std::make_unique()); - auto benchmark_client = factory.create(*api_, dispatcher_, stats_store_, cluster_manager, - http_tracer_, "foocluster", /*sink_stat_prefix=*/0, request_generator); + auto benchmark_client = + factory.create(*api_, dispatcher_, stats_store_, cluster_manager, http_tracer_, "foocluster", + /*sink_stat_prefix=*/0, request_generator); EXPECT_NE(nullptr, benchmark_client.get()); } From 302807dbf35ca3f3faa0ad36ec569f6736c1da57 Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Thu, 16 Jul 2020 10:17:17 -0400 Subject: [PATCH 35/44] fix test Signed-off-by: Qin Qin --- test/integration/test_integration_basics.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/integration/test_integration_basics.py b/test/integration/test_integration_basics.py index 1e737295c..8b7c6b246 100644 --- a/test/integration/test_integration_basics.py +++ b/test/integration/test_integration_basics.py @@ -91,7 +91,8 @@ def _mini_stress_test(fixture, args): asserts.assertGreaterEqual( int(global_histograms["benchmark_http_client.request_to_response"]["count"]), 1) - assertGreaterEqual(int(global_histograms["benchmark_http_client.latency_2xx"]["count"]), 1) + asserts.assertGreaterEqual( + int(global_histograms["benchmark_http_client.latency_2xx"]["count"]), 1) return counters @@ -667,8 +668,8 @@ def test_http_request_release_timing(http_test_server_fixture, qps_parameterizat total_requests) asserts.assertEqual(int(global_histograms["benchmark_http_client.queue_to_connect"]["count"]), total_requests) - assertEqual(int(global_histograms["benchmark_http_client.latency_2xx"]["count"]), - total_requests) + asserts.assertEqual(int(global_histograms["benchmark_http_client.latency_2xx"]["count"]), + total_requests) asserts.assertCounterEqual(counters, "benchmark.http_2xx", (total_requests)) From 49e708ec19bf9c9a608a4f759e8752d56fd97dd4 Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Thu, 16 Jul 2020 10:21:51 -0400 Subject: [PATCH 36/44] fix format Signed-off-by: Qin Qin --- test/integration/test_integration_basics.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/test_integration_basics.py b/test/integration/test_integration_basics.py index 8b7c6b246..e342c27c4 100644 --- a/test/integration/test_integration_basics.py +++ b/test/integration/test_integration_basics.py @@ -91,8 +91,8 @@ def _mini_stress_test(fixture, args): asserts.assertGreaterEqual( int(global_histograms["benchmark_http_client.request_to_response"]["count"]), 1) - asserts.assertGreaterEqual( - int(global_histograms["benchmark_http_client.latency_2xx"]["count"]), 1) + asserts.assertGreaterEqual(int(global_histograms["benchmark_http_client.latency_2xx"]["count"]), + 1) return counters From 60e46cd9a009dc2428e8577a810ea0b2a67a4e49 Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Thu, 16 Jul 2020 12:17:19 -0400 Subject: [PATCH 37/44] rerun CI Signed-off-by: Qin Qin --- docs/root/statistics.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/statistics.md b/docs/root/statistics.md index 6887f235b..48e9a6c65 100644 --- a/docs/root/statistics.md +++ b/docs/root/statistics.md @@ -6,7 +6,7 @@ no metrics streamed during a test run. The work to stream its metrics is in progress. -## Statistics in Nighthawk +## Statistics in Nighthawk All statistics below defined in Nighthawk are per worker. For counter metric, Nighthawk use Envoy's Counter directly. For histogram From c80178250e411996a197d4efd539283ac9e21638 Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Thu, 16 Jul 2020 14:26:28 -0400 Subject: [PATCH 38/44] rerun CI Signed-off-by: Qin Qin --- docs/root/statistics.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/statistics.md b/docs/root/statistics.md index 48e9a6c65..d3b416749 100644 --- a/docs/root/statistics.md +++ b/docs/root/statistics.md @@ -3,7 +3,7 @@ ## Background Currently Nighthawk only outputs metrics at the end of a test run and there are no metrics streamed during a test run. The work to stream its metrics is in -progress. +progress. ## Statistics in Nighthawk From 925519735f7da22d954d009c0144b467de4a4e75 Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Thu, 16 Jul 2020 15:51:00 -0400 Subject: [PATCH 39/44] rerun CI Signed-off-by: Qin Qin --- docs/root/statistics.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/statistics.md b/docs/root/statistics.md index d3b416749..b6e0142b3 100644 --- a/docs/root/statistics.md +++ b/docs/root/statistics.md @@ -13,7 +13,7 @@ For counter metric, Nighthawk use Envoy's Counter directly. For histogram metric, Nighthawk wraps Envoy's Histogram into its own Statistic concept (see [#391](https://github.com/envoyproxy/nighthawk/pull/391)). -Name | Type | Description +Name | Type | Description -----| ----- | ---------------- upstream_rq_total | Counter | Total number of requests sent from Nighthawk http_1xx | Counter | Total number of response with code 1xx From d56991f725ef2cd4ef7e164ca7a6a3dadd2a0bfc Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Mon, 20 Jul 2020 14:30:25 -0400 Subject: [PATCH 40/44] add constructor to stat struct Signed-off-by: Qin Qin --- source/client/benchmark_client_impl.cc | 99 ++++++++++++++++---------- source/client/benchmark_client_impl.h | 28 ++++---- source/client/factories_impl.cc | 22 +++--- source/client/factories_impl.h | 2 +- test/benchmark_http_client_test.cc | 21 +++--- 5 files changed, 96 insertions(+), 76 deletions(-) diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index 753ca7328..cbcb7764b 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -20,6 +20,34 @@ using namespace std::chrono_literals; namespace Nighthawk { namespace Client { +BenchmarkClientStatistic::BenchmarkClientStatistic(BenchmarkClientStatistic& statistic) + : connect_statistic(std::move(statistic.connect_statistic)), + response_statistic(std::move(statistic.response_statistic)), + response_header_size_statistic(std::move(statistic.response_header_size_statistic)), + response_body_size_statistic(std::move(statistic.response_body_size_statistic)), + latency_1xx_statistic(std::move(statistic.latency_1xx_statistic)), + latency_2xx_statistic(std::move(statistic.latency_2xx_statistic)), + latency_3xx_statistic(std::move(statistic.latency_3xx_statistic)), + latency_4xx_statistic(std::move(statistic.latency_4xx_statistic)), + latency_5xx_statistic(std::move(statistic.latency_5xx_statistic)), + latency_xxx_statistic(std::move(statistic.latency_xxx_statistic)) {} + +BenchmarkClientStatistic::BenchmarkClientStatistic( + StatisticPtr&& connect_stat, StatisticPtr&& response_stat, + StatisticPtr&& response_header_size_stat, StatisticPtr&& response_body_size_stat, + StatisticPtr&& latency_1xx_stat, StatisticPtr&& latency_2xx_stat, + StatisticPtr&& latency_3xx_stat, StatisticPtr&& latency_4xx_stat, + StatisticPtr&& latency_5xx_stat, StatisticPtr&& latency_xxx_stat) + : connect_statistic(std::move(connect_stat)), response_statistic(std::move(response_stat)), + response_header_size_statistic(std::move(response_header_size_stat)), + response_body_size_statistic(std::move(response_body_size_stat)), + latency_1xx_statistic(std::move(latency_1xx_stat)), + latency_2xx_statistic(std::move(latency_2xx_stat)), + latency_3xx_statistic(std::move(latency_3xx_stat)), + latency_4xx_statistic(std::move(latency_4xx_stat)), + latency_5xx_statistic(std::move(latency_5xx_stat)), + latency_xxx_statistic(std::move(latency_xxx_stat)) {} + Envoy::Http::ConnectionPool::Cancellable* Http1PoolImpl::newStream(Envoy::Http::ResponseDecoder& response_decoder, Envoy::Http::ConnectionPool::Callbacks& callbacks) { @@ -54,30 +82,21 @@ BenchmarkClientHttpImpl::BenchmarkClientHttpImpl( Envoy::Tracing::HttpTracerSharedPtr& http_tracer, absl::string_view cluster_name, RequestGenerator request_generator, const bool provide_resource_backpressure) : api_(api), dispatcher_(dispatcher), scope_(scope.createScope("benchmark.")), - connect_statistic_(std::move(statistic.connect_statistic)), - response_statistic_(std::move(statistic.response_statistic)), - response_header_size_statistic_(std::move(statistic.response_header_size_statistic)), - response_body_size_statistic_(std::move(statistic.response_body_size_statistic)), - latency_1xx_statistic_(std::move(statistic.latency_1xx_statistic)), - latency_2xx_statistic_(std::move(statistic.latency_2xx_statistic)), - latency_3xx_statistic_(std::move(statistic.latency_3xx_statistic)), - latency_4xx_statistic_(std::move(statistic.latency_4xx_statistic)), - latency_5xx_statistic_(std::move(statistic.latency_5xx_statistic)), - latency_xxx_statistic_(std::move(statistic.latency_xxx_statistic)), use_h2_(use_h2), + statistic_(statistic), use_h2_(use_h2), benchmark_client_stats_({ALL_BENCHMARK_CLIENT_STATS(POOL_COUNTER(*scope_))}), cluster_manager_(cluster_manager), http_tracer_(http_tracer), cluster_name_(std::string(cluster_name)), request_generator_(std::move(request_generator)), provide_resource_backpressure_(provide_resource_backpressure) { - connect_statistic_->setId("benchmark_http_client.queue_to_connect"); - response_statistic_->setId("benchmark_http_client.request_to_response"); - response_header_size_statistic_->setId("benchmark_http_client.response_header_size"); - response_body_size_statistic_->setId("benchmark_http_client.response_body_size"); - latency_1xx_statistic_->setId("benchmark_http_client.latency_1xx"); - latency_2xx_statistic_->setId("benchmark_http_client.latency_2xx"); - latency_3xx_statistic_->setId("benchmark_http_client.latency_3xx"); - latency_4xx_statistic_->setId("benchmark_http_client.latency_4xx"); - latency_5xx_statistic_->setId("benchmark_http_client.latency_5xx"); - latency_xxx_statistic_->setId("benchmark_http_client.latency_xxx"); + statistic_.connect_statistic->setId("benchmark_http_client.queue_to_connect"); + statistic_.response_statistic->setId("benchmark_http_client.request_to_response"); + statistic_.response_header_size_statistic->setId("benchmark_http_client.response_header_size"); + statistic_.response_body_size_statistic->setId("benchmark_http_client.response_body_size"); + statistic_.latency_1xx_statistic->setId("benchmark_http_client.latency_1xx"); + statistic_.latency_2xx_statistic->setId("benchmark_http_client.latency_2xx"); + statistic_.latency_3xx_statistic->setId("benchmark_http_client.latency_3xx"); + statistic_.latency_4xx_statistic->setId("benchmark_http_client.latency_4xx"); + statistic_.latency_5xx_statistic->setId("benchmark_http_client.latency_5xx"); + statistic_.latency_xxx_statistic->setId("benchmark_http_client.latency_xxx"); } void BenchmarkClientHttpImpl::terminate() { @@ -90,16 +109,18 @@ void BenchmarkClientHttpImpl::terminate() { StatisticPtrMap BenchmarkClientHttpImpl::statistics() const { StatisticPtrMap statistics; - statistics[connect_statistic_->id()] = connect_statistic_.get(); - statistics[response_statistic_->id()] = response_statistic_.get(); - statistics[response_header_size_statistic_->id()] = response_header_size_statistic_.get(); - statistics[response_body_size_statistic_->id()] = response_body_size_statistic_.get(); - statistics[latency_1xx_statistic_->id()] = latency_1xx_statistic_.get(); - statistics[latency_2xx_statistic_->id()] = latency_2xx_statistic_.get(); - statistics[latency_3xx_statistic_->id()] = latency_3xx_statistic_.get(); - statistics[latency_4xx_statistic_->id()] = latency_4xx_statistic_.get(); - statistics[latency_5xx_statistic_->id()] = latency_5xx_statistic_.get(); - statistics[latency_xxx_statistic_->id()] = latency_xxx_statistic_.get(); + statistics[statistic_.connect_statistic->id()] = statistic_.connect_statistic.get(); + statistics[statistic_.response_statistic->id()] = statistic_.response_statistic.get(); + statistics[statistic_.response_header_size_statistic->id()] = + statistic_.response_header_size_statistic.get(); + statistics[statistic_.response_body_size_statistic->id()] = + statistic_.response_body_size_statistic.get(); + statistics[statistic_.latency_1xx_statistic->id()] = statistic_.latency_1xx_statistic.get(); + statistics[statistic_.latency_2xx_statistic->id()] = statistic_.latency_2xx_statistic.get(); + statistics[statistic_.latency_3xx_statistic->id()] = statistic_.latency_3xx_statistic.get(); + statistics[statistic_.latency_4xx_statistic->id()] = statistic_.latency_4xx_statistic.get(); + statistics[statistic_.latency_5xx_statistic->id()] = statistic_.latency_5xx_statistic.get(); + statistics[statistic_.latency_xxx_statistic->id()] = statistic_.latency_xxx_statistic.get(); return statistics; }; @@ -137,9 +158,9 @@ bool BenchmarkClientHttpImpl::tryStartRequest(CompletionCallback caller_completi auto stream_decoder = new StreamDecoder( dispatcher_, api_.timeSource(), *this, std::move(caller_completion_callback), - *connect_statistic_, *response_statistic_, *response_header_size_statistic_, - *response_body_size_statistic_, request->header(), shouldMeasureLatencies(), content_length, - generator_, http_tracer_); + *statistic_.connect_statistic, *statistic_.response_statistic, + *statistic_.response_header_size_statistic, *statistic_.response_body_size_statistic, + request->header(), shouldMeasureLatencies(), content_length, generator_, http_tracer_); requests_initiated_++; pool_ptr->newStream(*stream_decoder, *stream_decoder); return true; @@ -189,17 +210,17 @@ void BenchmarkClientHttpImpl::onPoolFailure(Envoy::Http::ConnectionPool::PoolFai void BenchmarkClientHttpImpl::exportLatency(const uint32_t response_code, const uint64_t latency_ns) { if (response_code > 99 && response_code <= 199) { - latency_1xx_statistic_->addValue(latency_ns); + statistic_.latency_1xx_statistic->addValue(latency_ns); } else if (response_code > 199 && response_code <= 299) { - latency_2xx_statistic_->addValue(latency_ns); + statistic_.latency_2xx_statistic->addValue(latency_ns); } else if (response_code > 299 && response_code <= 399) { - latency_3xx_statistic_->addValue(latency_ns); + statistic_.latency_3xx_statistic->addValue(latency_ns); } else if (response_code > 399 && response_code <= 499) { - latency_4xx_statistic_->addValue(latency_ns); + statistic_.latency_4xx_statistic->addValue(latency_ns); } else if (response_code > 499 && response_code <= 599) { - latency_5xx_statistic_->addValue(latency_ns); + statistic_.latency_5xx_statistic->addValue(latency_ns); } else { - latency_xxx_statistic_->addValue(latency_ns); + statistic_.latency_xxx_statistic->addValue(latency_ns); } } diff --git a/source/client/benchmark_client_impl.h b/source/client/benchmark_client_impl.h index a03a338d3..2584ab997 100644 --- a/source/client/benchmark_client_impl.h +++ b/source/client/benchmark_client_impl.h @@ -44,12 +44,25 @@ using namespace Envoy; // We need this because of macro expectations. COUNTER(pool_overflow) \ COUNTER(pool_connection_failure) +// For counter metrics, Nighthawk use Envoy Counter directly. For histogram metrics, Nighthawk use +// its own Statistic instead of Envoy Histogram. Here BenchmarkClientStats contains only counters +// while BenchmarkClientStatistic contains only histograms. struct BenchmarkClientStats { ALL_BENCHMARK_CLIENT_STATS(GENERATE_COUNTER_STRUCT) }; -// For histogram metrics, Nighthawk Statistic is used instead of Envoy Histogram. +// BenchmarkClientStatistic contains only histogram metrics. struct BenchmarkClientStatistic { + BenchmarkClientStatistic(BenchmarkClientStatistic& statistic); + BenchmarkClientStatistic(StatisticPtr&& connect_stat, StatisticPtr&& response_stat, + StatisticPtr&& response_header_size_stat, + StatisticPtr&& response_body_size_stat, StatisticPtr&& latency_1xx_stat, + StatisticPtr&& latency_2xx_stat, StatisticPtr&& latency_3xx_stat, + StatisticPtr&& latency_4xx_stat, StatisticPtr&& latency_5xx_stat, + StatisticPtr&& latency_xxx_stat); + + // These are declared order dependent. Changing ordering may trigger on assert upon + // destruction when tls has been involved during usage. StatisticPtr connect_statistic; StatisticPtr response_statistic; StatisticPtr response_header_size_statistic; @@ -131,18 +144,7 @@ class BenchmarkClientHttpImpl : public BenchmarkClient, Envoy::Api::Api& api_; Envoy::Event::Dispatcher& dispatcher_; Envoy::Stats::ScopePtr scope_; - // These are declared order dependent. Changing ordering may trigger on assert upon - // destruction when tls has been involved during usage. - StatisticPtr connect_statistic_; - StatisticPtr response_statistic_; - StatisticPtr response_header_size_statistic_; - StatisticPtr response_body_size_statistic_; - StatisticPtr latency_1xx_statistic_; - StatisticPtr latency_2xx_statistic_; - StatisticPtr latency_3xx_statistic_; - StatisticPtr latency_4xx_statistic_; - StatisticPtr latency_5xx_statistic_; - StatisticPtr latency_xxx_statistic_; + BenchmarkClientStatistic statistic_; const bool use_h2_; std::chrono::seconds timeout_{5s}; uint32_t connection_limit_{1}; diff --git a/source/client/factories_impl.cc b/source/client/factories_impl.cc index 6f488edf5..b767733d3 100644 --- a/source/client/factories_impl.cc +++ b/source/client/factories_impl.cc @@ -31,7 +31,7 @@ BenchmarkClientPtr BenchmarkClientFactoryImpl::create( Envoy::Api::Api& api, Envoy::Event::Dispatcher& dispatcher, Envoy::Stats::Scope& scope, Envoy::Upstream::ClusterManagerPtr& cluster_manager, Envoy::Tracing::HttpTracerSharedPtr& http_tracer, absl::string_view cluster_name, - const int sink_stat_prefix, RequestSource& request_generator) const { + int sink_stat_prefix, RequestSource& request_generator) const { StatisticFactoryImpl statistic_factory(options_); // While we lack options to configure which statistic backend goes where, we directly pass // StreamingStatistic for the stats that track response sizes. Ideally we would have options @@ -39,17 +39,15 @@ BenchmarkClientPtr BenchmarkClientFactoryImpl::create( // NullStatistic). // TODO(#292): Create options and have the StatisticFactory consider those when instantiating // statistics. - BenchmarkClientStatistic statistic; - statistic.connect_statistic = statistic_factory.create(); - statistic.response_statistic = statistic_factory.create(); - statistic.response_header_size_statistic = std::make_unique(); - statistic.response_body_size_statistic = std::make_unique(); - statistic.latency_1xx_statistic = std::make_unique(scope, sink_stat_prefix); - statistic.latency_2xx_statistic = std::make_unique(scope, sink_stat_prefix); - statistic.latency_3xx_statistic = std::make_unique(scope, sink_stat_prefix); - statistic.latency_4xx_statistic = std::make_unique(scope, sink_stat_prefix); - statistic.latency_5xx_statistic = std::make_unique(scope, sink_stat_prefix); - statistic.latency_xxx_statistic = std::make_unique(scope, sink_stat_prefix); + BenchmarkClientStatistic statistic( + statistic_factory.create(), statistic_factory.create(), + std::make_unique(), std::make_unique(), + std::make_unique(scope, sink_stat_prefix), + std::make_unique(scope, sink_stat_prefix), + std::make_unique(scope, sink_stat_prefix), + std::make_unique(scope, sink_stat_prefix), + std::make_unique(scope, sink_stat_prefix), + std::make_unique(scope, sink_stat_prefix)); auto benchmark_client = std::make_unique( api, dispatcher, scope, statistic, options_.h2(), cluster_manager, http_tracer, cluster_name, request_generator.get(), !options_.openLoop()); diff --git a/source/client/factories_impl.h b/source/client/factories_impl.h index 2be43ee7a..ff99e08c7 100644 --- a/source/client/factories_impl.h +++ b/source/client/factories_impl.h @@ -31,7 +31,7 @@ class BenchmarkClientFactoryImpl : public OptionBasedFactoryImpl, public Benchma Envoy::Stats::Scope& scope, Envoy::Upstream::ClusterManagerPtr& cluster_manager, Envoy::Tracing::HttpTracerSharedPtr& http_tracer, - absl::string_view cluster_name, const int sink_stat_prefix, + absl::string_view cluster_name, int sink_stat_prefix, RequestSource& request_generator) const override; }; diff --git a/test/benchmark_http_client_test.cc b/test/benchmark_http_client_test.cc index a34dd983a..8835be557 100644 --- a/test/benchmark_http_client_test.cc +++ b/test/benchmark_http_client_test.cc @@ -33,7 +33,12 @@ class BenchmarkClientHttpTest : public Test { dispatcher_(api_->allocateDispatcher("test_thread")), cluster_manager_(std::make_unique()), cluster_info_(std::make_unique()), - http_tracer_(std::make_unique()), response_code_("200") { + http_tracer_(std::make_unique()), response_code_("200"), + statistic_(std::make_unique(), std::make_unique(), + std::make_unique(), std::make_unique(), + std::make_unique(), std::make_unique(), + std::make_unique(), std::make_unique(), + std::make_unique(), std::make_unique()) { EXPECT_CALL(cluster_manager(), httpConnPoolForCluster(_, _, _, _)) .WillRepeatedly(Return(&pool_)); EXPECT_CALL(cluster_manager(), get(_)).WillRepeatedly(Return(&thread_local_cluster_)); @@ -55,16 +60,6 @@ class BenchmarkClientHttpTest : public Test { {{":scheme", "http"}, {":method", "GET"}, {":path", "/"}, {":host", "localhost"}})); return std::make_unique(header); }; - statistic_.connect_statistic = std::make_unique(); - statistic_.response_statistic = std::make_unique(); - statistic_.response_header_size_statistic = std::make_unique(); - statistic_.response_body_size_statistic = std::make_unique(); - statistic_.latency_1xx_statistic = std::make_unique(); - statistic_.latency_2xx_statistic = std::make_unique(); - statistic_.latency_3xx_statistic = std::make_unique(); - statistic_.latency_4xx_statistic = std::make_unique(); - statistic_.latency_5xx_statistic = std::make_unique(); - statistic_.latency_xxx_statistic = std::make_unique(); } void testBasicFunctionality(const uint64_t max_pending, const uint64_t connection_limit, @@ -203,11 +198,15 @@ TEST_F(BenchmarkClientHttpTest, EnableLatencyMeasurement) { testBasicFunctionality(10, 1, 10); EXPECT_EQ(0, client_->statistics()["benchmark_http_client.queue_to_connect"]->count()); EXPECT_EQ(0, client_->statistics()["benchmark_http_client.request_to_response"]->count()); + EXPECT_EQ(10, client_->statistics()["benchmark_http_client.response_header_size"]->count()); + EXPECT_EQ(10, client_->statistics()["benchmark_http_client.response_body_size"]->count()); EXPECT_EQ(0, client_->statistics()["benchmark_http_client.latency_2xx"]->count()); client_->setShouldMeasureLatencies(true); testBasicFunctionality(10, 1, 10); EXPECT_EQ(10, client_->statistics()["benchmark_http_client.queue_to_connect"]->count()); EXPECT_EQ(10, client_->statistics()["benchmark_http_client.request_to_response"]->count()); + EXPECT_EQ(20, client_->statistics()["benchmark_http_client.response_header_size"]->count()); + EXPECT_EQ(20, client_->statistics()["benchmark_http_client.response_body_size"]->count()); EXPECT_EQ(10, client_->statistics()["benchmark_http_client.latency_2xx"]->count()); } From cc7ecc3668ddc945f1c8e242c260afd2f262c819 Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Mon, 20 Jul 2020 14:35:22 -0400 Subject: [PATCH 41/44] delete module Signed-off-by: Qin Qin --- nighthawk | 1 - 1 file changed, 1 deletion(-) delete mode 160000 nighthawk diff --git a/nighthawk b/nighthawk deleted file mode 160000 index 0f4fc953d..000000000 --- a/nighthawk +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 0f4fc953dcbf0d85605309714ef99cd46889cd95 From 8d9293309731c589133c2534112402ba9bd4bbb6 Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Mon, 20 Jul 2020 16:19:52 -0400 Subject: [PATCH 42/44] rerun CI Signed-off-by: Qin Qin --- docs/root/statistics.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/statistics.md b/docs/root/statistics.md index b6e0142b3..a97bbcd14 100644 --- a/docs/root/statistics.md +++ b/docs/root/statistics.md @@ -14,7 +14,7 @@ metric, Nighthawk wraps Envoy's Histogram into its own Statistic concept (see [#391](https://github.com/envoyproxy/nighthawk/pull/391)). Name | Type | Description ------| ----- | ---------------- +-----| ----- | ---------------- upstream_rq_total | Counter | Total number of requests sent from Nighthawk http_1xx | Counter | Total number of response with code 1xx http_2xx | Counter | Total number of response with code 2xx From d8f4d310040d1f99eb78d0ede8d9f616308eb8bf Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Wed, 22 Jul 2020 17:46:24 -0400 Subject: [PATCH 43/44] change sink_stat_prefix to worker_id Signed-off-by: Qin Qin --- docs/root/statistics.md | 23 +++++++++++++----- include/nighthawk/client/factories.h | 4 ++-- source/client/benchmark_client_impl.cc | 24 +++++++++---------- source/client/benchmark_client_impl.h | 14 +++++------ source/client/factories_impl.cc | 22 ++++++++--------- source/client/factories_impl.h | 2 +- test/factories_test.cc | 2 +- .../client/mock_benchmark_client_factory.h | 2 +- 8 files changed, 52 insertions(+), 41 deletions(-) diff --git a/docs/root/statistics.md b/docs/root/statistics.md index a97bbcd14..8972aafd1 100644 --- a/docs/root/statistics.md +++ b/docs/root/statistics.md @@ -32,7 +32,7 @@ benchmark_http_client.latency_4xx | HdrStatistic | Latency (in Nanosecond) histo benchmark_http_client.latency_5xx | HdrStatistic | Latency (in Nanosecond) histogram of request with code 5xx benchmark_http_client.latency_xxx | HdrStatistic | Latency (in Nanosecond) histogram of request with code <100 or >=600 benchmark_http_client.queue_to_connect | HdrStatistic | Histogram of request connection time (in Nanosecond) -benchmark_http_client.request_to_response | HdrStatistic | Latency (in Nanosecond) histogram of all requests (include requests with stream reset or pool failure) +benchmark_http_client.request_to_response | HdrStatistic | Latency (in Nanosecond) histogram include requests with stream reset or pool failure benchmark_http_client.response_header_size | StreamingStatistic | Statistic of response header size (min, max, mean, pstdev values in bytes) benchmark_http_client.response_body_size | StreamingStatistic | Statistic of response body size (min, max, mean, pstdev values in bytes) sequencer.callback | HdrStatistic | Latency (in Nanosecond) histogram of unblocked requests @@ -65,20 +65,30 @@ backend-specific format. Envoy metrics can be defined using a macro, e.g. ```cc +// Define Envoy stats. #define ALL_CLUSTER_STATS(COUNTER, GAUGE, HISTOGRAM) COUNTER(upstream_cx_total) GAUGE(upstream_cx_active, NeverImport) - HISTOGRAM(upstream_cx_length, Milliseconds) + HISTOGRAM(upstream_cx_length, Milliseconds) +// Put these stats as members of a struct. struct ClusterStats { ALL_CLUSTER_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT) -}; +}; +// Instantiate the above struct using a Stats::Pool. +ClusterStats stats{ + ALL_CLUSTER_STATS(POOL_COUNTER(...), POOL_GAUGE(...), POOL_HISTOGRAM(...))}; + +// Stats can be updated in the code: +stats.upstream_cx_total_.inc(); +stats.upstream_cx_active_.set(...); +stats.upstream_cx_length_.recordValue(...); ``` ## Envoy Metrics Limitation Currently Envoy metrics don't support key-value map. As a result, for metrics to be broken down by certain dimensions, we need to define a separate metric for -each dimension. For example, currently Nighthawk defines separate -[counters](https://github.com/envoyproxy/nighthawk/blob/master/source/client/benchmark_client_impl.h#L35-L40) +each dimension. For example, currently Nighthawk defines +[separate counters](https://github.com/envoyproxy/nighthawk/blob/master/source/client/benchmark_client_impl.h#L35-L40) to monitor the number of responses with corresponding response code. ## Envoy Metrics Flush @@ -100,7 +110,8 @@ metrics (aggregated across all workers). - Per worker level metrics provide information about the performance of each worker which will be hidden by the global level metrics. - Keep the workers independent which makes it easier/efficient to scale up to - multiple Nighthawks with large numbers of workers. + multiple Nighthawks with large numbers of workers. (The work to scale up to + multiple Nighthawks is still under development). Envoy metrics can be defined at per worker level using [Scope](https://github.com/envoyproxy/envoy/blob/e9c2c8c4a0141c9634316e8283f98f412d0dd207/include/envoy/stats/scope.h#L35) diff --git a/include/nighthawk/client/factories.h b/include/nighthawk/client/factories.h index 44b90b487..cc743d36c 100644 --- a/include/nighthawk/client/factories.h +++ b/include/nighthawk/client/factories.h @@ -36,7 +36,7 @@ class BenchmarkClientFactory { * @param cluster_name Name of the cluster that this benchmark client * will use. In conjunction with cluster_manager this will allow the this BenchmarkClient to * access the target connection pool. - * @param sink_stat_prefix Prefix of the stat (e.g. worker id). + * @param worker_id Worker number. * @param request_source Source of request-specifiers. Will be queries every time the * BenchmarkClient is asked to issue a request. * @@ -46,7 +46,7 @@ class BenchmarkClientFactory { Envoy::Stats::Scope& scope, Envoy::Upstream::ClusterManagerPtr& cluster_manager, Envoy::Tracing::HttpTracerSharedPtr& http_tracer, - absl::string_view cluster_name, const int sink_stat_prefix, + absl::string_view cluster_name, int worker_id, RequestSource& request_source) const PURE; }; diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index cbcb7764b..f373f542b 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -20,7 +20,7 @@ using namespace std::chrono_literals; namespace Nighthawk { namespace Client { -BenchmarkClientStatistic::BenchmarkClientStatistic(BenchmarkClientStatistic& statistic) +BenchmarkClientStatistic::BenchmarkClientStatistic(BenchmarkClientStatistic&& statistic) : connect_statistic(std::move(statistic.connect_statistic)), response_statistic(std::move(statistic.response_statistic)), response_header_size_statistic(std::move(statistic.response_header_size_statistic)), @@ -82,8 +82,8 @@ BenchmarkClientHttpImpl::BenchmarkClientHttpImpl( Envoy::Tracing::HttpTracerSharedPtr& http_tracer, absl::string_view cluster_name, RequestGenerator request_generator, const bool provide_resource_backpressure) : api_(api), dispatcher_(dispatcher), scope_(scope.createScope("benchmark.")), - statistic_(statistic), use_h2_(use_h2), - benchmark_client_stats_({ALL_BENCHMARK_CLIENT_STATS(POOL_COUNTER(*scope_))}), + statistic_(std::move(statistic)), use_h2_(use_h2), + benchmark_client_counters_({ALL_BENCHMARK_CLIENT_COUNTERS(POOL_COUNTER(*scope_))}), cluster_manager_(cluster_manager), http_tracer_(http_tracer), cluster_name_(std::string(cluster_name)), request_generator_(std::move(request_generator)), provide_resource_backpressure_(provide_resource_backpressure) { @@ -170,23 +170,23 @@ void BenchmarkClientHttpImpl::onComplete(bool success, const Envoy::Http::ResponseHeaderMap& headers) { requests_completed_++; if (!success) { - benchmark_client_stats_.stream_resets_.inc(); + benchmark_client_counters_.stream_resets_.inc(); } else { ASSERT(headers.Status()); const int64_t status = Envoy::Http::Utility::getResponseStatus(headers); if (status > 99 && status <= 199) { - benchmark_client_stats_.http_1xx_.inc(); + benchmark_client_counters_.http_1xx_.inc(); } else if (status > 199 && status <= 299) { - benchmark_client_stats_.http_2xx_.inc(); + benchmark_client_counters_.http_2xx_.inc(); } else if (status > 299 && status <= 399) { - benchmark_client_stats_.http_3xx_.inc(); + benchmark_client_counters_.http_3xx_.inc(); } else if (status > 399 && status <= 499) { - benchmark_client_stats_.http_4xx_.inc(); + benchmark_client_counters_.http_4xx_.inc(); } else if (status > 499 && status <= 599) { - benchmark_client_stats_.http_5xx_.inc(); + benchmark_client_counters_.http_5xx_.inc(); } else { - benchmark_client_stats_.http_xxx_.inc(); + benchmark_client_counters_.http_xxx_.inc(); } } } @@ -194,11 +194,11 @@ void BenchmarkClientHttpImpl::onComplete(bool success, void BenchmarkClientHttpImpl::onPoolFailure(Envoy::Http::ConnectionPool::PoolFailureReason reason) { switch (reason) { case Envoy::Http::ConnectionPool::PoolFailureReason::Overflow: - benchmark_client_stats_.pool_overflow_.inc(); + benchmark_client_counters_.pool_overflow_.inc(); break; case Envoy::Http::ConnectionPool::PoolFailureReason::LocalConnectionFailure: case Envoy::Http::ConnectionPool::PoolFailureReason::RemoteConnectionFailure: - benchmark_client_stats_.pool_connection_failure_.inc(); + benchmark_client_counters_.pool_connection_failure_.inc(); break; case Envoy::Http::ConnectionPool::PoolFailureReason::Timeout: break; diff --git a/source/client/benchmark_client_impl.h b/source/client/benchmark_client_impl.h index 2584ab997..1027e74ea 100644 --- a/source/client/benchmark_client_impl.h +++ b/source/client/benchmark_client_impl.h @@ -33,7 +33,7 @@ using namespace std::chrono_literals; using namespace Envoy; // We need this because of macro expectations. -#define ALL_BENCHMARK_CLIENT_STATS(COUNTER) \ +#define ALL_BENCHMARK_CLIENT_COUNTERS(COUNTER) \ COUNTER(stream_resets) \ COUNTER(http_1xx) \ COUNTER(http_2xx) \ @@ -44,16 +44,16 @@ using namespace Envoy; // We need this because of macro expectations. COUNTER(pool_overflow) \ COUNTER(pool_connection_failure) -// For counter metrics, Nighthawk use Envoy Counter directly. For histogram metrics, Nighthawk use -// its own Statistic instead of Envoy Histogram. Here BenchmarkClientStats contains only counters +// For counter metrics, Nighthawk use Envoy Counter directly. For histogram metrics, Nighthawk uses +// its own Statistic instead of Envoy Histogram. Here BenchmarkClientCounters contains only counters // while BenchmarkClientStatistic contains only histograms. -struct BenchmarkClientStats { - ALL_BENCHMARK_CLIENT_STATS(GENERATE_COUNTER_STRUCT) +struct BenchmarkClientCounters { + ALL_BENCHMARK_CLIENT_COUNTERS(GENERATE_COUNTER_STRUCT) }; // BenchmarkClientStatistic contains only histogram metrics. struct BenchmarkClientStatistic { - BenchmarkClientStatistic(BenchmarkClientStatistic& statistic); + BenchmarkClientStatistic(BenchmarkClientStatistic&& statistic); BenchmarkClientStatistic(StatisticPtr&& connect_stat, StatisticPtr&& response_stat, StatisticPtr&& response_header_size_stat, StatisticPtr&& response_body_size_stat, StatisticPtr&& latency_1xx_stat, @@ -156,7 +156,7 @@ class BenchmarkClientHttpImpl : public BenchmarkClient, uint64_t requests_completed_{}; uint64_t requests_initiated_{}; bool measure_latencies_{}; - BenchmarkClientStats benchmark_client_stats_; + BenchmarkClientCounters benchmark_client_counters_; Envoy::Upstream::ClusterManagerPtr& cluster_manager_; Envoy::Tracing::HttpTracerSharedPtr& http_tracer_; std::string cluster_name_; diff --git a/source/client/factories_impl.cc b/source/client/factories_impl.cc index b767733d3..eb1187c87 100644 --- a/source/client/factories_impl.cc +++ b/source/client/factories_impl.cc @@ -30,8 +30,8 @@ BenchmarkClientFactoryImpl::BenchmarkClientFactoryImpl(const Options& options) BenchmarkClientPtr BenchmarkClientFactoryImpl::create( Envoy::Api::Api& api, Envoy::Event::Dispatcher& dispatcher, Envoy::Stats::Scope& scope, Envoy::Upstream::ClusterManagerPtr& cluster_manager, - Envoy::Tracing::HttpTracerSharedPtr& http_tracer, absl::string_view cluster_name, - int sink_stat_prefix, RequestSource& request_generator) const { + Envoy::Tracing::HttpTracerSharedPtr& http_tracer, absl::string_view cluster_name, int worker_id, + RequestSource& request_generator) const { StatisticFactoryImpl statistic_factory(options_); // While we lack options to configure which statistic backend goes where, we directly pass // StreamingStatistic for the stats that track response sizes. Ideally we would have options @@ -39,15 +39,15 @@ BenchmarkClientPtr BenchmarkClientFactoryImpl::create( // NullStatistic). // TODO(#292): Create options and have the StatisticFactory consider those when instantiating // statistics. - BenchmarkClientStatistic statistic( - statistic_factory.create(), statistic_factory.create(), - std::make_unique(), std::make_unique(), - std::make_unique(scope, sink_stat_prefix), - std::make_unique(scope, sink_stat_prefix), - std::make_unique(scope, sink_stat_prefix), - std::make_unique(scope, sink_stat_prefix), - std::make_unique(scope, sink_stat_prefix), - std::make_unique(scope, sink_stat_prefix)); + BenchmarkClientStatistic statistic(statistic_factory.create(), statistic_factory.create(), + std::make_unique(), + std::make_unique(), + std::make_unique(scope, worker_id), + std::make_unique(scope, worker_id), + std::make_unique(scope, worker_id), + std::make_unique(scope, worker_id), + std::make_unique(scope, worker_id), + std::make_unique(scope, worker_id)); auto benchmark_client = std::make_unique( api, dispatcher, scope, statistic, options_.h2(), cluster_manager, http_tracer, cluster_name, request_generator.get(), !options_.openLoop()); diff --git a/source/client/factories_impl.h b/source/client/factories_impl.h index ff99e08c7..5e508b65a 100644 --- a/source/client/factories_impl.h +++ b/source/client/factories_impl.h @@ -31,7 +31,7 @@ class BenchmarkClientFactoryImpl : public OptionBasedFactoryImpl, public Benchma Envoy::Stats::Scope& scope, Envoy::Upstream::ClusterManagerPtr& cluster_manager, Envoy::Tracing::HttpTracerSharedPtr& http_tracer, - absl::string_view cluster_name, int sink_stat_prefix, + absl::string_view cluster_name, int worker_id, RequestSource& request_generator) const override; }; diff --git a/test/factories_test.cc b/test/factories_test.cc index b3cdbf0e7..2a74d8c5b 100644 --- a/test/factories_test.cc +++ b/test/factories_test.cc @@ -47,7 +47,7 @@ TEST_F(FactoriesTest, CreateBenchmarkClient) { std::make_unique()); auto benchmark_client = factory.create(*api_, dispatcher_, stats_store_, cluster_manager, http_tracer_, "foocluster", - /*sink_stat_prefix=*/0, request_generator); + /*worker_id=*/0, request_generator); EXPECT_NE(nullptr, benchmark_client.get()); } diff --git a/test/mocks/client/mock_benchmark_client_factory.h b/test/mocks/client/mock_benchmark_client_factory.h index 097440656..bddf91eb1 100644 --- a/test/mocks/client/mock_benchmark_client_factory.h +++ b/test/mocks/client/mock_benchmark_client_factory.h @@ -18,7 +18,7 @@ class MockBenchmarkClientFactory : public BenchmarkClientFactory { BenchmarkClientPtr(Envoy::Api::Api&, Envoy::Event::Dispatcher&, Envoy::Stats::Scope&, Envoy::Upstream::ClusterManagerPtr&, Envoy::Tracing::HttpTracerSharedPtr&, absl::string_view, - const int, RequestSource& request_generator)); + int, RequestSource& request_generator)); }; } // namespace Client From 5c3651574e610f4350c825843e373a2c888a29d7 Mon Sep 17 00:00:00 2001 From: Qin Qin Date: Wed, 22 Jul 2020 19:52:03 -0400 Subject: [PATCH 44/44] fix clang Signed-off-by: Qin Qin --- docs/root/statistics.md | 8 +++++--- source/client/benchmark_client_impl.cc | 2 +- source/client/benchmark_client_impl.h | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/root/statistics.md b/docs/root/statistics.md index 8972aafd1..932e5e53f 100644 --- a/docs/root/statistics.md +++ b/docs/root/statistics.md @@ -52,9 +52,11 @@ sequencer.blocking | HdrStatistic | Latency (in Nanosecond) histogram of blocked In Envoy, the stat [Store](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/include/envoy/stats/store.h#L29) is a singleton and provides a simple interface by which the rest of the code can -obtain handles to scopes, counters, gauges, and histograms. Envoy counters and -gauges are periodically (configured at ~5 sec interval) flushed to the sinks. -Note that currently histogram values are sent directly to the sinks. A stat +obtain handles to +[scopes](https://github.com/envoyproxy/envoy/blob/958745d658752f90f544296d9e75030519a9fb84/include/envoy/stats/scope.h#L37), +counters, gauges, and histograms. Envoy counters and gauges are periodically +(configured at ~5 sec interval) flushed to the sinks. Note that currently +histogram values are sent directly to the sinks. A stat [Sink](https://github.com/envoyproxy/envoy/blob/74530c92cfa3682b49b540fddf2aba40ac10c68e/include/envoy/stats/sink.h#L48) is an interface that takes generic stat data and translates it into a backend-specific wire format. Currently Envoy supports the TCP and UDP diff --git a/source/client/benchmark_client_impl.cc b/source/client/benchmark_client_impl.cc index f373f542b..32c10a4c1 100644 --- a/source/client/benchmark_client_impl.cc +++ b/source/client/benchmark_client_impl.cc @@ -20,7 +20,7 @@ using namespace std::chrono_literals; namespace Nighthawk { namespace Client { -BenchmarkClientStatistic::BenchmarkClientStatistic(BenchmarkClientStatistic&& statistic) +BenchmarkClientStatistic::BenchmarkClientStatistic(BenchmarkClientStatistic&& statistic) noexcept : connect_statistic(std::move(statistic.connect_statistic)), response_statistic(std::move(statistic.response_statistic)), response_header_size_statistic(std::move(statistic.response_header_size_statistic)), diff --git a/source/client/benchmark_client_impl.h b/source/client/benchmark_client_impl.h index 1027e74ea..cc2e9296f 100644 --- a/source/client/benchmark_client_impl.h +++ b/source/client/benchmark_client_impl.h @@ -53,7 +53,7 @@ struct BenchmarkClientCounters { // BenchmarkClientStatistic contains only histogram metrics. struct BenchmarkClientStatistic { - BenchmarkClientStatistic(BenchmarkClientStatistic&& statistic); + BenchmarkClientStatistic(BenchmarkClientStatistic&& statistic) noexcept; BenchmarkClientStatistic(StatisticPtr&& connect_stat, StatisticPtr&& response_stat, StatisticPtr&& response_header_size_stat, StatisticPtr&& response_body_size_stat, StatisticPtr&& latency_1xx_stat,