config: fix detailed metric keys missing in leading keys

[birdayz]

This patch fixes a bug in detailed metrics.

only the last descriptor uses the provided descriptor value in case of detailed metrics. when traversing the list of descriptors, the code "loses" the previous keys. this leads to metrics like:

descriptors = 
- key: first-key
  value: first-value
- key: second-key
  value: second-value
 
=> metric:

"test-domain.first-key_.second-key_second-value"

Value of the first descriptor is missing.
The last descriptor properly uses the detailed metric descriptor value, but all other descriptors (the first one here) are missing the value.

We would expect:

"test-domain.first-key_first_value.second-key_second-value"

this patch introduces a new string builder, that builds the detailed metric as the iteration of the input descriptor is happening. a unit test is attached to show the behavior. it fails without the new code, and successfully preserves all descriptor keys with the patched code.

added a few tests, i'm confident the change works as expected.

formatting changes were done by pre-commit. i'm a bit surprised, are these expected?

[birdayz]

@envoyproxy/ratelimit-maintainers it would be great if you can have a look. thank you!

[birdayz]

@mattklein123 @renuka-fernando excuse me for tagging directly. I am not sure if the suggested tag @envoyproxy/ratelimit-maintainers actually works, does not look like it.

Could you have a look? Thank you!

[birdayz]

@mattklein123 @renuka-fernando kind ping, if you could have a look.
thank you!

[mattklein123]

I do actually prefer your formatting changes, but it makes it hard to see the actual diff. Can you revert all of the formatting changes and just do the actual change? If you want to do a different PR with formatting changes only that is fine, thanks.

[birdayz]

@mattklein123 thanks for the feedback! i removed the formatting changes - could you review again?
thanks!

[mattklein123]

perf nit: can we only do this if detailed metrics are enabled?

[birdayz]

i fear it's not easily possible. only once we traversed the full "tree", we know if the leaf is a detailed metric (it can be hierarchical, and the leaf is the deciding factor).

so we build this string, even if we discard it later (line 364).

the previous solution loses this information because of exactly this reason (keys of previous descriptors in the "tree" are lost). so, basically, we need to memorize the path we have taken (including the keys AND values of the descriptors) we have seen, even if we drop it later.

i can think of two alternatives:

• we could traverse the tree again, after we have traversed it once, and see that the leaf has detailed metrics on (line 364). I haven't benchmarked, but i don't feel that this is faster than doing a bit of string concatenation (which cost most likely way less than the entire walk through the hierarchy)
• add a settings option to ratelimit-service, that steers detailed metrics in general. i.e. you can completely turn it off if you wish. however, even here we would spend a little more time doing this string concat, if somebody wants to have detailed metrics only for a small subset of their metrics.

I'm not sure if any of these is simpler than "just" doing the string concat and dropping later.

i tried to avoid refactoring this function more, because i am not very familiar with the codebase - and want to avoid introducing bugs i did not think of.

I personally think it should be fine to add this additional string concat (we do a few already, e.g. line 317). But of course, i'll ultimately defer to your judgment, how do you think about this?

[mattklein123]

Sure fair enough, thanks for the detailed explanation. SGTM.