stats: Ensure sanity of stats prefixes #8445
Conversation
Signed-off-by: Joshua Marantz <[email protected]>
jmarantz
requested review from
alyssawilk,
mattklein123 and
snowp
as code owners
There was a problem hiding this comment.
Thanks for jumping on this. I'm a little concerned with the complexity at the call sites and I'm wondering if we can simplify this somehow within the stats subsystem?
cc @kb000 PTAL
/wait
| @@ -32,7 +33,8 @@ Network::FilterFactoryCb MongoProxyFilterConfigFactory::createFilterFactoryFromP | |||
| fault_config = std::make_shared<Filters::Common::Fault::FaultDelayConfig>(proto_config.delay()); | |||
| } | |||
|
|
|||
| auto stats = std::make_shared<MongoStats>(context.scope(), stat_prefix); | |||
| auto stats = | |||
| std::make_shared<MongoStats>(context.scope(), Stats::Utility::sanitizeStatsName(stat_prefix)); | |||
There was a problem hiding this comment.
The fact that extensions have to do this seems kind of fragile to me. Also don't we have this same problem elsewhere like in the HCM? Is there any way we can centralize this in the stats APIs?
There was a problem hiding this comment.
I thought of just doing the fix inside the symbol table API.
But it really only comes up in 2 places: Mongo and one other one, where we are both using the macros (needing a trailing dot) and capturing the prefix in a symbol-table, for later join().
I think my preference for these two places would be to eliminate the use of the macros -- which I may cause contentions name-lookups -- at least in those places.
I can look at doing that instead in this PR, or I could do it as a follow-up.
Another cleanup I thought of is to remove the trailing dot from the usage in macros. That would be pretty easy but would hit about 50 places in the code so I'd like that to be a non-functional refactor.
There was a problem hiding this comment.
I think my preference for these two places would be to eliminate the use of the macros -- which I may cause contentions name-lookups -- at least in those places.
What do you mean exactly? FWIW the macros are way easier to understand for the average developer IMO than the other options, so I would prefer to keep them where can, as it hides a lot of underlying complexity in the standard case.
Another cleanup I thought of is to remove the trailing dot from the usage in macros. That would be pretty easy but would hit about 50 places in the code so I'd like that to be a non-functional refactor.
I think this would be my preference. We can just be fully consistent about no dots?
There was a problem hiding this comment.
I updated to be fully consistent with no trailing dots, and changed the macros to account for that. Actually to avoid blowing up the PR I made it accept either way, with a TODO to remove all trailing dots in a follow-up.
…join(). Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
* Ensure sanity of stats prefixes. Signed-off-by: Joshua Marantz <[email protected]>
* Ensure sanity of stats prefixes. Signed-off-by: Joshua Marantz <[email protected]>
Description: The stat-creation macros expect a prefix that's suffixed by a
".". However, scope-creation, and stat-prefixes that are used inSymbolTable::join()should not have one.There are a few cases where an extra period is present, or one is missing. In particular it was easy to get this wrong for prefixes that were used both for the macros and
SymbolTable::join().This PR adds ASSERTs for both those missing and extra
"."and fixes a handful of violations in both directions. For completeness it covers the case from #8444 though it does it slightly differently, re-usingStats::Utility::sanitizeStatsName().Risk Level: low
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a