stats: Scope should not be a parent-class of Store#23851
Merged
jmarantz merged 61 commits intoenvoyproxy:mainfrom Dec 14, 2022
Merged
stats: Scope should not be a parent-class of Store#23851jmarantz merged 61 commits intoenvoyproxy:mainfrom
jmarantz merged 61 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
…ats/... building (not passing) Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
…ation noise. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Contributor
Author
|
/retest |
|
Retrying Azure Pipelines: |
Contributor
Author
|
Greg - I think this is ready for a pass; no super great rush but IMO this is the right direction, and will also make it easier for @stevenzzzz to build and test a mechanism that uses scopes to hold onto live cluster stats across and xDS updates. |
…, and it turns out we only need the one for Gauges. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Contributor
Author
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
ggreenway
previously requested changes
Dec 12, 2022
Member
ggreenway
left a comment
There was a problem hiding this comment.
This looks good overall. I like the direction it's going.
/wait
envoy/stats/store.h
Outdated
| virtual void forEachScope(SizeFn f_size, StatFn<const Scope> f_stat) const PURE; | ||
|
|
||
| /** | ||
| * @return a null counter that will ignore increemts and always return 0. |
Member
There was a problem hiding this comment.
Suggested change
| * @return a null counter that will ignore increemts and always return 0. | |
| * @return a null counter that will ignore increments and always return 0. |
envoy/stats/store.h
Outdated
| // accessing the rootScope() to call these methods. | ||
| operator Scope&() { return *rootScope(); } | ||
|
|
||
| // Delegate somea methods to the root scope; these are exposed to make it more |
Member
There was a problem hiding this comment.
Suggested change
| // Delegate somea methods to the root scope; these are exposed to make it more | |
| // Delegate some methods to the root scope; these are exposed to make it more |
envoy/stats/store.h
Outdated
| operator Scope&() { return *rootScope(); } | ||
|
|
||
| // Delegate somea methods to the root scope; these are exposed to make it more | ||
| // convenient to use stats_macros.h. We mayconsider dropping them if desired, |
Member
There was a problem hiding this comment.
Suggested change
| // convenient to use stats_macros.h. We mayconsider dropping them if desired, | |
| // convenient to use stats_macros.h. We may consider dropping them if desired, |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
ggreenway
approved these changes
Dec 14, 2022
dubious90
pushed a commit
to envoyproxy/nighthawk
that referenced
this pull request
Dec 20, 2022
- Change the way of calling deliverHistogramToSinks. Now deliverHistogramToSinks is a member of Store instead of Scope. This is due to the change of envoy envoyproxy/envoy#23851. - Update config.yaml to envoy version - No changes in other files. Signed-off-by: fei-deng <feid@google.com>
This was referenced Jan 10, 2023
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
4 participants
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.
Commit Message: Refactors so that Stats::Store is no longer a subclass of Stats::Scope. Previously it was, so that the root-scope was effectively the same thing as the Store. This causes issues and confusion because Stores are typically created directly as test-class member variables, but Scopes were mostly created as shared_ptr for various reasons. This refactor attempts to cleanly, but minimally, enable the memory management of Scopes to be strictly shared_ptr. It also separate two different concepts.
Additional Description:
This leaves behind a bit of technical debt to keep this refactor reviewable. This is tracked in #24007 and can be addressed in a follow-up PR that will be much larger than this one, but will be strictly a series of simple replacements (
makeStats(store_) --> makeStats(*store_->rootScope()))To reviewers: I recommend looking at scope.h and store.h carefully to see the real change that was done here. Then also look at
isolated_store_impl.*,stat_test_utility.*, and the changes to the mocks. Finally look attest/integration/server.hfor the change in the thread-safe wrapper around isolated store there. Everything else should be pretty quick to review once you have that context.Risk Level: medium -- this is a pretty large refactor
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
Fixes: #23862