Repository navigation
Conversation
ProcessImpl::setupStatsSinks only consulted Nighthawk's own stats sink factory registry, so sink configurations naming a linked Envoy stats sink (e.g. envoy.stat_sinks.statsd) failed with "Didn't find a registered implementation", even though the CLI documentation advertised exactly such configurations. Users could not flush Nighthawk metrics to any Envoy-provided stats sink (envoyproxy#1607). Consult Envoy's Server::Configuration::StatsSinkFactory registry first, by typed config type URL, which is how Envoy's own server resolves stats sinks; fall back to Nighthawk's registry when no Envoy factory matches. Configurations matching neither registry keep failing fast with an error naming the sink. Errors returned by an Envoy sink factory's createStatsSink propagate as an EnvoyException and are never swallowed. Link the envoy.stat_sinks.statsd extension via extensions_build_config.bzl, and point the documented example at the cluster-free UDP address form. The statsd sink emits raw histogram samples when the benchmark client records them; percentile summaries are not part of its statsd payload. Fixes envoyproxy#1607 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Member
Author
|
Closing for now - we'll re-submit from our fork when the timing is right. Thanks! |
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.
Fixes #1607
What
ProcessImpl::setupStatsSinkslooked sinks up only in Nighthawk's ownNighthawkStatsSinkFactoryregistry. A bootstrap sink config naming alinked Envoy stats sink (e.g.
envoy.stat_sinks.statsd) therefore failedwith "Didn't find a registered implementation", even though the
--stats-sinkshelp text advertised exactly such configurations, sousers could not flush Nighthawk's benchmark metrics to any
Envoy-provided stats sink.
setupStatsSinksnow consults Envoy'sServer::Configuration::StatsSinkFactoryregistry first — by typedconfig type URL, the same lookup Envoy's own server performs — and falls
back to Nighthawk's registry when no Envoy factory matches. Configs
matching neither registry keep failing fast with an error naming the
sink. Errors returned by an Envoy sink factory's
createStatsSinkpropagate as an
EnvoyException; they are never swallowed.This change also links the
envoy.stat_sinks.statsdextension viaextensions_build_config.bzland points the documented example at thecluster-free UDP
addressform.Test evidence
New
process_testcases (v4 and v6):EnvoyStatsSinkHonored: a--stats-sinksconfig namingenvoy.stat_sinks.statsdwith a UDP address on a loopback listener;the run flushes datagrams containing
benchmark.counter names tothat listener. Before this change the run aborted in
getAndCheckFactory<NighthawkStatsSinkFactory>withDidn't find a registered implementation for 'envoy.stat_sinks.statsd' with type URL: 'envoy.config.metrics.v3.StatsdSink'.UnknownSinkNameFailsFast: sink nameenvoy.stat_sinks.nosuchsinkfails the run with an error naming the sink.
Full
process_testandservice_testsuites pass locally on macOSexcept the pre-existing
TestWithEncapsulationfailures (asetsockopt: Protocol not availableplatform limitation, reproducibleon
mainwithout this change).DCO signed; CI will confirm on Linux.
Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com