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 <bruno.palermo@superbid.net>
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 <bruno.palermo@superbid.net>
This was referenced Sep 17, 2026
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
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 is related to #1600
Records each Nighthawk statistic sample into an Envoy store histogram, so that Nighthawk's latency statistics appear in
MetricSnapshot::histograms().SinkableStatisticis constructed directly as aHistogramImplHelperover an emptyMetricImpland is never registered with the store, so it does not appear in a snapshot. Its only route to a sink was the directStats::Store::deliverHistogramToSinks()call inrecordValue(). Sinks that implementonHistogramComplete()as an empty override and work from the snapshot on flush therefore received nothing for them — in Envoy that ismetrics_service,open_telemetryandhystrix. Exporting a run through the OpenTelemetry sink would carry counters, gauges and Envoy's own histograms, with every Nighthawk latency metric absent and no error to indicate it.ParentHistogramImpl::recordValue()callsdeliverHistogramToSinks()itself, so recording into a store histogram replaces the direct delivery rather than adding to it: one call feeds both the snapshot and sinks that take individual samples.Notes for Reviewers
cluster.<n>.scope and named after the statistic's id, so sinks seecluster.<n>.benchmark_http_client.latency_2xxas before, and the worker remains recoverable from the name. A side effect is that Envoy's tag extractors now apply, so tag capable sinks receive the worker asenvoy.cluster_namerather than only as a name prefix.setId(), since a statistic is named after construction, and the resolved pointer is cached so recording remains a dereference.Histogram::Unit(stat_sinks: scale statsd histogram samples to milliseconds by unit envoy#47505), without changing what Nighthawk measures.NighthawkStatsSinkFactoryand theonHistogramComplete()path are untouched, andtest_stats_sinks.pypasses unchanged. One exception worth naming: a sink recovering the worker id withdynamic_cast<const SinkableStatistic*>on the delivered histogram now receives aParentHistogramand getsnullptr. The worker id remains available from the metric name or, for tag capable sinks, from the extracted tag.//test:envoy_stats_sink_threading_test, which pins the threading contract Nighthawk must satisfy to host Envoy's own stats sinks: they resolve a thread local writer on every use, and every thread touching one must be registered with theThreadLocalinstance. Nighthawk's worker threads register inWorkerImpl's constructor and the main thread inProcessImpl, including for a final flush issued aftertls_.shutdownGlobalThreading(), which is the orderProcessImpl::shutdown()uses.//test:statistic_testasserts the statistic appears instore.histograms()of aThreadLocalStoreImpl— noteIsolatedStoreImpl::histograms()always returns an empty vector and cannot show this — plus//test:envoy_stats_sink_threading_test,//test:benchmark_http_client_test,//test:client_worker_testand//test:process_test.