stats: add UDP statsd / DogStatsD stats sinks and --stats-sink-tag

[bpalermo]

Description

This PR is related to #1607

--stats-sinks is documented in the README with a statsd example, but no NighthawkStatsSinkFactory implementation is linked into the binaries, so any --stats-sinks value aborts at startup with Didn't find a registered implementation for 'envoy.stat_sinks.statsd'. This adds a UDP statsd sink and registers it under Envoy's sink names so the documented configs work as written.

Behavior:

• envoy.stat_sinks.statsd (envoy.config.metrics.v3.StatsdSink, UDP address form, IP literal or host name resolved at startup; tcp_cluster_name is rejected) and envoy.stat_sinks.dog_statsd (envoy.config.metrics.v3.DogStatsdSink: DogStatsD tags, optional max_bytes_per_datagram batching).
• Counters are sent as deltas (|c) on every flush, gauges as values (|g). Nighthawk's sinkable latency statistics record nanoseconds, so every sample is sent as a millisecond timing with microsecond precision (|ms); Envoy histograms are converted per their unit.
• Per-worker metrics are named worker.<n>.<rest> (covering both the cluster.<n>. and worker.<n>. store scopes), or carry a worker:<n> tag with DogStatsD.
• New --stats-sink-tag key:value (repeatable; CommandLineOptions.stats_sink_tags) adds tags to every message of tag-capable sinks, e.g. to identify a run or pod.
• Default metric prefix nighthawk (override with prefix).

Notes for Reviewers

• Envoy's UdpStatsdSink was not reusable as-is: it keeps a thread-local writer, and Nighthawk's flush worker thread registers with TLS after sinks are created, which trips Envoy's currentThreadRegisteredWorker assert; it also forwards histogram values verbatim as |ms, wrong for Nighthawk's nanosecond statistics. The sink here shares one UDP socket across threads and batches latency samples per recording thread (sent when the batch fills, on the next flush, and at shutdown).
• NighthawkStatsSinkFactory::createStatsSink now receives the translated typed config (it previously had no access to it), a ThreadLocal::SlotAllocator and the configured tags; include/ is documented as not a public API, and the only in-tree implementation (the test fake) is updated.
• Testing: //test:statsd_sink_test (naming with/without tags, ns to ms conversion, counter deltas and gauges, batching and end-of-run flush against a real loopback UDP receiver, factory registration, host name resolution and config validation); test/integration/test_stats_sinks.py (full nighthawk_client runs with each sink, capturing the datagrams); //test:options_test, //test:process_test, //test:factories_test. README usage regenerated; docs/root/statistics.md and version history updated.
• Verified end to end against an OpenTelemetry Collector statsd receiver: per-worker counts in Prometheus matched the client's JSON exactly.

[eric846]

What do you think of the following proposal?

Could Nighthawk be enhanced to directly support Envoy stats sink plugin factories? I think this would help us reuse existing sinks with less effort.

The existing --stats-sinks support uses Nighthawk-specific NighthawkStatsSinkFactory factories, which then create standard Envoy Envoy::Stats::Sink objects.

My proposal is for Nighthawk to directly support Envoy::Server::Configuration::StatsSinkFactory. Then anytime someone wanted to start using a particular Envoy sink plugin, they could just link it into their Nighthawk binary without hvaing to write a NighthawkStatsSinkFactory for it.

If there's an existing Envoy UDP statsd sink, we coulde use it instead of creating a new one just for Nighthawk. We could just link any official Envoy sink into the official Nighthawk binary, also including the DataDog one.

The cleanest way to introduce support for Envoy::Server::Configuration::StatsSinkFactory might be to introduce a new Nighthawk plugin type under a new command line flag --envoy-stats-sinks, and mark --stats-sinks as deprecated.

Envoy::Server::Configuration::StatsSinkFactory has the method virtual absl::StatusOr<Stats::SinkPtr> createStatsSink(const Protobuf::Message& config, Server::Configuration::ServerFactoryContext& server).

Fortunately Nighthawk implements NighthawkServerFactoryContext and NighthawkServerInstance which includes thread-local storage.

About tags: If we were using an Envoy sink plugin that already supported tags, could we just configure the tags in the sink plugin config proto as processed by the Envoy::Server::Configuration::StatsSinkFactory? Then we wouldn't need Nighthawk to have any special awareness of tags.

[eric846]

I just saw your notes about the UdpStatsdSink

• About the thread-local writer:
  • In your new statsd sink here, how do you avoid the issue? Do you just not keep a thread-local writer?
  • Any chance we could change either when Nighthawk creates the sinks or when it creates the flush worker thread?
• About the ms assumption:
  • That is really frustrating!
  • If we tried to use Envoy's official DataDog sink plugin instead of your Nighthawk-specific DogStatsdSinkFactory, we would again have the ms problem because it instantiates Envoy's UdpStatsdSink: https://github.com/envoyproxy/envoy/blob/0d3b190cdb825bd40652f6d9599b20eecd94a974/source/extensions/stat_sinks/dog_statsd/config.cc#L32
  • I'm wondering if we should add a flag to Nighthawk that tells it to convert nanoseconds to milliseconds before writing them to any stats sink, if Envoy statsd sinks have a universal assumption of ms. Or this ms conversion could be implied by the use of --envoy-stats-sinks, (Going down this path would only be worthwhile if the thread-local storage issue can also be overcome.)

[bpalermo]

Thanks, I like the direction and I think the factory part should happen. Before committing to the "delete ours and link Envoy's" half, I went and checked what Nighthawk's stats actually look like to a sink, and found something that changes the shape of the work.

Supporting Server::Configuration::StatsSinkFactory is cheap and I'm happy to do it. NighthawkServerFactoryContext already exists (source/client/process_impl.cc) and implements ServerFactoryContext with working threadLocal(), messageValidationContext(), localInfo(), scope(), api() and timeSource(). Envoy's UDP statsd and DogStatsD factories need only the first two, so they can be constructed as-is. The TCP statsd path would hit our panicking clusterManager(), but that's the branch we reject today anyway. A new --envoy-stats-sinks flag resolving factories straight from Envoy's registry, feeding the same sink list and flush worker, is a small change.

The blocker for reuse isn't the factory type, it's that Nighthawk's latency histograms aren't store histograms. SinkableStatistic is constructed directly as a HistogramImplHelper over the symbol table (source/common/statistic_impl.cc) — deliberately, per the comment there, to bypass StatName setup. It is never registered with the store, so it never appears in MetricSnapshot::histograms(). Its only route to a sink is deliverHistogramToSinks() -> Sink::onHistogramComplete().

That matters because most Envoy sinks implement onHistogramComplete as an explicit no-op and work purely from the snapshot: metrics_service/grpc_metrics_service_impl.h, open_telemetry/open_telemetry_impl.h and hystrix/hystrix.h all have void onHistogramComplete(const Stats::Histogram&, uint64_t) override {}. So if we linked the OTel or metrics-service sink today, counters, gauges and Envoy's own histograms would flow, while benchmark_http_client.latency_2xx, latency_grpc_ok and benchmark_stream.message_latency would be silently absent. No error, just missing data — which I'd rather not ship.

The statsd family does receive them, but reports them in the wrong unit. UdpStatsdSink::onHistogramComplete special-cases only Percent; everything else goes out as buildMessage(histogram, std::chrono::milliseconds(value).count(), "|ms"), which is the raw value with an ms label and no regard for Histogram::Unit. Nighthawk records nanoseconds and returns Unit::Unspecified on purpose, so every latency sample would be off by 10^6. Declaring a unit wouldn't help, since the sink ignores it — and Envoy has no nanosecond unit. The comment in that function already flags this as a known gap.

On tags: yes in principle, not yet in practice. TagSpecifier.fixed_value is exactly the right mechanism for constant tags like run:phase-c, and I agree that's better than a Nighthawk flag. But tag extraction has nothing to attach to here: our sinkable statistics carry an empty MetricImpl, so tags() is empty and the worker id stays embedded in the name rather than becoming a worker:<n> tag.

So I'd suggest splitting it:

