Skip to content

stats: new dynamic stat functionality has an issue with aliasing for StatNames, and its interaction with maps. #9768

@jmarantz

Description

@jmarantz

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:

  1. 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.

  2. 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.

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions