stats: rework stat sink flushing to centralize counter latching #6996
Conversation
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
This change reworks how stats are flushed to sinks by removing the Source class and replacing it with a MetricSnapshot which provides constant stats for all sinks to flush with counters pre-latched. In addition to cleaning up the flushing logic to unify and constify it, this change has the side effect of performing counter latching every flush interval (even if there are no sinks), this fixing the below reported issue. Fixes #6924 Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
|
Testing the issue reported in #6924, the rate of increments no longer has the dip and spike during a hot restart. The counter values aren't passed between hot restarts so if we look at absolute values in |
|
@fishcakez the reset to 0 is expected. Presumably your metrics system can deal with a counter reset to 0 and we don't need to persist the current value across hot restarts (since this is what happens during a full restart)? |
Yes, it can handle that just fine 👌 |
fredlas
previously approved these changes
May 21, 2019
| // fixfix comment about latching. | ||
| // Create a snapshot and flush to all sinks. | ||
| // NOTE: Even if there are no sinks, creating the snapshot has the important property that it | ||
| // latches all counters on a periodic basis. The hot restart code assumes this is being |
Signed-off-by: Matt Klein <mklein@lyft.com>
jmarantz
reviewed
May 21, 2019
| writer.write(fmt::format("{}.{}:{}|c{}", prefix_, getName(*counter), delta, | ||
| buildTagStr(counter->tags()))); | ||
| for (const auto& counter : snapshot.counters()) { | ||
| if (counter.counter_.get().used()) { |
There was a problem hiding this comment.
counter.counter_->used() does not work here?
There was a problem hiding this comment.
counter_ is a reference_wrapper
Signed-off-by: Matt Klein <mklein@lyft.com>
|
@jmarantz updated |
jmarantz
approved these changes
May 22, 2019
|
/retest |
|
🔨 rebuilding |
mpuncel
added a commit
to mpuncel/envoy
that referenced
this pull request
May 22, 2019
* master: (65 commits) proto: Add PATCH method to RequestMethod enum (envoyproxy#6737) exe: drop unused deps on zlib compressor code (envoyproxy#7022) coverage: fix some misc coverage (envoyproxy#7033) Enable proto schema for router_check_tool (envoyproxy#6992) stats: rework stat sink flushing to centralize counter latching (envoyproxy#6996) [test] convert lds api test config stubs to v2 (envoyproxy#7021) router: scoped rds (2c): implement scoped rds API (envoyproxy#6932) build: Add option for size-optimized binary (envoyproxy#6960) test: adding an integration test framework for file-based LDS (envoyproxy#6933) doc: update obsolete ref to api/XDS_PROTOCOL.md (envoyproxy#7002) dispatcher: faster runOnAllThreads (envoyproxy#7011) example: add csrf sandbox (envoyproxy#6805) fix syntax of gcov exclusion zone. (envoyproxy#7023) /runtime_modify: add support for query params in body (envoyproxy#6977) stats: Create stats for http codes with the symbol table. (envoyproxy#6733) health check: fix more fallout from inline deletion change (envoyproxy#6988) Max heap fix (envoyproxy#7016) Add support to unregister from lifecycle notifications (envoyproxy#6984) build spdy_core_alt_svc_wire_format (envoyproxy#7010) ext_authz: Make sure initiateCall only called once (envoyproxy#6949) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>
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.
This change reworks how stats are flushed to sinks by removing the
Source class and replacing it with a MetricSnapshot which provides
constant stats for all sinks to flush with counters pre-latched.
In addition to cleaning up the flushing logic to unify and constify
it, this change has the side effect of performing counter latching
every flush interval (even if there are no sinks), this fixing
the below reported issue.
Fixes #6924
Risk Level: Low
Testing: Modified existing tests and new unit tests
Docs Changes: N/A
Release Notes: N/A