From 9830e1e9b43cb6d1ae98efdaa251eaccc6a4db63 Mon Sep 17 00:00:00 2001 From: Bruno Palermo Date: Thu, 17 Sep 2026 17:52:00 -0300 Subject: [PATCH 1/6] stats: record Nighthawk statistics into an Envoy store histogram Nighthawk's latency statistics reached stats sinks only through Stats::Store::deliverHistogramToSinks(), called directly from recordValue(). SinkableStatistic is built as a HistogramImplHelper over an empty MetricImpl and is never registered with the store, so it never appears in MetricSnapshot::histograms(). Sinks that read the snapshot on flush rather than implementing onHistogramComplete() therefore received nothing at all for them: the OpenTelemetry, metrics service and hystrix sinks all define onHistogramComplete() as an empty override, so a run exporting through them carried counters, gauges and Envoy's own histograms while every Nighthawk latency metric was silently absent. Record each sample into an Envoy store histogram as well. ParentHistogramImpl::recordValue() calls deliverHistogramToSinks() itself, so this replaces rather than duplicates the direct delivery: one call now feeds both the snapshot and the sinks that want individual samples. The histogram is created in the worker's "cluster.." scope and named after the statistic's id, so emitted names are unchanged and a sink can still recover the worker from the name. It is bound when the id is assigned, since a statistic is named after construction, and the lookup is cached so recording stays a pointer dereference. The mirror does not change what Nighthawk reports: its output is rendered from the HdrHistogram or Circllhist data, which keeps its nanosecond resolution. That leaves the mirror free to carry a coarser representation later, once Envoy's statsd sinks scale by unit, without touching what Nighthawk measures. Signed-off-by: Bruno Palermo --- docs/root/version_history.md | 1 + source/common/statistic_impl.cc | 36 +++++++++++++++++++++++++++------ source/common/statistic_impl.h | 29 ++++++++++++++++++++++++++ test/statistic_test.cc | 28 +++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 6 deletions(-) diff --git a/docs/root/version_history.md b/docs/root/version_history.md index 8205d41f3..7f0ebd575 100644 --- a/docs/root/version_history.md +++ b/docs/root/version_history.md @@ -14,6 +14,7 @@ Version history ### Changelist +- Nighthawk's latency statistics are now recorded into an Envoy store histogram as well as their own HdrHistogram/Circllhist, so they appear in `MetricSnapshot::histograms()`. Stats sinks that only read the snapshot on flush, such as the OpenTelemetry and metrics service sinks, previously received nothing for them: Nighthawk delivered samples exclusively through `deliverHistogramToSinks()`, which those sinks implement as a no-op. Sinks reading `onHistogramComplete()` are unaffected, and the emitted metric names are unchanged, since the mirror is created in the worker's `cluster..` scope. Nighthawk's own output still comes from the statistic itself and keeps its nanosecond resolution. - `--request-body-file` sends a file's bytes verbatim as the request body (binary safe; no Content-Type is set). The `RequestOptions.request_body` (bytes) field carries it over the gRPC service API. Mutually exclusive with `--request-body-size`. - `OptionsImpl::toCommandLineOptions()` now always emits `request_options.request_body_size`; it was only set when at least one `--request-header` was configured, so the size was lost on the gRPC service path otherwise. - The Envoy exception on the tunneling startup path is logged instead of printed to stdout, keeping stdout reserved for the formatted output. diff --git a/source/common/statistic_impl.cc b/source/common/statistic_impl.cc index 22ed7ecf5..7b85ef70d 100644 --- a/source/common/statistic_impl.cc +++ b/source/common/statistic_impl.cc @@ -418,14 +418,34 @@ Envoy::Stats::Histogram::Unit SinkableStatistic::unit() const { Envoy::Stats::SymbolTable& SinkableStatistic::symbolTable() { return scope_.symbolTable(); } +Envoy::Stats::Histogram& SinkableStatistic::storeHistogram() { + if (store_histogram_ == nullptr) { + bindStoreHistogram(); + } + return *store_histogram_; +} + +void SinkableStatistic::bindStoreHistogram() { + // name() is the statistic's id; the scope supplies the "cluster.." prefix. Resolved once and + // cached, so recording a sample stays a pointer dereference. + store_histogram_ = &scope_.histogramFromString(name(), unit()); +} + SinkableHdrStatistic::SinkableHdrStatistic(Envoy::Stats::Scope& scope, std::optional worker_id) : SinkableStatistic(scope, worker_id) {} void SinkableHdrStatistic::recordValue(uint64_t value) { HdrStatistic::addValue(value); - // Currently in Envoy Scope implementation, deliverHistogramToSinks() will flush the histogram - // value directly to stats Sinks. - scope_.store().deliverHistogramToSinks(*this, value); + // Recording into the store histogram delivers the sample to sinks implementing + // onHistogramComplete() and, unlike a direct deliverHistogramToSinks() call, also makes the + // statistic visible to sinks that only read MetricSnapshot::histograms() on flush, such as the + // OpenTelemetry and metrics service sinks. See SinkableStatistic::storeHistogram(). + storeHistogram().recordValue(value); +} + +void SinkableHdrStatistic::setId(absl::string_view id) { + HdrStatistic::setId(id); + bindStoreHistogram(); } std::string SinkableHdrStatistic::tagExtractedName() const { @@ -442,9 +462,13 @@ SinkableCircllhistStatistic::SinkableCircllhistStatistic(Envoy::Stats::Scope& sc void SinkableCircllhistStatistic::recordValue(uint64_t value) { CircllhistStatistic::addValue(value); - // Currently in Envoy Scope implementation, deliverHistogramToSinks() will flush the histogram - // value directly to stats Sinks. - scope_.store().deliverHistogramToSinks(*this, value); + // See the comment in SinkableHdrStatistic::recordValue(). + storeHistogram().recordValue(value); +} + +void SinkableCircllhistStatistic::setId(absl::string_view id) { + CircllhistStatistic::setId(id); + bindStoreHistogram(); } std::string SinkableCircllhistStatistic::tagExtractedName() const { diff --git a/source/common/statistic_impl.h b/source/common/statistic_impl.h index b5f9c25d0..a546f4f02 100644 --- a/source/common/statistic_impl.h +++ b/source/common/statistic_impl.h @@ -211,12 +211,37 @@ class SinkableStatistic : public Envoy::Stats::HistogramImplHelper { const std::optional worker_id() const { return worker_id_; } protected: + /** + * The Envoy store histogram that mirrors this statistic, created on first use. Recording a + * sample into it both feeds the snapshot that flush based stats sinks read and delivers the + * sample to sinks implementing onHistogramComplete(), because + * Envoy::Stats::ParentHistogramImpl::recordValue() calls deliverHistogramToSinks() itself. + * + * It is created lazily rather than in the constructor because a statistic's id, which names the + * histogram, is assigned after construction. The scope is the worker's ("cluster.."), so the + * histogram is named "cluster.." and stats sinks can recover the worker from the name. + * + * Nighthawk's own output is unaffected: it is rendered from this statistic's HdrHistogram or + * Circllhist data, not from the mirror, so the mirror is free to carry a coarser representation + * than the nanoseconds recorded here. + */ + Envoy::Stats::Histogram& storeHistogram(); + + /** + * Points storeHistogram() at the store histogram named by the statistic's current id. Called + * when the id is assigned, so that the mirror is named correctly regardless of whether any + * sample was recorded before that. + */ + void bindStoreHistogram(); + // This is used in child class 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. std::optional worker_id_; + // Owned by the store; see storeHistogram(). + Envoy::Stats::Histogram* store_histogram_{nullptr}; }; // Implementation of sinkable Nighthawk Statistic with HdrHistogram. @@ -239,6 +264,8 @@ class SinkableHdrStatistic : public SinkableStatistic, public HdrStatistic { // Nighthawk::Statistic void addValue(uint64_t value) override { recordValue(value); } + // Rebinds the store histogram, which is named after the id. + void setId(absl::string_view id) override; }; // Implementation of sinkable Nighthawk Statistic with Circllhist Histogram. @@ -262,6 +289,8 @@ class SinkableCircllhistStatistic : public SinkableStatistic, public CircllhistS // Nighthawk::Statistic void addValue(uint64_t value) override { recordValue(value); } + // Rebinds the store histogram, which is named after the id. + void setId(absl::string_view id) override; }; } // namespace Nighthawk diff --git a/test/statistic_test.cc b/test/statistic_test.cc index 63497c8c1..f810de291 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -6,7 +6,9 @@ #include // std::bad_cast #include "external/envoy/source/common/protobuf/utility.h" +#include "external/envoy/source/common/stats/allocator_impl.h" #include "external/envoy/source/common/stats/isolated_store_impl.h" +#include "external/envoy/source/common/stats/thread_local_store.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" @@ -475,4 +477,30 @@ TYPED_TEST(SinkableStatisticTest, SimpleSinkableStatistic) { EXPECT_EQ(worker_id, stat.worker_id().value()); } +TYPED_TEST(SinkableStatisticTest, IsVisibleInTheStoreSnapshot) { + // The point of mirroring into a store histogram: stats sinks that only read + // MetricSnapshot::histograms() on flush (the OpenTelemetry and metrics service sinks, which + // implement onHistogramComplete() as a no-op) see Nighthawk's latency statistics at all. + // A ThreadLocalStoreImpl as in ProcessImpl; IsolatedStoreImpl::histograms() always returns an + // empty vector, so it cannot show this. + Envoy::Stats::SymbolTableImpl symbol_table; + Envoy::Stats::AllocatorImpl allocator(symbol_table); + Envoy::Stats::ThreadLocalStoreImpl store(allocator); + Envoy::Stats::ScopeSharedPtr worker_scope = store.createScope("cluster.0."); + TypeParam stat(*worker_scope, /*worker_id=*/0); + stat.setId("benchmark_http_client.latency_2xx"); + stat.recordValue(1500); + + std::vector histogram_names; + for (const Envoy::Stats::ParentHistogramSharedPtr& histogram : store.histograms()) { + histogram_names.push_back(histogram->name()); + } + // The worker scope supplies the prefix, so a sink can recover the worker from the name. + EXPECT_THAT(histogram_names, Contains("cluster.0.benchmark_http_client.latency_2xx")); + // Nighthawk's own view of the statistic is unchanged; it is rendered from the Hdr/Circllhist + // data, not from the mirror. + EXPECT_EQ(1, stat.count()); + EXPECT_EQ(1500, stat.max()); +} + } // namespace Nighthawk From 9fa489169f694dc443051bb8f61b5bc0a54fcf77 Mon Sep 17 00:00:00 2001 From: Bruno Palermo Date: Thu, 17 Sep 2026 17:52:00 -0300 Subject: [PATCH 2/6] test: guard the threading contract for hosting Envoy stats sinks Envoy's own stats sinks resolve a thread local writer on every use, and whether Nighthawk can host them was an open question blocking a proposal to consume Envoy's StatsSinkFactory directly rather than maintaining Nighthawk specific sinks. It can. Both cases here mirror ProcessImpl's arrangement, with Envoy's UdpStatsdSink: worker threads register with the ThreadLocal instance before the main thread does, the sink is created on the main thread afterwards, and it is used from a worker thread - including for a final flush issued after tls_.shutdownGlobalThreading(), which is the order ProcessImpl::shutdown() uses, since per thread data survives until that thread calls shutdownThread(). The precondition is narrow and Nighthawk's runtime already satisfies it: every thread touching such a sink must be registered, which worker threads do in WorkerImpl's constructor and the main thread does in ProcessImpl. An unregistered thread trips ASSERT(currentThreadRegisteredWorker(index)) in SlotImpl::getWorker(), which is what a unit test constructing such a sink without registering threads would hit. Signed-off-by: Bruno Palermo --- test/BUILD | 16 +++ test/envoy_stats_sink_threading_test.cc | 128 ++++++++++++++++++++++++ 2 files changed, 144 insertions(+) create mode 100644 test/envoy_stats_sink_threading_test.cc diff --git a/test/BUILD b/test/BUILD index 4e9ed3e90..65a53d412 100644 --- a/test/BUILD +++ b/test/BUILD @@ -258,6 +258,22 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "envoy_stats_sink_threading_test", + srcs = ["envoy_stats_sink_threading_test.cc"], + repository = "@envoy", + deps = [ + "//source/common:nighthawk_common_lib", + "@envoy//source/common/network:utility_lib_with_external_headers", + "@envoy//source/common/stats:allocator_lib_with_external_headers", + "@envoy//source/common/stats:thread_local_store_lib_with_external_headers", + "@envoy//source/common/thread_local:thread_local_lib_with_external_headers", + "@envoy//source/extensions/stat_sinks/common/statsd:statsd_lib_with_external_headers", + "@envoy//test/mocks/event:event_mocks", + "@envoy//test/test_common:environment_lib", + ], +) + envoy_cc_test( name = "statistic_test", srcs = ["statistic_test.cc"], diff --git a/test/envoy_stats_sink_threading_test.cc b/test/envoy_stats_sink_threading_test.cc new file mode 100644 index 000000000..208c345e0 --- /dev/null +++ b/test/envoy_stats_sink_threading_test.cc @@ -0,0 +1,128 @@ +// Guards the threading contract Nighthawk has to satisfy to host Envoy's own stats sinks, which +// resolve a thread local writer on every use. Both cases mirror ProcessImpl: worker threads +// register before the main thread does, the sink is created on the main thread afterwards, and it +// is then used from a worker thread - including for a final flush after global thread local +// threading has been shut down, which is the order ProcessImpl::shutdown() uses. +// +// The precondition is that every thread touching such a sink is registered with the ThreadLocal +// instance; an unregistered thread trips ASSERT(currentThreadRegisteredWorker(index)) in +// SlotImpl::getWorker(). Nighthawk's worker threads register in WorkerImpl's constructor and the +// main thread registers in ProcessImpl, so the runtime satisfies it; a unit test constructing such +// a sink without registering threads would not. +#include + +#include "absl/synchronization/notification.h" + +#include "external/envoy/source/common/event/dispatcher_impl.h" +#include "external/envoy/source/common/network/utility.h" +#include "external/envoy/source/common/stats/allocator_impl.h" +#include "external/envoy/source/common/stats/thread_local_store.h" +#include "external/envoy/source/common/thread_local/thread_local_impl.h" +#include "external/envoy/source/extensions/stat_sinks/common/statsd/statsd.h" +#include "external/envoy/test/mocks/event/mocks.h" +#include "external/envoy/test/test_common/utility.h" + +#include "gtest/gtest.h" + +namespace Nighthawk { + +using namespace testing; + +TEST(EnvoyStatsSinkThreading, UdpStatsdSinkUnderAWorkerThread) { + Envoy::Api::ApiPtr api = Envoy::Api::createApiForTest(); + Envoy::ThreadLocal::InstanceImpl tls; + Envoy::Event::DispatcherPtr main_dispatcher = api->allocateDispatcher("main_thread"); + + // A worker registers before the main thread does, as ClientWorkerImpl does via createWorkers(). + Envoy::Event::DispatcherPtr worker_dispatcher = api->allocateDispatcher("worker_thread"); + tls.registerThread(*worker_dispatcher, false); + tls.registerThread(*main_dispatcher, true); + + Envoy::Stats::SymbolTableImpl symbol_table; + Envoy::Stats::AllocatorImpl allocator(symbol_table); + Envoy::Stats::ThreadLocalStoreImpl store(allocator); + store.initializeThreading(*main_dispatcher, tls); + + // Sink creation happens on the main thread, after both registrations, as in setupStatsSinks(). + Envoy::Network::Address::InstanceConstSharedPtr address = + Envoy::Network::Utility::parseInternetAddressNoThrow("127.0.0.1", 8125); + ASSERT_NE(nullptr, address); + auto sink = std::make_unique( + tls, address, /*use_tag=*/true, "nighthawk"); + store.addSink(*sink); + + // The worker thread runs its dispatcher once (WorkerImpl::start does RunType::NonBlock) and then + // uses the sink, which is where the thread local writer is resolved. + std::string failure; + std::thread worker([&]() { + worker_dispatcher->run(Envoy::Event::Dispatcher::RunType::NonBlock); + Envoy::Stats::Histogram& histogram = store.rootScope()->histogramFromString( + "test_histogram", Envoy::Stats::Histogram::Unit::Unspecified); + histogram.recordValue(1500); + // recordValue() reaches the sink through deliverHistogramToSinks(), which is where + // UdpStatsdSink resolves its thread local writer - the same resolution flush() performs. + }); + worker.join(); + + main_dispatcher->run(Envoy::Event::Dispatcher::RunType::NonBlock); + tls.shutdownGlobalThreading(); + store.shutdownThreading(); + sink.reset(); + tls.shutdownThread(); + EXPECT_EQ("", failure); +} + +// ProcessImpl::shutdown() calls tls_.shutdownGlobalThreading() before flush_worker_->shutdown(), +// and FlushWorkerImpl::shutdownThread() performs a final flush. This exercises that order: a sink +// used from a worker thread after global thread local threading has been shut down. +TEST(EnvoyStatsSinkThreading, SinkUsedAfterGlobalThreadingShutdown) { + Envoy::Api::ApiPtr api = Envoy::Api::createApiForTest(); + Envoy::ThreadLocal::InstanceImpl tls; + Envoy::Event::DispatcherPtr main_dispatcher = api->allocateDispatcher("main_thread"); + Envoy::Event::DispatcherPtr worker_dispatcher = api->allocateDispatcher("worker_thread"); + tls.registerThread(*worker_dispatcher, false); + tls.registerThread(*main_dispatcher, true); + + Envoy::Stats::SymbolTableImpl symbol_table; + Envoy::Stats::AllocatorImpl allocator(symbol_table); + Envoy::Stats::ThreadLocalStoreImpl store(allocator); + store.initializeThreading(*main_dispatcher, tls); + + Envoy::Network::Address::InstanceConstSharedPtr address = + Envoy::Network::Utility::parseInternetAddressNoThrow("127.0.0.1", 8125); + ASSERT_NE(nullptr, address); + auto sink = std::make_unique( + tls, address, /*use_tag=*/true, "nighthawk"); + store.addSink(*sink); + + Envoy::Stats::Histogram& histogram = store.rootScope()->histogramFromString( + "test_histogram", Envoy::Stats::Histogram::Unit::Unspecified); + + // One long lived worker thread, as FlushWorkerImpl is: it runs its dispatcher (picking up the + // thread local initializers), then waits, then performs a final flush on the same thread after + // global threading has been shut down. + absl::Notification registered; + absl::Notification globals_shut_down; + std::thread worker([&]() { + worker_dispatcher->run(Envoy::Event::Dispatcher::RunType::NonBlock); + histogram.recordValue(1500); + registered.Notify(); + globals_shut_down.WaitForNotification(); + // FlushWorkerImpl::shutdownThread() flushes here, after ProcessImpl shut global threading down. + histogram.recordValue(2500); + tls.shutdownThread(); + }); + registered.WaitForNotification(); + main_dispatcher->run(Envoy::Event::Dispatcher::RunType::NonBlock); + + // The shutdown order ProcessImpl uses today. + tls.shutdownGlobalThreading(); + globals_shut_down.Notify(); + worker.join(); + + store.shutdownThreading(); + sink.reset(); + tls.shutdownThread(); +} + +} // namespace Nighthawk From 1b7d4c9d446c441a3d6ec905189a99d727788b25 Mon Sep 17 00:00:00 2001 From: Bruno Palermo Date: Sat, 19 Sep 2026 16:03:16 -0300 Subject: [PATCH 3/6] client: add --envoy-stats-sinks for Envoy stats sink plugins Nighthawk's --stats-sinks resolves sinks through its own NighthawkStatsSinkFactory, so using an Envoy stats sink means writing a Nighthawk specific factory for it first. --envoy-stats-sinks resolves Envoy::Server::Configuration::StatsSinkFactory from Envoy's own registry instead, so any Envoy sink linked into the binary can be configured directly. The two flags can be combined and both lists are flushed by the same flush worker. The existing NighthawkServerFactoryContext supplies what the sinks need, and Envoy's UDP statsd and DogStatsD sinks are linked in. Verified end to end against a UDP listener: a run reports its counters, gauges and, since Nighthawk's statistics are recorded into a store histogram, its latency statistics as well. DogStatsD additionally carries the worker as a tag (envoy.cluster_name:0), because tag extraction now applies to those histograms. The flush worker owns the sinks that the stats store holds references to through addSink(), so it must be created whenever a sink of either kind is configured. Gating that on --stats-sinks alone left the sinks destroyed at the end of the setup scope while the store kept calling into them, which segfaulted in deliverHistogramToSinks() on the first connection event. The shutdown path is gated on the flush worker existing rather than on --stats-sinks for the same reason. Sinks that hold a gRPC client are deliberately not linked. Envoy binds such a client to the dispatcher of the thread that creates the sink, which here is the main thread, while Nighthawk flushes from its own flush worker thread; flushing one trips ASSERT(isThreadSafe()) in GoogleAsyncClientImpl::sendRaw. Envoy itself flushes stats on the main dispatcher, so the assumption holds there and not here. Linking the OpenTelemetry sink would therefore advertise support that aborts, and in an optimised build, where the assert compiles out, would be silently thread unsafe instead. Signed-off-by: Bruno Palermo --- README.md | 13 +++++++++ api/client/options.proto | 6 ++++ docs/root/version_history.md | 1 + extensions_build_config.bzl | 2 ++ include/nighthawk/client/options.h | 5 ++++ source/client/options_impl.cc | 33 ++++++++++++++++++++-- source/client/options_impl.h | 4 +++ source/client/process_impl.cc | 30 +++++++++++++++++--- test/mocks/client/mock_options.h | 2 ++ test/options_test.cc | 45 ++++++++++++++++++++++++++++-- 10 files changed, 133 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index c3c38f51b..5eef96d10 100644 --- a/README.md +++ b/README.md @@ -166,6 +166,7 @@ bazel-bin/nighthawk_client [--user-defined-plugin-config ] ... [--latency-response-header-name ] [--stats-flush-interval-duration ] [--stats-flush-interval ] +[--envoy-stats-sinks ] ... [--stats-sinks ] ... [--no-duration] [--simple-warmup] [--rate-limiter-plugin-config ] @@ -238,6 +239,18 @@ For example '1s' or '1.000000001s'. Mutually exclusive with Time interval (in seconds) between flushes to configured stats sinks. Mutually exclusive with --stats-flush-interval-duration. Default: 5. +--envoy-stats-sinks (accepted multiple times) +Stats sinks (in json) implemented as Envoy stats sink plugins, +resolved through Envoy's own factory registry rather than Nighthawk's, +so any Envoy sink linked into this binary can be used without a +Nighthawk specific factory. This argument is intended to be specified +multiple times, and can be combined with --stats-sinks. Sinks that +hold a gRPC client are not usable yet, see the version history. +Example (json): {name:"envoy.stat_sinks.dog_statsd" +,typed_config:{"@type":"type.googleapis.com/envoy.config.metrics.v3.Do +gStatsdSink",address:{socket_address:{address:"127.0.0.1" +,port_value:8125}}}} + --stats-sinks (accepted multiple times) Stats sinks (in json) where Nighthawk metrics will be flushed. This argument is intended to be specified multiple times. Example (json): diff --git a/api/client/options.proto b/api/client/options.proto index ea36aeb51..e7990b6f0 100644 --- a/api/client/options.proto +++ b/api/client/options.proto @@ -308,6 +308,12 @@ message CommandLineOptions { // Optional set of stat sinks where Nighthawk metrics will be flushed to. repeated envoy.config.metrics.v3.StatsSink stats_sinks = 34; + // Stats sinks implemented as Envoy stats sink plugins, resolved through + // envoy.server.configuration.StatsSinkFactory rather than Nighthawk's own + // NighthawkStatsSinkFactory, so that any Envoy sink linked into the binary can be used without + // a Nighthawk specific factory. Sinks configured here and in stats_sinks are both flushed. + repeated envoy.config.metrics.v3.StatsSink envoy_stats_sinks = 124; + oneof oneof_stats_flush_interval { // Time interval (number of seconds) between periodical flushes to // configured stats sinks. If not specified the default is 5 seconds. diff --git a/docs/root/version_history.md b/docs/root/version_history.md index 7f0ebd575..63507bbd8 100644 --- a/docs/root/version_history.md +++ b/docs/root/version_history.md @@ -14,6 +14,7 @@ Version history ### Changelist +- `--envoy-stats-sinks` configures stats sinks implemented as Envoy stats sink plugins, resolved through `Envoy::Server::Configuration::StatsSinkFactory` rather than Nighthawk's own `NighthawkStatsSinkFactory`, so any Envoy sink linked into the binary can be used without writing a Nighthawk specific factory. It can be combined with `--stats-sinks`. Envoy's UDP statsd and DogStatsD sinks are linked in. Sinks that hold a gRPC client, such as the OpenTelemetry and metrics service sinks, are not usable yet: Envoy binds the client to the dispatcher of the thread that creates the sink, while Nighthawk flushes stats from its own flush worker thread, so flushing one trips `ASSERT(isThreadSafe())`. - Nighthawk's latency statistics are now recorded into an Envoy store histogram as well as their own HdrHistogram/Circllhist, so they appear in `MetricSnapshot::histograms()`. Stats sinks that only read the snapshot on flush, such as the OpenTelemetry and metrics service sinks, previously received nothing for them: Nighthawk delivered samples exclusively through `deliverHistogramToSinks()`, which those sinks implement as a no-op. Sinks reading `onHistogramComplete()` are unaffected, and the emitted metric names are unchanged, since the mirror is created in the worker's `cluster..` scope. Nighthawk's own output still comes from the statistic itself and keeps its nanosecond resolution. - `--request-body-file` sends a file's bytes verbatim as the request body (binary safe; no Content-Type is set). The `RequestOptions.request_body` (bytes) field carries it over the gRPC service API. Mutually exclusive with `--request-body-size`. - `OptionsImpl::toCommandLineOptions()` now always emits `request_options.request_body_size`; it was only set when at least one `--request-header` was configured, so the size was lost on the gRPC service path otherwise. diff --git a/extensions_build_config.bzl b/extensions_build_config.bzl index de2edbc82..215e1ae82 100644 --- a/extensions_build_config.bzl +++ b/extensions_build_config.bzl @@ -7,6 +7,8 @@ EXTENSIONS = { "envoy.tracers.zipkin": "//source/extensions/tracers/zipkin:config", "envoy.transport_sockets.raw_buffer": "//source/extensions/transport_sockets/raw_buffer:config", "envoy.access_loggers.file": "//source/extensions/access_loggers/file:config", + "envoy.stat_sinks.statsd": "//source/extensions/stat_sinks/statsd:config", + "envoy.stat_sinks.dog_statsd": "//source/extensions/stat_sinks/dog_statsd:config", "envoy.clusters.dns": "//source/extensions/clusters/dns:dns_cluster_lib", "envoy.clusters.eds": "//source/extensions/clusters/eds:eds_lib", "envoy.clusters.static": "//source/extensions/clusters/static:static_cluster_lib", diff --git a/include/nighthawk/client/options.h b/include/nighthawk/client/options.h index 16ea17577..05fe47f15 100644 --- a/include/nighthawk/client/options.h +++ b/include/nighthawk/client/options.h @@ -104,6 +104,11 @@ class Options { virtual bool simpleWarmup() const PURE; virtual bool noDuration() const PURE; virtual std::vector statsSinks() const PURE; + /** + * @return std::vector sinks to be created through Envoy's + * own Server::Configuration::StatsSinkFactory. + */ + virtual std::vector envoyStatsSinks() const PURE; virtual uint32_t statsFlushInterval() const PURE; virtual Envoy::Protobuf::Duration statsFlushIntervalDuration() const PURE; virtual std::string responseHeaderWithLatencyInput() const PURE; diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index c29c973be..90fef9dbb 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -401,6 +401,17 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { "googleapis.com/" "envoy.config.metrics.v3.StatsdSink\",tcp_cluster_name:\"statsd\"}}", false, "string", cmd); + TCLAP::MultiArg envoy_stats_sinks( + "", "envoy-stats-sinks", + "Stats sinks (in json) implemented as Envoy stats sink plugins, resolved through Envoy's own " + "factory registry rather than Nighthawk's, so any Envoy sink linked into this binary can be " + "used without a Nighthawk specific factory. This argument is intended to be specified " + "multiple times, and can be combined with --stats-sinks. Sinks that hold a gRPC client are " + "not usable yet, see the version history. Example (json): " + "{name:\"envoy.stat_sinks.dog_statsd\",typed_config:{\"@type\":\"type.googleapis.com/" + "envoy.config.metrics.v3.DogStatsdSink\",address:{socket_address:{address:\"127.0.0.1\"," + "port_value:8125}}}}", + false, "string", cmd); TCLAP::ValueArg stats_flush_interval( "", "stats-flush-interval", @@ -460,10 +471,10 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { // Verify that if --stats-flush-interval or --stats-flush-interval-duration is // set, then --stats-sinks must also be set. if ((stats_flush_interval.isSet() || stats_flush_interval_duration.isSet()) && - !stats_sinks.isSet()) { + !stats_sinks.isSet() && !envoy_stats_sinks.isSet()) { throw MalformedArgvException( "if --stats-flush-interval or --stats-flush-interval-duration is set, " - "then --stats-sinks must also be set"); + "then --stats-sinks or --envoy-stats-sinks must also be set"); } TCLAP_SET_IF_SPECIFIED(requests_per_second, requests_per_second_); @@ -622,6 +633,18 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) { stats_sinks_.push_back(sink); } } + if (envoy_stats_sinks.isSet()) { + for (const std::string& stats_sink : envoy_stats_sinks.getValue()) { + envoy::config::metrics::v3::StatsSink sink; + try { + Envoy::MessageUtil::loadFromJson(stats_sink, sink, + Envoy::ProtobufMessage::getStrictValidationVisitor()); + } catch (const Envoy::EnvoyException& e) { + throw MalformedArgvException(e.what()); + } + envoy_stats_sinks_.push_back(sink); + } + } TCLAP_SET_IF_SPECIFIED(stats_flush_interval, stats_flush_interval_); if (stats_flush_interval_duration.isSet()) { if (Envoy::Protobuf::util::TimeUtil::FromString(stats_flush_interval_duration.getValue(), @@ -978,6 +1001,9 @@ OptionsImpl::OptionsImpl(const nighthawk::client::CommandLineOptions& options) { for (const envoy::config::metrics::v3::StatsSink& stats_sink : options.stats_sinks()) { stats_sinks_.push_back(stats_sink); } + for (const envoy::config::metrics::v3::StatsSink& stats_sink : options.envoy_stats_sinks()) { + envoy_stats_sinks_.push_back(stats_sink); + } stats_flush_interval_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(options, stats_flush_interval, stats_flush_interval_); if (options.has_stats_flush_interval_duration()) { @@ -1231,6 +1257,9 @@ CommandLineOptionsPtr OptionsImpl::toCommandLineOptionsInternal() const { for (const envoy::config::metrics::v3::StatsSink& stats_sink : stats_sinks_) { *command_line_options->add_stats_sinks() = stats_sink; } + for (const envoy::config::metrics::v3::StatsSink& stats_sink : envoy_stats_sinks_) { + *command_line_options->add_envoy_stats_sinks() = stats_sink; + } if (stats_flush_interval_duration_.seconds() > 0 || stats_flush_interval_duration_.nanos() > 0) { *command_line_options->mutable_stats_flush_interval_duration() = stats_flush_interval_duration_; } else { diff --git a/source/client/options_impl.h b/source/client/options_impl.h index 6e639e7c4..422088050 100644 --- a/source/client/options_impl.h +++ b/source/client/options_impl.h @@ -116,6 +116,9 @@ class OptionsImpl : public Options, public Envoy::Logger::Loggable envoyStatsSinks() const override { + return envoy_stats_sinks_; + } std::vector statsSinks() const override { return stats_sinks_; } @@ -211,6 +214,7 @@ class OptionsImpl : public Options, public Envoy::Logger::Loggable stats_sinks_; + std::vector envoy_stats_sinks_; uint32_t stats_flush_interval_{5}; Envoy::Protobuf::Duration stats_flush_interval_duration_; std::string latency_response_header_name_; diff --git a/source/client/process_impl.cc b/source/client/process_impl.cc index 52e31934b..fa4b80c24 100644 --- a/source/client/process_impl.cc +++ b/source/client/process_impl.cc @@ -40,6 +40,7 @@ #include "external/envoy/source/common/stats/tag_producer_impl.h" #include "external/envoy/source/common/thread_local/thread_local_impl.h" #include "external/envoy/source/exe/main_common.h" +#include "external/envoy/source/server/configuration_impl.h" #include "external/envoy/source/server/null_overload_manager.h" #include "external/envoy/source/server/server.h" #include "external/envoy_api/envoy/config/core/v3/resolver.pb.h" @@ -879,6 +880,22 @@ void ProcessImpl::setupStatsSinks(const envoy::config::bootstrap::v3::Bootstrap& Envoy::Config::Utility::getAndCheckFactory(stats_sink); stats_sinks.emplace_back(factory.createStatsSink(store_root_.symbolTable())); } + // Sinks implemented as Envoy stats sink plugins. These are resolved from Envoy's own factory + // registry, so any Envoy sink linked into this binary works without a Nighthawk specific + // factory. They receive Nighthawk's statistics through the same paths Envoy's own stats take: + // MetricSnapshot::histograms() on flush, and onHistogramComplete() per sample. + for (const envoy::config::metrics::v3::StatsSink& stats_sink : options_.envoyStatsSinks()) { + ENVOY_LOG(info, "loading Envoy stats sink configuration in Nighthawk"); + auto& factory = + Envoy::Config::Utility::getAndCheckFactory( + stats_sink); + Envoy::ProtobufTypes::MessagePtr message = Envoy::Config::Utility::translateToFactoryConfig( + stats_sink, Envoy::ProtobufMessage::getStrictValidationVisitor(), factory); + absl::StatusOr sink_or_error = + factory.createStatsSink(*message, server_->serverFactoryContext()); + THROW_IF_NOT_OK_REF(sink_or_error.status()); + stats_sinks.emplace_back(std::move(sink_or_error.value())); + } for (std::unique_ptr& sink : stats_sinks) { store_root_.addSink(*sink); } @@ -969,7 +986,7 @@ bool ProcessImpl::runInternal(OutputCollector& collector, const UriPtr& tracing_ // Needs to happen as early as possible (before createWorkers()) in the instantiation to // preempt the objects that require stats. - if (!options_.statsSinks().empty()) { + if (!options_.statsSinks().empty() || !options_.envoyStatsSinks().empty()) { absl::StatusOr producer_or_error = Envoy::Stats::TagProducerImpl::createTagProducer(bootstrap_.stats_config(), envoy_options_.statsTags()); @@ -1063,7 +1080,11 @@ bool ProcessImpl::runInternal(OutputCollector& collector, const UriPtr& tracing_ ENVOY_LOG(error, bootstrap_.DebugString()); - if (!options_.statsSinks().empty()) { + // The flush worker takes ownership of the sinks, which the stats store holds references to + // via addSink(). It must therefore be created whenever any sink was configured, of either + // kind: otherwise the sinks are destroyed at the end of this scope and the next + // deliverHistogramToSinks() call dereferences freed memory. + if (!options_.statsSinks().empty() || !options_.envoyStatsSinks().empty()) { // There should be only a single live flush worker instance at any time. flush_worker_ = std::make_unique( stats_flush_interval, *api_, tls_, store_root_, stats_sinks, *cluster_manager_); @@ -1091,9 +1112,10 @@ bool ProcessImpl::runInternal(OutputCollector& collector, const UriPtr& tracing_ w->waitForCompletion(); } - if (!options_.statsSinks().empty() && flush_worker_ != nullptr) { + if (flush_worker_ != nullptr) { // Stop the running dispatcher in flush_worker_. Needs to be called after all - // client workers are complete so that all the metrics can be flushed. + // client workers are complete so that all the metrics can be flushed. Gating this on the + // flush worker existing is enough; it is created for sinks of either kind. flush_worker_->exitDispatcher(); flush_worker_->waitForCompletion(); } diff --git a/test/mocks/client/mock_options.h b/test/mocks/client/mock_options.h index d71726f6c..7071d014a 100644 --- a/test/mocks/client/mock_options.h +++ b/test/mocks/client/mock_options.h @@ -82,6 +82,8 @@ class MockOptions : public Options { MOCK_METHOD(bool, noDuration, (), (const, override)); MOCK_METHOD(std::vector, statsSinks, (), (const, override)); + MOCK_METHOD(std::vector, envoyStatsSinks, (), + (const, override)); MOCK_METHOD(uint32_t, statsFlushInterval, (), (const, override)); MOCK_METHOD(Envoy::Protobuf::Duration, statsFlushIntervalDuration, (), (const, override)); MOCK_METHOD(std::string, responseHeaderWithLatencyInput, (), (const, override)); diff --git a/test/options_test.cc b/test/options_test.cc index eea915ec9..aebb7fb08 100644 --- a/test/options_test.cc +++ b/test/options_test.cc @@ -103,7 +103,7 @@ TEST_F(OptionsImplTest, StatsSinksMustBeSetWhenStatsFlushIntervalSet) { TestUtility::createOptionsImpl(fmt::format("{} --stats-flush-interval 10", client_name_)), MalformedArgvException, "if --stats-flush-interval or --stats-flush-interval-duration is set, then --stats-sinks " - "must also be set"); + "or --envoy-stats-sinks must also be set"); } TEST_F(OptionsImplTest, StatsSinksMustBeSetWhenStatsFlushIntervalDurationSet) { @@ -111,7 +111,48 @@ TEST_F(OptionsImplTest, StatsSinksMustBeSetWhenStatsFlushIntervalDurationSet) { "{} --stats-flush-interval-duration 1.000000001s", client_name_)), MalformedArgvException, "if --stats-flush-interval or --stats-flush-interval-duration is set, " - "then --stats-sinks must also be set"); + "then --stats-sinks or --envoy-stats-sinks must also be set"); +} + +TEST_F(OptionsImplTest, EnvoyStatsSinksRoundTrip) { + // Sinks resolved through Envoy's own StatsSinkFactory are carried separately from the Nighthawk + // ones, and both may be set at once. + const std::string envoy_sink_json = + "{name:\"envoy.stat_sinks.dog_statsd\",typed_config:{\"@type\":\"type.googleapis.com/" + "envoy.config.metrics.v3.DogStatsdSink\",address:{socket_address:{address:\"127.0.0.1\"," + "port_value:8125}}}}"; + std::unique_ptr options = TestUtility::createOptionsImpl( + fmt::format("{} --envoy-stats-sinks {} {}", client_name_, envoy_sink_json, good_test_uri_)); + ASSERT_EQ(1, options->envoyStatsSinks().size()); + EXPECT_EQ("envoy.stat_sinks.dog_statsd", options->envoyStatsSinks()[0].name()); + EXPECT_EQ(0, options->statsSinks().size()); + + CommandLineOptionsPtr cmd = options->toCommandLineOptions(); + ASSERT_EQ(1, cmd->envoy_stats_sinks_size()); + EXPECT_EQ("envoy.stat_sinks.dog_statsd", cmd->envoy_stats_sinks(0).name()); + OptionsImpl round_trip(*cmd); + ASSERT_EQ(1, round_trip.envoyStatsSinks().size()); + EXPECT_EQ("envoy.stat_sinks.dog_statsd", round_trip.envoyStatsSinks()[0].name()); +} + +TEST_F(OptionsImplTest, EnvoyStatsSinksSatisfiesTheFlushIntervalRequirement) { + const std::string envoy_sink_json = + "{name:\"envoy.stat_sinks.dog_statsd\",typed_config:{\"@type\":\"type.googleapis.com/" + "envoy.config.metrics.v3.DogStatsdSink\",address:{socket_address:{address:\"127.0.0.1\"," + "port_value:8125}}}}"; + // --stats-flush-interval requires some sink, and an Envoy one counts. + std::unique_ptr options = TestUtility::createOptionsImpl( + fmt::format("{} --stats-flush-interval 10 --envoy-stats-sinks {} {}", client_name_, + envoy_sink_json, good_test_uri_)); + EXPECT_EQ(10, options->statsFlushInterval()); + EXPECT_EQ(1, options->envoyStatsSinks().size()); +} + +TEST_F(OptionsImplTest, MalformedEnvoyStatsSinkIsRejected) { + EXPECT_THROW_WITH_REGEX( + TestUtility::createOptionsImpl( + fmt::format("{} --envoy-stats-sinks {} {}", client_name_, "{not-json", good_test_uri_)), + MalformedArgvException, ""); } TEST_F(OptionsImplTest, InvalidStatsFlushIntervalDuration) { From 7298c1ad418ecf8e7c015226d79a24c95e2b81b6 Mon Sep 17 00:00:00 2001 From: Bruno Palermo Date: Sat, 26 Sep 2026 06:28:09 -0300 Subject: [PATCH 4/6] stats: declare the mirror's unit so sinks can scale it The store histogram was created with Unit::Unspecified because Nighthawk records nanoseconds and Envoy has no nanosecond unit. That is honest about the resolution and useless to every consumer: Envoy's statsd sinks label all histograms "|ms" and scale only by unit, so an unspecified nanosecond histogram is exported as milliseconds a factor of 10^6 out -- a 3.9 ms latency reported as 3,943,277 ms, or about 65 minutes. envoyproxy/envoy#47505 has since added scale_histogram_units_to_milliseconds to the statsd sinks, but it scales Microseconds and leaves Unspecified alone, so it does not reach this case. Declaring the unit does. The mirror now declares Microseconds and samples are converted into it. Conversion rounds rather than truncates: truncating biases every sample low by up to a microsecond, which is systematic rather than a wash. The precision this costs is smaller than it looks. Envoy stores histogram samples in libcircllhist, whose buckets are val x 10^exp with val in [10, 99] -- two significant decimal digits, 90 bins per decade. Above about 10 us integer microseconds are finer than the bucketing that follows, so nothing survives the histogram that the conversion removed. Below that the quantisation is real, and a sample under half a microsecond reaches the mirror as zero. Every statistic that reaches the mirror is a request latency; the response size statistics are byte counts and are deliberately not sinkable. That invariant is recorded at unit(), because declaring a byte count as Microseconds and dividing it by a thousand would corrupt it silently and no test here would notice. Nighthawk's own output is unaffected: it is rendered from the HdrHistogram or Circllhist data, which keeps nanoseconds. Two existing tests change with the unit. SimpleSinkableStatistic asserted the sample arrives at the sink unconverted, and now uses a nanosecond value that converts exactly; its min and max move to Helper::expectNear because HdrHistogram quantises 123000 to 123003, which is the statistic's own precision and not the conversion. A new test asserts what reaches a sink rather than what the store merges, since the merged statistics would additionally be rounded by the bucketing. This is necessary but not sufficient on its own: until the Envoy dependency advances past #47505 and a user sets the opt-in field, statsd output is wrong by 10^3 rather than 10^6. It is the half Nighthawk controls. Signed-off-by: Bruno Palermo --- docs/root/version_history.md | 2 +- source/common/statistic_impl.cc | 14 ++++++-- source/common/statistic_impl.h | 39 +++++++++++++++++++--- test/statistic_test.cc | 57 +++++++++++++++++++++++++++++---- 4 files changed, 96 insertions(+), 16 deletions(-) diff --git a/docs/root/version_history.md b/docs/root/version_history.md index 7f0ebd575..2c91f4828 100644 --- a/docs/root/version_history.md +++ b/docs/root/version_history.md @@ -14,7 +14,7 @@ Version history ### Changelist -- Nighthawk's latency statistics are now recorded into an Envoy store histogram as well as their own HdrHistogram/Circllhist, so they appear in `MetricSnapshot::histograms()`. Stats sinks that only read the snapshot on flush, such as the OpenTelemetry and metrics service sinks, previously received nothing for them: Nighthawk delivered samples exclusively through `deliverHistogramToSinks()`, which those sinks implement as a no-op. Sinks reading `onHistogramComplete()` are unaffected, and the emitted metric names are unchanged, since the mirror is created in the worker's `cluster..` scope. Nighthawk's own output still comes from the statistic itself and keeps its nanosecond resolution. +- Nighthawk's latency statistics are now recorded into an Envoy store histogram as well as their own HdrHistogram/Circllhist, so they appear in `MetricSnapshot::histograms()`. Stats sinks that only read the snapshot on flush, such as the OpenTelemetry and metrics service sinks, previously received nothing for them: Nighthawk delivered samples exclusively through `deliverHistogramToSinks()`, which those sinks implement as a no-op. Sinks reading `onHistogramComplete()` are unaffected, and the emitted metric names are unchanged, since the mirror is created in the worker's `cluster..` scope. The mirror declares `Microseconds` and samples are converted into it, so sinks can scale it by unit rather than knowing Nighthawk's convention; Envoy's statsd sinks label every histogram `|ms` and scale only by unit, so an unspecified nanosecond histogram would be exported a factor of 10^6 out. Nighthawk's own output still comes from the statistic itself and keeps its nanosecond resolution. - `--request-body-file` sends a file's bytes verbatim as the request body (binary safe; no Content-Type is set). The `RequestOptions.request_body` (bytes) field carries it over the gRPC service API. Mutually exclusive with `--request-body-size`. - `OptionsImpl::toCommandLineOptions()` now always emits `request_options.request_body_size`; it was only set when at least one `--request-header` was configured, so the size was lost on the gRPC service path otherwise. - The Envoy exception on the tunneling startup path is logged instead of printed to stdout, keeping stdout reserved for the formatted output. diff --git a/source/common/statistic_impl.cc b/source/common/statistic_impl.cc index 7b85ef70d..8d6bf4fd6 100644 --- a/source/common/statistic_impl.cc +++ b/source/common/statistic_impl.cc @@ -413,7 +413,7 @@ SinkableStatistic::~SinkableStatistic() { } Envoy::Stats::Histogram::Unit SinkableStatistic::unit() const { - return Envoy::Stats::Histogram::Unit::Unspecified; + return Envoy::Stats::Histogram::Unit::Microseconds; } Envoy::Stats::SymbolTable& SinkableStatistic::symbolTable() { return scope_.symbolTable(); } @@ -431,6 +431,14 @@ void SinkableStatistic::bindStoreHistogram() { store_histogram_ = &scope_.histogramFromString(name(), unit()); } +uint64_t SinkableStatistic::toStoreHistogramUnit(uint64_t nanoseconds) { + // Nighthawk records nanoseconds; the store histogram declares Microseconds. See unit(). + // Rounded rather than truncated: truncation biases every sample low by up to a microsecond, + // which is a systematic error rather than a wash. Samples below half a microsecond still land + // in the mirror as zero -- see unit() for why that boundary is acceptable here. + return (nanoseconds + 500) / 1000; +} + SinkableHdrStatistic::SinkableHdrStatistic(Envoy::Stats::Scope& scope, std::optional worker_id) : SinkableStatistic(scope, worker_id) {} @@ -440,7 +448,7 @@ void SinkableHdrStatistic::recordValue(uint64_t value) { // onHistogramComplete() and, unlike a direct deliverHistogramToSinks() call, also makes the // statistic visible to sinks that only read MetricSnapshot::histograms() on flush, such as the // OpenTelemetry and metrics service sinks. See SinkableStatistic::storeHistogram(). - storeHistogram().recordValue(value); + storeHistogram().recordValue(toStoreHistogramUnit(value)); } void SinkableHdrStatistic::setId(absl::string_view id) { @@ -463,7 +471,7 @@ SinkableCircllhistStatistic::SinkableCircllhistStatistic(Envoy::Stats::Scope& sc void SinkableCircllhistStatistic::recordValue(uint64_t value) { CircllhistStatistic::addValue(value); // See the comment in SinkableHdrStatistic::recordValue(). - storeHistogram().recordValue(value); + storeHistogram().recordValue(toStoreHistogramUnit(value)); } void SinkableCircllhistStatistic::setId(absl::string_view id) { diff --git a/source/common/statistic_impl.h b/source/common/statistic_impl.h index a546f4f02..f40fb1465 100644 --- a/source/common/statistic_impl.h +++ b/source/common/statistic_impl.h @@ -200,9 +200,29 @@ class SinkableStatistic : public Envoy::Stats::HistogramImplHelper { SinkableStatistic(Envoy::Stats::Scope& scope, std::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 is {Unspecified, Bytes, Microseconds, Milliseconds, Percent}; + // there is no nanosecond unit, which is what Nighthawk::Statistic records. Microseconds is + // returned, and samples are divided on the way into the store histogram, so the mirror is + // self-describing: a sink can scale it by unit instead of knowing Nighthawk's convention. + // Unspecified would be honest about the resolution and useless to every sink -- Envoy's statsd + // sinks label histograms "|ms" and scale only by unit, so an unspecified nanosecond histogram is + // exported as milliseconds a factor of 10^6 out. + // + // The cost is bounded and smaller than it looks: Envoy stores histogram samples in libcircllhist, + // whose buckets are val x 10^exp with val in [10, 99] -- two significant decimal digits, 90 bins + // per decade. Above about 10 us, integer microseconds are finer than the bucketing that follows, + // so nothing survives the histogram that the conversion removed. Below that the quantisation is + // real: single-digit microseconds mean steps of 10 to 25 percent, and a sample under half a + // microsecond reaches the mirror as zero. Sub-10 us latencies are the case to revisit if a + // nanosecond unit ever exists upstream. None of this affects what Nighthawk reports: its output + // is rendered from the HdrHistogram or Circllhist data, which keeps nanoseconds. + // + // This is a property of the class, and it is only correct because every SinkableStatistic is a + // latency. The response size statistics are byte counts, not durations, and are deliberately + // not sinkable -- BenchmarkClientStatistic is handed StreamingStatistic for those two. If they + // or anything else non-temporal ever become sinkable, this has to become per statistic: + // declaring a byte count as Microseconds and dividing it by a thousand turns a 10 byte body + // into 0 and reports it as a duration, and nothing here would fail to make that visible. 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 @@ -222,8 +242,8 @@ class SinkableStatistic : public Envoy::Stats::HistogramImplHelper { * histogram is named "cluster.." and stats sinks can recover the worker from the name. * * Nighthawk's own output is unaffected: it is rendered from this statistic's HdrHistogram or - * Circllhist data, not from the mirror, so the mirror is free to carry a coarser representation - * than the nanoseconds recorded here. + * Circllhist data, not from the mirror, which is why the mirror can carry the coarser + * representation that unit() describes rather than the nanoseconds recorded here. */ Envoy::Stats::Histogram& storeHistogram(); @@ -234,6 +254,15 @@ class SinkableStatistic : public Envoy::Stats::HistogramImplHelper { */ void bindStoreHistogram(); + /** + * Converts a sample from the nanoseconds Nighthawk records to the microseconds the store + * histogram declares. See unit() for why the mirror is not recorded in nanoseconds. + * + * @param nanoseconds the sample as recorded by Nighthawk. + * @return uint64_t the same sample in whole microseconds. + */ + static uint64_t toStoreHistogramUnit(uint64_t nanoseconds); + // This is used in child class for delivering the histogram data to sinks. Envoy::Stats::Scope& scope_; diff --git a/test/statistic_test.cc b/test/statistic_test.cc index f810de291..3a61f1f19 100644 --- a/test/statistic_test.cc +++ b/test/statistic_test.cc @@ -444,7 +444,7 @@ TYPED_TEST(SinkableStatisticTest, EmptySinkableStatistic) { 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_EQ(Envoy::Stats::Histogram::Unit::Microseconds, stat.unit()); EXPECT_FALSE(stat.used()); EXPECT_EQ("", stat.name()); EXPECT_EQ("", stat.tagExtractedName()); @@ -455,21 +455,27 @@ TYPED_TEST(SinkableStatisticTest, SimpleSinkableStatistic) { Envoy::Stats::MockIsolatedStatsStore mock_store; const int worker_id = 0; TypeParam stat(*mock_store.rootScope(), worker_id); - const uint64_t sample_value = 123; + // A nanosecond sample, as Nighthawk records them, chosen to convert exactly: the store + // histogram declares Microseconds and is fed converted samples. See SinkableStatistic::unit(). + const uint64_t sample_value = 123000; + const uint64_t sample_value_micros = 123; const std::string stat_name = "stat_name"; - EXPECT_CALL(mock_store, deliverHistogramToSinks(_, sample_value)).Times(2); + EXPECT_CALL(mock_store, deliverHistogramToSinks(_, sample_value_micros)).Times(2); stat.recordValue(sample_value); stat.addValue(sample_value); stat.setId(stat_name); EXPECT_EQ(2, stat.count()); - Helper::expectNear(123.0, stat.mean(), stat.significantDigits()); + // Nighthawk's own view keeps the nanoseconds it was given; only the mirror is converted. + Helper::expectNear(123000.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()); + // Near, not equal: HdrHistogram stores to a significant-digit precision, so it returns 123003 + // for this sample. That is the statistic's own quantisation and unrelated to the mirror. + Helper::expectNear(123000.0, stat.min(), stat.significantDigits()); + Helper::expectNear(123000.0, stat.max(), stat.significantDigits()); + EXPECT_EQ(Envoy::Stats::Histogram::Unit::Microseconds, stat.unit()); EXPECT_TRUE(stat.used()); EXPECT_EQ(stat_name, stat.name()); EXPECT_EQ("0.stat_name", stat.tagExtractedName()); @@ -503,4 +509,41 @@ TYPED_TEST(SinkableStatisticTest, IsVisibleInTheStoreSnapshot) { EXPECT_EQ(1500, stat.max()); } +TYPED_TEST(SinkableStatisticTest, MirrorsSamplesAsMicroseconds) { + // The mirror declares Microseconds and is fed converted samples, so a sink can scale it by unit + // instead of knowing that Nighthawk records nanoseconds. Envoy's statsd sinks label every + // histogram "|ms" and scale only by unit, so an Unspecified nanosecond histogram is exported a + // factor of 10^6 out; this is what stops that. + Envoy::Stats::SymbolTableImpl symbol_table; + Envoy::Stats::AllocatorImpl allocator(symbol_table); + Envoy::Stats::ThreadLocalStoreImpl store(allocator); + + // Capture what reaches a sink, which is the thing being asserted -- the store's own merged + // statistics would additionally be rounded by libcircllhist's bucketing. + auto sink = std::make_unique>(); + std::vector> delivered; + std::vector units; + ON_CALL(*sink, onHistogramComplete(_, _)) + .WillByDefault( + [&delivered, &units](const Envoy::Stats::Histogram& histogram, uint64_t value) { + delivered.emplace_back(histogram.name(), value); + units.push_back(histogram.unit()); + }); + store.addSink(*sink); + + Envoy::Stats::ScopeSharedPtr worker_scope = store.createScope("cluster.0."); + TypeParam stat(*worker_scope, /*worker_id=*/0); + stat.setId("benchmark_http_client.latency_2xx"); + // 1.5 ms expressed the way Nighthawk records it. + stat.recordValue(1500000); + + EXPECT_THAT(delivered, Contains(std::make_pair( + std::string("cluster.0.benchmark_http_client.latency_2xx"), 1500UL))); + EXPECT_THAT(units, Contains(Envoy::Stats::Histogram::Unit::Microseconds)); + // Nighthawk still reports nanoseconds; only the mirror is coarser. Near rather than equal + // because HdrStatistic quantises to its significant digits, which is its own behaviour and not + // the conversion under test -- the conversion is asserted on the delivered sample above. + Helper::expectNear(1500000.0, stat.max(), stat.significantDigits()); +} + } // namespace Nighthawk From b3287796dd7bb80e31739355ff53d9c93315c94a Mon Sep 17 00:00:00 2001 From: Bruno Palermo Date: Sat, 26 Sep 2026 10:36:32 -0300 Subject: [PATCH 5/6] stats: correct where the mirror's precision is actually lost The unit comment claimed the conversion to microseconds costs nothing above about 10 us, because libcircllhist bins samples to two significant digits anyway. That is true for sinks reading the merged histogram statistics, and false for the one path this change exists to serve. ParentHistogramImpl::recordValue() records into the thread local circllhist and then calls deliverHistogramToSinks() with the raw value, and UdpStatsdSink::flush() iterates counters and gauges but never snapshot.histograms(). Histograms reach the statsd sinks only through onHistogramComplete(), sample by sample, exactly as recorded. Nothing bins them between here and the wire, so microseconds is the real precision floor there: a 40.7 us sample leaves as 41 us, about 1 percent at that scale and proportionally worse below, against roughly 0.02 percent for the millisecond scale request latencies Nighthawk measures. The comment now separates the two kinds of consumer instead of generalising from the binned one, and does not describe microseconds as a considered floor -- it is the floor of what Envoy's Histogram::Unit can currently express. Signed-off-by: Bruno Palermo --- docs/root/version_history.md | 2 +- source/common/statistic_impl.h | 33 +++++++++++++++++++++++++-------- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/docs/root/version_history.md b/docs/root/version_history.md index 2c91f4828..9d81a7e2a 100644 --- a/docs/root/version_history.md +++ b/docs/root/version_history.md @@ -14,7 +14,7 @@ Version history ### Changelist -- Nighthawk's latency statistics are now recorded into an Envoy store histogram as well as their own HdrHistogram/Circllhist, so they appear in `MetricSnapshot::histograms()`. Stats sinks that only read the snapshot on flush, such as the OpenTelemetry and metrics service sinks, previously received nothing for them: Nighthawk delivered samples exclusively through `deliverHistogramToSinks()`, which those sinks implement as a no-op. Sinks reading `onHistogramComplete()` are unaffected, and the emitted metric names are unchanged, since the mirror is created in the worker's `cluster..` scope. The mirror declares `Microseconds` and samples are converted into it, so sinks can scale it by unit rather than knowing Nighthawk's convention; Envoy's statsd sinks label every histogram `|ms` and scale only by unit, so an unspecified nanosecond histogram would be exported a factor of 10^6 out. Nighthawk's own output still comes from the statistic itself and keeps its nanosecond resolution. +- Nighthawk's latency statistics are now recorded into an Envoy store histogram as well as their own HdrHistogram/Circllhist, so they appear in `MetricSnapshot::histograms()`. Stats sinks that only read the snapshot on flush, such as the OpenTelemetry and metrics service sinks, previously received nothing for them: Nighthawk delivered samples exclusively through `deliverHistogramToSinks()`, which those sinks implement as a no-op. Sinks reading `onHistogramComplete()` are unaffected, and the emitted metric names are unchanged, since the mirror is created in the worker's `cluster..` scope. The mirror declares `Microseconds` and samples are converted into it, so sinks can scale it by unit rather than knowing Nighthawk's convention; Envoy's statsd sinks label every histogram `|ms` and scale only by unit, so an unspecified nanosecond histogram would be exported a factor of 10^6 out. The conversion costs sub-microsecond precision on the statsd path, which receives every sample as recorded rather than binned, until Envoy gains a nanosecond histogram unit. Nighthawk's own output still comes from the statistic itself and keeps its nanosecond resolution. - `--request-body-file` sends a file's bytes verbatim as the request body (binary safe; no Content-Type is set). The `RequestOptions.request_body` (bytes) field carries it over the gRPC service API. Mutually exclusive with `--request-body-size`. - `OptionsImpl::toCommandLineOptions()` now always emits `request_options.request_body_size`; it was only set when at least one `--request-header` was configured, so the size was lost on the gRPC service path otherwise. - The Envoy exception on the tunneling startup path is logged instead of printed to stdout, keeping stdout reserved for the formatted output. diff --git a/source/common/statistic_impl.h b/source/common/statistic_impl.h index f40fb1465..bfa098695 100644 --- a/source/common/statistic_impl.h +++ b/source/common/statistic_impl.h @@ -208,14 +208,31 @@ class SinkableStatistic : public Envoy::Stats::HistogramImplHelper { // sinks label histograms "|ms" and scale only by unit, so an unspecified nanosecond histogram is // exported as milliseconds a factor of 10^6 out. // - // The cost is bounded and smaller than it looks: Envoy stores histogram samples in libcircllhist, - // whose buckets are val x 10^exp with val in [10, 99] -- two significant decimal digits, 90 bins - // per decade. Above about 10 us, integer microseconds are finer than the bucketing that follows, - // so nothing survives the histogram that the conversion removed. Below that the quantisation is - // real: single-digit microseconds mean steps of 10 to 25 percent, and a sample under half a - // microsecond reaches the mirror as zero. Sub-10 us latencies are the case to revisit if a - // nanosecond unit ever exists upstream. None of this affects what Nighthawk reports: its output - // is rendered from the HdrHistogram or Circllhist data, which keeps nanoseconds. + // What the conversion costs depends on which kind of sink is reading, and the two differ: + // + // - Sinks that read the merged histogram statistics -- Prometheus, the admin endpoint, + // OpenTelemetry, the gRPC metrics service -- see values already binned by libcircllhist, + // whose buckets are val x 10^exp with val in [10, 99]: two significant decimal digits, 90 + // bins per decade. Above about 10 us those bins are coarser than a microsecond, so the + // conversion removes nothing that would have survived anyway. + // - Sinks implementing onHistogramComplete(), which is how the statsd sinks consume + // histograms, see every sample exactly as recorded. ParentHistogramImpl::recordValue() + // records into the thread local circllhist and then calls deliverHistogramToSinks() with + // the raw value, and UdpStatsdSink::flush() never reads snapshot.histograms(). Nothing + // bins the value between here and the wire, so on that path microseconds is the real + // precision floor: a 40.7 us sample leaves as 41 us and the remainder is gone. + // + // That is the path this change exists to serve, so the loss is not hypothetical: roughly 1 + // percent at 40 us and proportionally worse below, against about 0.02 percent for the + // millisecond scale request latencies Nighthawk actually measures. Rounding rather than + // truncating halves it. A sample under half a microsecond reaches the mirror as zero. + // + // A nanosecond unit upstream would remove the loss on that path; there is no such unit today. + // This is the floor of what Envoy can currently express, not a considered limit, and should + // not be written down as one. + // + // None of this affects what Nighthawk reports: its output is rendered from the HdrHistogram or + // Circllhist data, which keeps nanoseconds. // // This is a property of the class, and it is only correct because every SinkableStatistic is a // latency. The response size statistics are byte counts, not durations, and are deliberately From 940be26dbc33caa4d2ea548f61fef145c67dcf98 Mon Sep 17 00:00:00 2001 From: Bruno Palermo Date: Sat, 26 Sep 2026 15:39:24 -0300 Subject: [PATCH 6/6] test: drop the Envoy extension dependency from the threading test The test depended on Envoy's UdpStatsdSink, which is an envoy_cc_library inside an envoy_extension_package(). Those keep the package default visibility -- //source/extensions/... and //test/extensions/... inside Envoy -- while envoy_cc_extension publishes its :config target publicly, which is what every other Nighthawk dependency on an extension uses. Nighthawk widened that to public through the EXTENSION_PACKAGE_VISIBILITY override in its own extensions_build_config.bzl, which WORKSPACE wired in as the envoy_build_config repository. The bzlmod migration removed WORKSPACE, and nothing consumes that file now, so the override is inert and the dependency stopped resolving: Visibility error: target '@@envoy+//source/extensions/stat_sinks/common/statsd:statsd_lib_with_external_headers' is not visible from target '//test:envoy_stats_sink_threading_test' That is an analysis failure, so the test, asan and tsan jobs all aborted before running anything, while test_gcc and the benchmarks passed -- they do not build this target. The dependency was never needed. What the test pins is the thread local resolution every such sink performs on every use, which is the call that trips ASSERT(currentThreadRegisteredWorker(index)) in SlotImpl::getWorker() from an unregistered thread. A local fake that resolves a slot the same way exercises exactly that, and the two interfaces it needs -- envoy/stats:stats_interface and envoy/thread_local:thread_local_interface -- are envoy_package(), which is //visibility:public rather than extension scoped. It also makes the test honest about its subject: it was never testing statsd, only borrowing it as something with per thread state. The comment says so, so that a later change does not "improve" it back to a real sink and reintroduce the coupling. Signed-off-by: Bruno Palermo --- test/BUILD | 5 +-- test/envoy_stats_sink_threading_test.cc | 59 ++++++++++++++++++------- 2 files changed, 45 insertions(+), 19 deletions(-) diff --git a/test/BUILD b/test/BUILD index e0337caef..3b762590d 100644 --- a/test/BUILD +++ b/test/BUILD @@ -247,12 +247,11 @@ envoy_cc_test( srcs = ["envoy_stats_sink_threading_test.cc"], deps = [ "//source/common:nighthawk_common_lib", - "@envoy//source/common/network:utility_lib_with_external_headers", + "@envoy//envoy/stats:stats_interface", + "@envoy//envoy/thread_local:thread_local_interface", "@envoy//source/common/stats:allocator_lib_with_external_headers", "@envoy//source/common/stats:thread_local_store_lib_with_external_headers", "@envoy//source/common/thread_local:thread_local_lib_with_external_headers", - "@envoy//source/extensions/stat_sinks/common/statsd:statsd_lib_with_external_headers", - "@envoy//test/mocks/event:event_mocks", "@envoy//test/test_common:environment_lib", ], ) diff --git a/test/envoy_stats_sink_threading_test.cc b/test/envoy_stats_sink_threading_test.cc index 52cdf7d97..853f97b0d 100644 --- a/test/envoy_stats_sink_threading_test.cc +++ b/test/envoy_stats_sink_threading_test.cc @@ -9,26 +9,60 @@ // SlotImpl::getWorker(). Nighthawk's worker threads register in WorkerImpl's constructor and the // main thread registers in ProcessImpl, so the runtime satisfies it; a unit test constructing such // a sink without registering threads would not. +// +// The sink here is a local fake rather than Envoy's UdpStatsdSink. What is under test is the +// thread local resolution every such sink performs, not any one sink's behaviour, and the fake +// performs exactly that resolution -- getTyped() on a slot, on whichever thread calls it, which is +// the call that trips the assert. Depending on a real one would also mean depending on an Envoy +// extension library, and those are visible only inside Envoy: envoy_cc_extension publishes its +// :config target publicly, while an envoy_cc_library in an envoy_extension_package() keeps the +// package default. Nighthawk widened that through extensions_build_config.bzl when WORKSPACE wired +// it in; under bzlmod nothing consumes that file. #include #include "absl/synchronization/notification.h" +#include "envoy/stats/sink.h" +#include "envoy/thread_local/thread_local.h" + #include "source/common/event/dispatcher_impl.h" -#include "source/common/network/utility.h" #include "source/common/stats/allocator_impl.h" #include "source/common/stats/thread_local_store.h" #include "source/common/thread_local/thread_local_impl.h" -#include "source/extensions/stat_sinks/common/statsd/statsd.h" -#include "test/mocks/event/mocks.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" namespace Nighthawk { +namespace { using namespace testing; -TEST(EnvoyStatsSinkThreading, UdpStatsdSinkUnderAWorkerThread) { +// Stands in for an Envoy stats sink that keeps per thread state: it resolves a thread local slot +// on every use, which is what UdpStatsdSink does for its writer and what makes an unregistered +// thread fatal. Counting the resolutions is incidental; reaching them at all is the test. +class ThreadLocalResolvingSink : public Envoy::Stats::Sink { +public: + explicit ThreadLocalResolvingSink(Envoy::ThreadLocal::SlotAllocator& tls) + : slot_(tls.allocateSlot()) { + slot_->set([](Envoy::Event::Dispatcher&) -> Envoy::ThreadLocal::ThreadLocalObjectSharedPtr { + return std::make_shared(); + }); + } + + void flush(Envoy::Stats::MetricSnapshot&) override { ++slot_->getTyped().uses_; } + void onHistogramComplete(const Envoy::Stats::Histogram&, uint64_t) override { + ++slot_->getTyped().uses_; + } + +private: + struct Writer : public Envoy::ThreadLocal::ThreadLocalObject { + uint64_t uses_{0}; + }; + Envoy::ThreadLocal::SlotSharedPtr slot_; +}; + +TEST(EnvoyStatsSinkThreading, SinkUsedFromAWorkerThread) { Envoy::Api::ApiPtr api = Envoy::Api::createApiForTest(); Envoy::ThreadLocal::InstanceImpl tls; Envoy::Event::DispatcherPtr main_dispatcher = api->allocateDispatcher("main_thread"); @@ -44,11 +78,7 @@ TEST(EnvoyStatsSinkThreading, UdpStatsdSinkUnderAWorkerThread) { store.initializeThreading(*main_dispatcher, tls); // Sink creation happens on the main thread, after both registrations, as in setupStatsSinks(). - Envoy::Network::Address::InstanceConstSharedPtr address = - Envoy::Network::Utility::parseInternetAddressNoThrow("127.0.0.1", 8125); - ASSERT_NE(nullptr, address); - auto sink = std::make_unique( - tls, address, /*use_tag=*/true, "nighthawk"); + auto sink = std::make_unique(tls); store.addSink(*sink); // The worker thread runs its dispatcher once (WorkerImpl::start does RunType::NonBlock) and then @@ -59,8 +89,8 @@ TEST(EnvoyStatsSinkThreading, UdpStatsdSinkUnderAWorkerThread) { Envoy::Stats::Histogram& histogram = store.rootScope()->histogramFromString( "test_histogram", Envoy::Stats::Histogram::Unit::Unspecified); histogram.recordValue(1500); - // recordValue() reaches the sink through deliverHistogramToSinks(), which is where - // UdpStatsdSink resolves its thread local writer - the same resolution flush() performs. + // recordValue() reaches the sink through deliverHistogramToSinks(), which is where the sink + // resolves its thread local writer - the same resolution flush() performs. }); worker.join(); @@ -88,11 +118,7 @@ TEST(EnvoyStatsSinkThreading, SinkUsedAfterGlobalThreadingShutdown) { Envoy::Stats::ThreadLocalStoreImpl store(allocator); store.initializeThreading(*main_dispatcher, tls); - Envoy::Network::Address::InstanceConstSharedPtr address = - Envoy::Network::Utility::parseInternetAddressNoThrow("127.0.0.1", 8125); - ASSERT_NE(nullptr, address); - auto sink = std::make_unique( - tls, address, /*use_tag=*/true, "nighthawk"); + auto sink = std::make_unique(tls); store.addSink(*sink); Envoy::Stats::Histogram& histogram = store.rootScope()->histogramFromString( @@ -125,4 +151,5 @@ TEST(EnvoyStatsSinkThreading, SinkUsedAfterGlobalThreadingShutdown) { tls.shutdownThread(); } +} // namespace } // namespace Nighthawk