1. Add --envoy-stats-sinks for Server::Configuration::StatsSinkFactory, as you propose. Happy to do this now, in this PR or a separate one — your call.
2. Deprecate --stats-sinks, but keep the Nighthawk sink until the gaps below are closed, since it's currently the only one reporting correct units and per-worker identity.
3. Make Nighthawk's statistics real store-registered histograms with proper StatNames. That's the change that unlocks everything: they land in snapshots, every Envoy sink handles them by the normal path, and fixed_value tags start working. A separate small Envoy-side fix to scale by Histogram::Unit would then let the statsd family report correct timings, after which our sink is redundant and can go.

One thing I still need to pin down: I hit a thread-local assert when a TLS-based sink ran under our flush worker and worked around it with a process-wide socket. FlushWorkerImpl::flushStats carries a Thread::SkipAsserts, so that area is touchy. I'd want to confirm Envoy's UDP sinks are safe on that thread before claiming step 1 works with them unmodified.

Does that split sound reasonable, and would you prefer step 1 as its own PR with this one rebased on top?

[eric846]

I would definitely do any other experiments in a separate PR on the side, because the current PR may end up being the only viable solution. I left some review comments on the current PR in case we use it.

On tags, agreed. Maybe Nighthawk's stats can be improved later, but for now a new flag is simpler.

My overall motivation here is to try to find a way to avoid adding a new sink, which is a lot of code to review and maintain in Nighthawk. I think avoiding this new sink is even worth it even if it means updating Nighthawk's stats to be more like Envoy's, and/or workarounds like a compatibility mode where we have to divide all values by 1,000,000.

For the other PR:

• You mentioned that supporting the Envoy factories is easy but there are blockers when adopting the plugins. I would only proceed with Envoy factories if it's possible to overcome the blockers.
  • What's the situation with benchmark_http_client.latency_2xx? Would Envoy's UDP statsd sink save it, but OTel and most others would drop it? I don't think we should create excitement that OTel or other sinks would now be supported, only to have obvious things like benchmark_http_client.latency_2xx missing.
• If you're seeing panics in NighthawkServerFactoryContext and NighthawkServerInstance, we routinely replace a panic with a working stub when some new code path starts hitting the panic (usually Envoy calling us in a new way). You might be able to get past the panic.
• You mentioned some redesign of stats in Nighthawk that might make it work properly with Envoy stats sinks. Is it possible to make the changes backward-compatible with third-party Nighthawk stats sink plugins (which do exist) or add the new Envoy-statsd-compatible code in parallel to all the existing Nighthawk stats?
• We have to consider the risk of a new stats implementation in Nighthawk being too expensive to create, review, and check for backward compatibility. It would be great if existing tests can confirm the correctness.

[bpalermo]

Your motivation makes sense to me, and I'd rather land less Nighthawk code than more — so let me answer the latency_2xx question directly, because it turns out to be fixable and small.

benchmark_http_client.latency_2xx was exactly the blocker, and it isn't one any more. You're right about the asymmetry: Envoy's UDP statsd sink would keep it, and OTel, metrics_service and hystrix would all drop it, because they implement onHistogramComplete() as an empty override and read MetricSnapshot::histograms() instead — which Nighthawk's statistics never appear in, since SinkableStatistic is a HistogramImplHelper over an empty MetricImpl that is never registered with the store.

The fix is to record each sample into a store histogram as well. ParentHistogramImpl::recordValue() calls deliverHistogramToSinks() itself, so this replaces our direct delivery rather than duplicating it: one call now feeds both the snapshot and the sinks that want individual samples. I have this working on a branch — three source files, about forty lines, plus a test asserting the histogram shows up in store.histograms(). Happy to put it up as its own PR, per your preference for keeping experiments separate.

On backward compatibility with existing Nighthawk sink plugins, which was your sharpest question. NighthawkStatsSinkFactory is untouched, onHistogramComplete() delivery is preserved, and the emitted metric names are unchanged — the histogram is created in the worker's cluster.<n>. scope, so it arrives named cluster.<n>.benchmark_http_client.latency_2xx exactly as before. The existing test_stats_sinks.py integration tests pass without modification, which is the evidence you asked for.

