stats: make it's possible to enable the new stats API#45846
Conversation
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
… dev-add-flag-to-enable-new-stats-api
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the explicit-tags logic for the stats store, allowing scope-level tags to propagate to every stat without re-parsing flat names, controlled by the runtime guard envoy.reloadable_features.enable_stats_explicit_tags. The review feedback identifies a critical memory corruption issue in legacy mode where populateList is called with empty arguments, resulting in an incorrectly sized StatNameList. Additionally, suggestions are made to add safety assertions in SymbolTable::populateList and to use auto for type deduction when creating a shared pointer.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| // Only single prefix for legacy mode. Both prefix_ and base_prefix_ are set to the same value, | ||
| // which is the flat (tagged) prefix. | ||
| parent_.alloc_.symbolTable().populateList(prefix, {}, {}, prefix_list_); |
There was a problem hiding this comment.
In legacy mode, calling the 4-argument populateList with empty base_name and name_tags results in a corrupted StatNameList because stat_name_count is set to 2, but only 1 element's bytes are written. When the list is cleared in the destructor, it will iterate 2 times, reading out-of-bounds garbage bytes for the second element and calling symbol_table.free on them, which can cause memory corruption or crashes.
Since we only need to store a single prefix in legacy mode, we should use the 3-argument populateList overload instead, which correctly populates the list with exactly 1 element.
| parent_.alloc_.symbolTable().populateList(prefix, {}, {}, prefix_list_); | |
| parent_.alloc_.symbolTable().populateList(&prefix, 1, prefix_list_); |
There was a problem hiding this comment.
the populate list could handle empty stat name correctly. The empty stat name also has one byte.
| void SymbolTable::populateList(StatName tagged_name, StatName base_name, StatNameTagSpan name_tags, | ||
| StatNameList& list) { | ||
| const size_t stat_name_count = name_tags.size() * 2 + 2; |
There was a problem hiding this comment.
To prevent silent corruption of StatNameList when empty StatNames are passed, we should add assertions to ensure that tagged_name, base_name, and all tags in name_tags are not empty.
void SymbolTable::populateList(StatName tagged_name, StatName base_name, StatNameTagSpan name_tags,
StatNameList& list) {
ASSERT(!tagged_name.empty());
ASSERT(!base_name.empty());
for (const auto& tag : name_tags) {
ASSERT(!tag.first.empty());
ASSERT(!tag.second.empty());
}
const size_t stat_name_count = name_tags.size() * 2 + 2;There was a problem hiding this comment.
the populate list could handle empty stat name correctly. The empty stat name also has one byte.
| // re-interns them into the child's own pool). | ||
| const TagUtility::TagStatNameJoiner joiner(base_prefix_, prefix_tags_, prefix_, base_name, | ||
| name_tags, tagged_name, symbolTable()); | ||
| std::shared_ptr<ScopeImpl> new_scope = std::make_shared<ScopeImpl>( |
There was a problem hiding this comment.
According to the general rules, we should prefer using auto for variable declarations when the type is clearly deduced from the right-hand side (e.g., when using std::make_shared or std::make_unique).
| std::shared_ptr<ScopeImpl> new_scope = std::make_shared<ScopeImpl>( | |
| auto new_scope = std::make_shared<ScopeImpl>( |
References
- Use 'auto' for variable declarations when the type is clearly deduced from the right-hand side (e.g., when using 'std::make_shared' or 'std::make_unique'), as explicit types in these cases are considered redundant and noisy.
Commit Message: stats: make it's possible to enable the new stats API
Additional Description:
The PR did two things:
stats_tagsanduse_all_default_tagsto infer whether the new tag-friendly API should be used or not.stats_tagsanduse_all_default_tagswhen we constructing the ThreadLocalStore and the root scope. So, we need to ensure the a single scope implementation could support both legacy and new logic.The new tag-friendly API will be enabled when: no custom
stats_tagsanduse_all_default_tagsis true.NOTE: We still add a new runtime flag. This runtime flag is used to guard existing behavior in the process of migrating the existing stats API calling to use the new API. We need lots of changes to migrate and ensure all default tags could be emitted correctly by the new API. Before complete the migration, the runtime guard is necessary.
Risk Level: low.
Testing: unit.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.