stats: Add API Scope::scopeFromStatName and use it to skip some symbol table locks during routing recompute from control-plane update.#14418
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>
…lang-tidy. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
snowp
left a comment
There was a problem hiding this comment.
Looks right to me, just some comment comments :)
| * @param name supplies the scope's namespace prefix. | ||
| */ | ||
| virtual ScopePtr createScope(const std::string& name) PURE; | ||
| virtual ScopePtr scopeFromStatName(StatName name) PURE; |
source/common/stats/utility.h
Outdated
| * | ||
| * @param scope The scope in which to create the counter. | ||
| * @param elements The vector of mixed DynamicName and StatName | ||
| * @param tags optionally specified tags. |
| * @return A counter named using the joined elements. | ||
| */ | ||
| static ScopePtr scopeFromElements(Scope& scope, const ElementVec& elements); | ||
| static ScopePtr scopeFromStatNames(Scope& scope, const StatNameVec& names); |
There was a problem hiding this comment.
Should this have its own doxygen comments?
mattklein123
left a comment
There was a problem hiding this comment.
Thanks generally LGTM with small comments. Should there be some explicit tests for the new functionality vs. the implicit testing we have here?
/wait
source/common/router/config_impl.h
Outdated
| struct StatNameProvider { | ||
| StatNameProvider(absl::string_view name, Stats::SymbolTable& symbol_table) | ||
| : stat_name_storage_(std::make_unique<Stats::StatNameManagedStorage>(name, symbol_table)) {} | ||
| std::unique_ptr<Stats::StatNameManagedStorage> stat_name_storage_; |
There was a problem hiding this comment.
Is there any reason this needs to be heap allocated within the struct? Can it be inline?
There was a problem hiding this comment.
I thought there was some order-of-init issue but evidently not. Done.
| auto new_scope = std::make_unique<ScopeImpl>(*this, stat_name_storage.statName()); | ||
| Thread::LockGuard lock(lock_); | ||
| scopes_.emplace(new_scope.get()); | ||
| return new_scope; |
There was a problem hiding this comment.
nit: can you just call scopeFromStatName?
There was a problem hiding this comment.
Yup, good catch, thanks.
source/common/stats/utility.h
Outdated
| * @param scope The scope in which to create the counter. | ||
| * @param elements The vector of mixed DynamicName and StatName | ||
| * @param tags optionally specified tags. | ||
| * @return A counter named using the joined elements. |
There was a problem hiding this comment.
I think these are out of date.
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
mattklein123
left a comment
There was a problem hiding this comment.
This LGTM though I would still prefer to have some explicit stats tests of the new behavior?
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Commit Message: Adds an API for constructing scopes from StatName without holding a string representation, which helps avoid taking a lock. It was previously thought that scopes were not created on the hot-path, so this was presumed not to be necessary. But flame-graphs and code analysis suggest we can still contend on VirtualCluster creation, which involves making scopes.
As of now we are not trying to remove all creation of scopes from strings, but that is something we may want to consider as a follow-up.
Additional Description:
Risk Level: medium
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a