stats: do not canonicalize StatNames when storing in allocator#9836
stats: do not canonicalize StatNames when storing in allocator#9836jmarantz merged 24 commits intoenvoyproxy:masterfrom
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>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
…of the Lookup helper. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
… there is a call to reset(). Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks I like this solution much better. Q: How does a test writer know that they might have to use the new wrapper utility until we make every test use the utility? I'm worried about hard to debug failures. Is there anything we can do to make that more obvious somehow?
/wait
jmarantz
left a comment
There was a problem hiding this comment.
Thanks I like this solution much better. Q: How does a test writer know that they might have to use the new wrapper utility until we make every test use the utility? I'm worried about hard to debug failures. Is there anything we can do to make that more obvious somehow?
/wait
After this I can embark on a massive PR to remove the API Stats::counter("xxx") and then there will be no choice but to use this mechanism :)
I think while that PR is baking we will only find issues where devs are explicitly creating Dynamic stat names in the same PR.
Signed-off-by: Joshua Marantz <jmarantz@google.com>
…atStore, making some things simpler. Signed-off-by: Joshua Marantz <jmarantz@google.com>
…e way too slow. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
|
/azp run envoy-macos |
|
Azure Pipelines successfully started running 1 pipeline(s). |
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, this is awesome. Really happy with how this turned out!
|
Thanks Matt -- definitely appreciate the honest feedback about how to improve it and make it easier to complete the task across the codebase. |
Description: Resolves a problem that was discovered when experimentally switching the default SymbolTable implementation from FakeSymbolTableImpl to RealSymbolTableImpl, and then running all tests. The problem is that there are now distinct StatName representations for the same string, and hash lookups using the StatName hash/equal functions will consider them distinct.
This manifested in two problems:
There was an accidental canonicalization that was taking place in the stat allocation process, where a StatName was being rendered as a string and then re-encoded. This meant that the StatName passed into the allocator would not be equivalent to counter.statName() coming out of it. This behaved badly in maps, such as the one in IsolatedStatStore, and likely others. In this PR we resolve this problem by removing that serialize/re-parse phase during allocation. This should make things faster as well.
Tests often check stat values by looking up via string, and there's no easy way to figure out which parts of the string were supposed to be dynamic. The leanest solution is to just just use the find-by-name functionality in TestUtility::findCounter and findGauge. This PR adds a class to make that work using the Scope API to minimize code changes.
Ultimately all tests should be changed to this mechanism; this PR only changes the ones that were needed due to dynamic stats, and a couple of others as a PoC.
Risk Level: medium -- this re-enables dynamic symbol tables.
Testing: Ran through CI completely with real-symbol tables on by default for tests, but the PR as proposed goes back to fakes-by-default.
Docs Changes: n/a
Release Notes: n/a
Fixes: #9768