Conversation
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.<n>." 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 <b@palermo.dev>
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 <b@palermo.dev>
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 <b@palermo.dev>
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 <b@palermo.dev>
bpalermo
force-pushed
the
up/envoy-stats-sinks
branch
from
September 26, 2026 10:10
624ab31 to
1b7d4c9
Compare
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 <b@palermo.dev>
# Conflicts: # test/statistic_test.cc
# Conflicts: # docs/root/version_history.md # source/client/process_impl.cc
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 <b@palermo.dev>
bpalermo
added a commit
to bpalermo/nighthawk
that referenced
this pull request
Sep 27, 2026
test/process_test.cc:211 in ProcessTest.TwoProcessInSequence failed under asan on 940be26: a run against a loopback endpoint with nothing listening reported success where the test expects failure. asan itself reported nothing -- no leak, no use-after-free, no overflow. The job failed on a gtest assertion alone, and the identical code passes asan on envoyproxy#1617, which contains these commits plus its own. The same file already carries a hardcoded sleep(5) with a comment that the value was chosen empirically to survive sanitizer runs in CI, so the suite is timing sensitive under asan. This commit is empty and exists only to retest that. Signed-off-by: Bruno Palermo <b@palermo.dev>
mattklein123
pushed a commit
to envoyproxy/envoy
that referenced
this pull request
Sep 30, 2026
### Commit Message stats: add a Nanoseconds histogram unit ### Additional Description `Stats::Histogram::Unit` stops at microseconds. The motivating consumer is [Nighthawk](https://github.com/envoyproxy/nighthawk), Envoy's load generator, which records request latencies in nanoseconds and is adding export of those histograms through Envoy's own statsd and DogStatsD sinks (envoyproxy/nighthawk#1617). Today it has to either quantize to microseconds at recording time, losing sub-microsecond resolution on the loopback and same-host latencies it exists to measure, or register the histogram without a unit, in which case the statsd sinks relabel the raw nanosecond value as milliseconds and report a 3.9 ms latency as about 65 minutes. The same applies to any embedder or extension measuring sub-microsecond durations. This adds `Unit::Nanoseconds` and wires it through the three places that switch on the unit: - `HistogramCompletableTimespanImpl` records the elapsed nanoseconds for such a histogram. - The statsd, DogStatsD and Graphite statsd sinks scale nanosecond samples to milliseconds when `scale_histogram_units_to_milliseconds` (#47505) is enabled; with it disabled the sample passes through unchanged as for every other unit. - The Lua filter accepts `"nanoseconds"` as a histogram unit. The unit is opt-in by construction: nothing in Envoy creates a nanosecond histogram, so no existing metric, value or bucket boundary changes. Storage is unaffected: circllhist bins values to two significant digits, so scaling samples by 1000 shifts exponents without adding populated bins. Not included: the stats access logger's own proto unit enum, which is an API change and has no sub-microsecond source values; and unit-aware default Prometheus/OTel buckets, which would be a behavior change for existing output. The changelog notes that nanosecond histograms need explicit `histogram_bucket_settings` for those exporters. ### Risk Level Low. Additive enum value; no default behavior change. ### Testing New `ElapsedAndCompleteNanoseconds` timespan test; nanosecond samples added to the `SiSuffix` and `HistogramUnitScalingOffByDefault` tests of both statsd sinks; Lua `HistogramUnits` test extended. ### Docs Changes Lua filter docs list the new unit string. ### Release Notes Added a new feature fragment. ### Platform Specific Features N/A Signed-off-by: Bruno Palermo <b@palermo.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR builds on #1616 and supersedes the closed #1600
Adds
--envoy-stats-sinks, which configures stats sinks through Envoy's ownServer::Configuration::StatsSinkFactoryregistry.--stats-sinksresolves sinks through Nighthawk'sNighthawkStatsSinkFactory, so each Envoy sink needs a Nighthawk specific factory written for it before it can be used. With this flag, any Envoy sink linked into the binary is configurable directly, with the sameenvoy.config.metrics.v3.StatsSinkJSON.Both flags may be used together; sinks from either are registered with the stats store and flushed by the same flush worker.
Envoy's UDP
statsdanddog_statsdsinks are available to it. This PR adds them to Nighthawk'sextensions_build_config.bzl, but that is currently belt and braces rather than the reason they link: both are already in Envoy's own default extension set, and since the bzlmod migration (#1613) removedWORKSPACE, nothing wires Nighthawk's copy of that file in as theenvoy_build_configrepository any more. The entries are kept so the sinks survive if that override is ever restored — Nighthawk's curated list is 13 extensions against Envoy's 28, so a restored override without them would drop these.Nighthawk's statistics reach these sinks as normal Envoy metrics, including latency statistics, which #1616 records into store histograms. Under
dog_statsdthey carry tags produced by Envoy's tag extractors:This answers the tags question from #1600: the worker is emitted as
envoy.cluster_namewithout Nighthawk having any tag specific handling, andstats_config.stats_tagsapplies as it does in Envoy.Notes for Reviewers
CommandLineOptions.envoy_stats_sinksis field 124, skipping 121 to 123 which are taken by the fields in stats: add UDP statsd / DogStatsD stats sinks and --stats-sink-tag #1600, client: add --grpc-mode unary for gRPC load with grpc-status scoring #1602 and client: gRPC bidirectional streaming mode (--grpc-mode bidi-stream) #1603, so the numbering holds whatever order those land in.NighthawkServerFactoryContext, which already provides thethreadLocal()andmessageValidationContext()the UDP sinks require. No new context plumbing.ProcessImplcreates the flush worker, which owns the sinks, whenever either flag is set. It was previously gated on--stats-sinksalone, which with only--envoy-stats-sinksleft the sinks destroyed at the end of the setup scope while the stats store still held references fromaddSink(), faulting indeliverHistogramToSinks(). The shutdown path is keyed on the flush worker existing for the same reason.--stats-flush-intervaland--stats-flush-interval-durationaccept either flag as the required sink.Limitations
Latencies are reported in the wrong unit by the statsd sinks, and the values above show it. Envoy's
UdpStatsdSink::onHistogramCompletelabels every histogram|msand scales only byHistogram::Unit, so a histogram carrying nanoseconds is exported a factor of 10^6 out.Two things have moved since this was written. stat_sinks: scale statsd histogram samples to milliseconds by unit envoy#47505 has landed, adding
scale_histogram_units_to_milliseconds— but it scalesMicrosecondsand leavesUnspecifiedalone, so it does not reach a nanosecond histogram on its own. stats: record Nighthawk statistics into an Envoy store histogram #1616 supplies the other half: the store histogram now declaresMicrosecondsand converts samples into it, which is what makes that scaling apply. This branch does not carry that commit yet and picks it up when stats: record Nighthawk statistics into an Envoy store histogram #1616 merges; the numbers above were captured before it.With both halves the output is correct. With stats: record Nighthawk statistics into an Envoy store histogram #1616 alone it is out by 10^3 rather than 10^6, which is where this lands until Nighthawk's Envoy dependency advances past #47505 and the opt-in field is set. stats: add UDP statsd / DogStatsD stats sinks and --stats-sink-tag #1600 is closed, so it is no longer an alternative for correct units.
Sinks that hold a gRPC client, such as
open_telemetryandmetrics_service, are not linked. Envoy binds the client to the dispatcher of the thread constructing the sink, which here is the main thread; Nighthawk flushes fromFlushWorkerImpl's own thread, so flushing tripsASSERT(isThreadSafe())inGoogleAsyncClientImpl::sendRaw. Envoy flushes stats on the main dispatcher, so this does not arise there. UnderNDEBUGthe assert compiles out and the call is simply not thread safe, so linking them would be worse than omitting them.Constructing Envoy sinks on the flush worker's dispatcher instead of the main one would bind the client to the thread that flushes it. That changes the ordering of sink setup against worker creation, so it is left out of this PR; happy to take it as a follow-up if the approach looks right.
Testing:
//test:options_testcovers the round trip throughCommandLineOptions, the two sink lists remaining separate,--envoy-stats-sinkssatisfying the flush interval requirement, and rejection of malformed JSON. Behaviour above was confirmed against a UDP listener receiving a run's flushes. README usage regenerated; version history updated.