-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Description
In the new system, there are multiple distinct StatName representations for the same string, which can lead to issues with map usage.
I discovered this by modifying source/common/stats/symbol_table_creator.cc to set the default to use real symbol tables, and found crashes in several tests due to unexpected map behavior. One of these was //test/extensions/filters/network/mongo_proxy:proxy_test . There were two problems:
-
The unexpected map behavior without canonical names led to a crash. This can be resolved by removing a superfluous toString() -> encode() dance which occurs currently when instantiating MetricImpl (a common base class to all stats). With this fix in place, there are no longer crashes due to unexpected map behavior, at least in the mongo test.
-
After that, there is still an issue that we are doing a stat lookup based on a pure string in a test, and that fails, because the StatName representation formed from the test flow is not the same as the one created in the production code.
I believe that an effective fix can be made by always re-encoding the StatNames without dynamic components when storing in the Allocator, which will match the behavior expected in tests. Other maps, including the histogram map in IsolatedStoreImpl, will have to be treated more carefully.
Users should not enable --use_fake_symbol_tables 0 until this is resolved.