stats: In StatNameSet, differentiate between dynamic and builtin name lookup, which should have a fallback and avoid locks #8243
Conversation
…h a required fallback stat-name for Builtins rather than a potential lock. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
…her cleanups. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
jmarantz
changed the title
|
Looks good to me; appreciate the simplification. |
…ted. Signed-off-by: Joshua Marantz <jmarantz@google.com>
There was a problem hiding this comment.
In general I really like where this is going. @jmarantz is your thinking to just fully get rid of dynamic for now and only do the default fallbacks? We probably can't when it comes to things like HTTP status codes, but just wondering what your thinking is. Are there certain things in this PR that you specifically want to discuss?
/wait-any
|
I'll address the code nits later today but wanted to carry on the dialog. I would love to not have any dynamic stats. I handled HTTP status (I think) acceptably via an array of atomics in envoy/source/common/http/codes.cc Line 186 in 81460d8 I could re-do dynamo_filter.cc that way if the 'status' reported there is really an http-style status as opposed to any arbitrary integer. I don't know anything about Dynamo so guidance here would be appreciated. We also name stats for tables, which I'm guessing are totally data-driven in dynamo_stats.cc, so removing support for getDynamic() would be a regression there. I'm not sure if there is also a known set of choices for error_type. Same with partition-id. source/extensions/filters/http/fault/fault_filter.cc names a stat for the downstream cluster. We could plumb that all the way up and symbolize a stat-name for that when the downstream cluster is created. That probably wouldn't be too bad. source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc increments a stat based on a tag. I'm not sure if/when we can enumerate the legal set of tags. mongo proxy names stats for collections and call-sites. Can/should we enumerate those? Then there's the wasm folks (@mandarjog et al) who are creating named stats from wasm code. But I think that might be OK because the flow from wasm is that you allocate a stat by name (taking a lock no matter what if that stat was never seen before) and then get a handle to it which you can re-use in your filter. So that might be OK? |
|
Hmm yeah I don't think we are going to easily be able to get rid of dynamics right now, but I agree our default policy should be to not use them where it makes sense to do so. Thanks for the very detailed explanation. |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
|
/retest |
|
🔨 rebuilding |
|
For a quick sanity check I built the PR against my test environment and verified the stats for redis look good. |
|
Thanks Nicolas! |
| @@ -671,15 +683,19 @@ class StatNameSet { | |||
| * set's mutex and also the SymbolTable mutex which must be taken during | |||
| * StatNamePool::add(). | |||
| */ | |||
| StatName getStatName(absl::string_view token); | |||
| StatName getDynamic(absl::string_view token); | |||
| StatName getBuiltin(absl::string_view token, StatName fallback); | |||
… lookup, which should have a fallback and avoid locks (envoyproxy#8243) * Split StatNameSet API into explicit Dynamic and Builtin requests, with a required fallback stat-name for Builtins rather than a potential lock. Signed-off-by: Joshua Marantz <jmarantz@google.com>
… lookup, which should have a fallback and avoid locks (envoyproxy#8243) * Split StatNameSet API into explicit Dynamic and Builtin requests, with a required fallback stat-name for Builtins rather than a potential lock. Signed-off-by: Joshua Marantz <jmarantz@google.com>
… lookup, which should have a fallback and avoid locks (envoyproxy#8243) * Split StatNameSet API into explicit Dynamic and Builtin requests, with a required fallback stat-name for Builtins rather than a potential lock. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Description: In #7890 it became clear we could enumerate the set of possible Redis commands, and use a fallback symbol to report stats on unexpected redis commands, to avoid taking an unexpected lock. This captures that semantic in StatNameSet. Unfortunately there are still some classes of stat name lookups that appear to be dynamic, so will generally require locks. This PR calls them out explicitly and we can potentially discuss them individually in this PR and decide what we want to do.
Risk Level: low
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a