event: reintroduce dispatcher stats#6659
event: reintroduce dispatcher stats#6659htuch merged 2 commits intoenvoyproxy:masterfrom mergeconflict:enable_dispatcher_stats
Conversation
Signed-off-by: Dan Rosen <mergeconflict@google.com>
|
/review @htuch @mattklein123 |
mergeconflict
left a comment
mergeconflict
left a comment
There was a problem hiding this comment.
A few notes for reviewers inline...
| WorkerFactory& worker_factory) | ||
| : server_(server), factory_(listener_factory), stats_(generateStats(server.stats())), | ||
| WorkerFactory& worker_factory, | ||
| bool enable_dispatcher_stats) |
There was a problem hiding this comment.
Note: I put this here, rather than as a param in startWorkers() or a new initializeStats() method, to avoid touching the ListenerManager interface. I could do this differently if anybody feels I should...
| }; | ||
|
|
||
| // TODO(mergeconflict): We also need integration testing to validate that the expected histograms | ||
| // are written when `enable_dispatcher_stats` is true. See issue #6582. |
There was a problem hiding this comment.
I'm planning to implement integration tests in a following PR, once I've dealt with #6582, if that's still ok.
| running---but if this number elevates substantially above its normal observed baseline, it likely | ||
| indicates kernel scheduler delays. | ||
|
|
||
| These statistics can be enabled by setting :ref:`enable_dispatcher_stats <envoy_api_field_config.bootstrap.v2.Bootstrap.enable_dispatcher_stats>` |
| // Enable :ref:`stats for event dispatcher <operations_performance>`, defaults to false. | ||
| // Note that this records a value for each iteration of the event loop on every thread. This | ||
| // should normally be minimal overhead, but when using | ||
| // :ref:`statsd <envoy_api_msg_config.metrics.v2.StatsdSink>`, it will send each observed value |
There was a problem hiding this comment.
Is it just statsd or also other stats sinks?
There was a problem hiding this comment.
Yes, just statsd. The Monarch sink appears to just dump histograms on the floor, and the Hystrix and Metrics sinks both do the right thing with parent histograms.
|
/retest |
|
🔨 rebuilding |
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
LGTM; ideally when you add new behavioral tests these also cover the enable/disable case.
Signed-off-by: Dan Rosen <mergeconflict@google.com>
|
@mergeconflict needs master merge for version history. |
* master: docs: add extension policy (envoyproxy#6678) ext_authz: added ability to detect partial request body data (envoyproxy#6583) version_history.rst: jwt_authn change missed 1.10.0 (envoyproxy#6684) docs: fix link in pull request template (envoyproxy#6679) Explicitly convert absl::string_view to std::string. (envoyproxy#6687) docs: improving watermark docs/comments (envoyproxy#6683) http filter: add CSRF filter (envoyproxy#6470) event: reintroduce dispatcher stats (envoyproxy#6659) security: postmortem for CVE-2019-990[01] (envoyproxy#6597) Improve build rules for (test only) library quic_port_utils. (envoyproxy#6672) spell check: skip unsupported extensions when called with a file (envoyproxy#6648) Changed TestHooks to ListenerHooks (envoyproxy#6642) proto: move extension-specific linking validation into extensions (envoyproxy#6657) stats: add/test heterogenous set of StatNameStorage objects. (envoyproxy#6504) docs: move xds protocol to rst (envoyproxy#6670) fix version history order (envoyproxy#6671) Signed-off-by: Michael Puncel <mpuncel@squareup.com>
3 participants
Description: Reintroduce dispatcher stats previously reverted in #6649. Dispatcher stats are now gated on a new bootstrap config parameter,
enable_dispatcher_stats, and disabled by default.Risk Level: Low
Testing: Manually verified that config is respected (see #6582, this still requires integration tests).
Docs Changes: Included a note about how statsd doesn't play well with these high-volume histograms.
Signed-off-by: Dan Rosen mergeconflict@google.com