Skip to content

stats: record Nighthawk statistics into an Envoy store histogram - #1616

Open
bpalermo wants to merge 2 commits into
envoyproxy:mainfrom
bpalermo:up/store-histogram
Open

bpalermo wants to merge 2 commits into
envoyproxy:mainfrom
bpalermo:up/store-histogram

Conversation

@bpalermo

@bpalermo bpalermo commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

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().

SinkableStatistic is constructed directly as a HistogramImplHelper over an empty MetricImpl and is never registered with the store, so it does not appear in a snapshot. Its only route to a sink was the direct Stats::Store::deliverHistogramToSinks() call in recordValue(). Sinks that implement onHistogramComplete() as an empty override and work from the snapshot on flush therefore received nothing for them — in Envoy that is metrics_service, open_telemetry and hystrix. 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() calls deliverHistogramToSinks() 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

  • Emitted metric names are unchanged. The histogram is created in the worker's cluster.<n>. scope and named after the statistic's id, so sinks see cluster.<n>.benchmark_http_client.latency_2xx as 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 as envoy.cluster_name rather than only as a name prefix.
  • The histogram is bound in setId(), since a statistic is named after construction, and the resolved pointer is cached so recording remains a dereference.
  • Nighthawk's own output is unaffected: it is rendered from the HdrHistogram or Circllhist data and keeps nanosecond resolution. The mirror is a separate object and can carry a coarser representation later, once Envoy's statsd sinks scale by Histogram::Unit (stat_sinks: scale statsd histogram samples to milliseconds by unit envoy#47505), without changing what Nighthawk measures.
  • Compatibility: NighthawkStatsSinkFactory and the onHistogramComplete() path are untouched, and test_stats_sinks.py passes unchanged. One exception worth naming: a sink recovering the worker id with dynamic_cast<const SinkableStatistic*> on the delivered histogram now receives a ParentHistogram and gets nullptr. The worker id remains available from the metric name or, for tag capable sinks, from the extracted tag.
  • Cost: one mirrored record per request in HTTP mode, as the latency by status statistics are mutually exclusive. The added work is a cached thread local lookup and a circllhist insert, which measured below the run to run spread of the harness at 5k rps on loopback, against roughly 140 us of client CPU per request.
  • The second commit adds //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 the ThreadLocal instance. Nighthawk's worker threads register in WorkerImpl's constructor and the main thread in ProcessImpl, including for a final flush issued after tls_.shutdownGlobalThreading(), which is the order ProcessImpl::shutdown() uses.
  • Testing: //test:statistic_test asserts the statistic appears in store.histograms() of a ThreadLocalStoreImpl — note IsolatedStoreImpl::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_test and //test:process_test.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant