stats: skip symbolizing empty tokens#16239
stats: skip symbolizing empty tokens#16239jessicayuen wants to merge 1 commit intoenvoyproxy:mainfrom
Conversation
Prevents stats with invalid metric formats, where there are consecutive `..` from being emitted. For example, `cluster.egress_dynamodb_iad.zone.1a..upstream_rq_2xx` Fixes envoyproxy#15541 Signed-off-by: Jess Yuen <jyuen@lyft.com>
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
Looks great! Just a couple of test nits.
| TEST_F(CodeUtilityTest, EmptyPerZoneStats) { | ||
| // Empty from-zone / to-zone stats should not be emitted. | ||
| addResponse(200, false, false, "", "", "from_az"); | ||
| EXPECT_EQ(0U, cluster_scope_.counter("prefix.zone.from_az..upstream_rq_completed").value()); |
There was a problem hiding this comment.
Instead of allocating the counter and checking that it's zero, can you use TestStore::findCounterByString(...) here?
There was a problem hiding this comment.
Sorry, a couple of things occurred to me about this test that might require more tweaks:
- I think you should pair a negative test ("can't find stat, or stat is 0") with a positive test to be sure that stats are recorded where expected
- Depending on the implementation of the string lookup method, you might actually get a counter returned for this when you don't expect it, because with your change to symbol_table_impl.cc the name will be corrected and then the stat found. So a better test might be to do the name lookup, expect that there's a counter, but get the counter's name back and test it against the expected string. I'm a little surprised that you don't find a match with
counter()which will convert the string to a StatName, eliding the empty segment. So I think your test might not be spelling the stat-name correctly, even ignoring the... In the new method, it will use a string-map maintained test/common/stats/stat_test_utility.cc and I'd expect the lookup to fail.
There was a problem hiding this comment.
This test doesn't really do what's intended due to this in codes.cc:
envoy/source/common/http/codes.cc
Line 107 in 491b116
So in #16675 I am just reverting this file.
| EXPECT_EQ("foo.x", encodeDecode(".foo.x")); | ||
| EXPECT_EQ("foo.x", encodeDecode(".foo..x")); | ||
| EXPECT_EQ("foo.x", encodeDecode("..foo.x")); | ||
| EXPECT_EQ("foo.x", encodeDecode("..foo..x")); |
There was a problem hiding this comment.
I'm not sure why these weren't added in the first place, but can you add some tests with trailing dots also?
|
Looks like test/common/stats:symbol_table_fuzz_test might need some tweaking as well for the new behavior. |
|
/wait |
2 participants
Commit Message: Prevents stats with invalid metric formats, where there are consecutive
..from being emitted.Additional Description: For example, drop stats with the format
cluster.egress_dynamodb_iad.zone.1a..upstream_rq_2xxRisk Level: Medium
Testing: Unit
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
Fixes #15541