-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor Stats::RawStatData into a StatsOptions struct #3629
Merged
Merged
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
bb349a1
Refactor Stats::RawStatData into a StatsOptions struct.
ambuc 7bfa762
Store StatsOptions values as size_t
ambuc 27990bf
Add heapAllocator stat name truncation in ThreadLocalStore, plus nits
ambuc b92ab75
Remove unused BlockMemoryHashSet accessor function
ambuc 3a7aff1
Rename safeInitialize() to truncateAndInit()
ambuc 3f38407
Change interface of sizeGivenName()
ambuc 19f9f3c
Use new translateBootstrap interface in v1_to_bootstrap
ambuc 06bd4c2
Fix formatting
ambuc 217f72f
Add documentation, re-trigger ci
ambuc 1ae932c
Rework some interfaces to pass statsOptions rather than Scope at lowe…
ambuc b07d5c6
Prefer UNREFERENCED_PARAMETER to (void)
ambuc 732af4f
Better inline docs for stat name / object / suffix terminology
ambuc 7328dc0
Merge remote-tracking branch 'upstream/master' into refactor-StatsOpt…
ambuc 71f6b24
Edit stats inline documentation
ambuc 2aa9d8c
Provide safe copy assertion in RawStatData::initialize()
ambuc 911fc5d
Abstract checkAndInit() and truncateAndInit() with shared initializat…
ambuc dbcdb72
Formatting fixes and nits
ambuc 9a30244
Merge remote-tracking branch 'upstream/master' into refactor-StatsOpt…
ambuc aeb9251
Refactor sizeGivenName(), sizeGivenStatsOptions() into structSize(), …
ambuc cb0fd8a
Add todo addressing embedded assumption about allocator fallback
ambuc 19e0311
Merge remote-tracking branch 'upstream/master' into refactor-StatsOpt…
ambuc 4cad816
Throw std::bad_alloc() in HeapRawStatDataAllocator::alloc()
ambuc 9c6b880
Merge remote-tracking branch 'upstream/master' into refactor-StatsOpt…
ambuc c1fa6fb
Merge remote-tracking branch 'upstream/master' into refactor-StatsOpt…
ambuc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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 file contains 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 file contains 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 file contains 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 file contains 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 file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the object part and the suffix part of a stat? Can you potentially provide an example in the comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's more color in
/source/common/stats/stats_impl.h
, which I'm not sure how much of to duplicate in the include file. So the object part is something likecluster.<cluster_name>.outlier_detection
, and the suffix part is one of the statistics listed in the docs here likeejections_enforced_consecutive_gateway_failure
. We just want to limit the lengths of these halves separately. Do you think it's worth adding that to the include too?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just comment about where the other comments are? In general I would expect the public includes to have the detail so I can understand what things mean, but fine to link to other places. Whatever works. It's mostly that this nomenclature caught me off guard so I think it might confuse others also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me. I did some work in 732af4f.