There is one real exception, a sink that recovers the worker id via dynamic_cast<const SinkableStatistic*> on the delivered histogram will now get nullptr, because it receives a ParentHistogram. The sink in this PR does exactly that, and only survives because it also parses the cluster.<n>. prefix from the name. If that is a concern for third-party plugins, the worker id is still recoverable from the name, and we could keep the old delivery path alongside the new one — though I would rather not, since carrying both is the kind of maintenance cost you are trying to avoid.

On the ms problem and your conversion-flag idea. I would push back gently on converting nanoseconds before writing to any sink: it bakes a unit assumption into Nighthawk globally and corrupts sinks that want raw values. There is a cleaner shape available, because the store histogram is a separate object from Nighthawk's HdrHistogram — so it can carry microseconds and declare Unit::Microseconds while Nighthawk's own output keeps full nanosecond resolution. No global conversion, no precision loss where it matters.

That only helps once Envoy's statsd sinks actually honour the unit, which today they do not: UdpStatsdSink::onHistogramComplete does std::chrono::milliseconds(value).count(), which relabels rather than converts, and only special-cases Percent. I have drafted a fix for that at envoyproxy/envoy#47505 (scale by unit, behind a runtime guard). It is a draft and unmerged, so it is a sequencing dependency rather than something to count on — but it also fixes downstream_rq_tx_time, which is mislabelled in Envoy today, so it stands on its own merits.

On the thread-local writer, from your earlier comment: the sink here avoids it by keeping one process-wide UDP socket guarded by a mutex, with no thread-local writer at all. Your suggestion about changing when Nighthawk creates the sinks, or when it creates the flush worker thread, is the more interesting direction and I have not pinned down the root cause yet — FlushWorkerImpl::flushStats carries a Thread::SkipAsserts, so the area is already delicate. Since --envoy-stats-sinks is worthless if Envoy's UDP sinks cannot run under our flush worker, I will get to the bottom of that before proposing the flag, rather than assuming it works.

On the panics: good to know stubbing them is routine. For the UDP sinks it may not even arise — they need only threadLocal() and messageValidationContext(), both of which work today. It is clusterManager() that panics, and that is only on the TCP statsd path, which this PR rejects anyway.

On cost, since you raised the risk of an expensive stats change: the mirror adds a cached thread-local lookup and a circllhist insert per sample, and exactly one mirrored record per request in HTTP mode (the latency-by-status statistics are mutually exclusive). I measured it on loopback at 5k rps and it sits below the noise floor — two runs of the identical build differed by more than the change did — which bounds it at well under a microsecond per sample against roughly 140 us of client CPU per request.

Happy to keep this PR open as the fallback while the other path is explored, as you suggest. If the mirror plus --envoy-stats-sinks work out, the sink here should simply be deleted, which I think is the outcome you are after.

[eric846]

Reading your earlier comment more closely, I think you answered some of my questions. I agree with the way you split it. I would try that in a separate PR in case the current PR ends up being the better option.

[bpalermo]

Opened #1616 with the split you suggested. It's the store histogram change only — recording each sample into an Envoy store histogram so Nighthawk's latency statistics appear in MetricSnapshot::histograms(), which is what makes benchmark_http_client.latency_2xx visible to the OTel and metrics_service sinks rather than silently absent.

It also carries a test answering the threading question from your earlier comment, and I owe you a correction there: I said the thread-local issue was unresolved and that I'd get to the bottom of it before proposing anything. I have, and it isn't a blocker — Envoy's UdpStatsdSink works under Nighthawk's arrangement, including a final flush after tls_.shutdownGlobalThreading(), which is the order ProcessImpl::shutdown() uses. The only precondition is that threads touching such a sink are registered with the ThreadLocal instance, which Nighthawk's already are. What I originally hit was almost certainly a test harness that registered no threads, not the runtime.

--envoy-stats-sinks is deliberately not in it — that can follow once the foundation is agreed. Keeping this PR open as the fallback, as you suggested. No urgency on my side.