stats: check emptiness in stripRegisteredPrefix before search.

[mathetake]

Signed-off-by: Takeshi Yoneda takeshi@tetrate.io

Commit Message: This commit checks the emptiness of CustomStatNamespaces in its stripRegisteredPrefix before doing string search to improve the performance in the case where no custom stat namespace is registered.

cc @jmarantz

Additional Description: NA
Risk Level: low
Testing: unittest
Docs Changes: NA
Release Notes: NA
Platform Specific Features: NA
[Optional Runtime guard:] NA
[Optional Fixes #Issue] NA
[Optional Deprecated:] NA

[wbpcode]

Why do we need to remove all custom namespaces? Wouldn't it be easier to distinguish if you keep the custom namespace as a prefix?

[mathetake]

no, the "custom name spaces" are just for envoy internal usage and not used for the user facing stuff. See #17357 and Prometheus endpoint is already removing them.

[wbpcode]

I checked #17357, but I am still confused. I agree with the value brought by custom namespace. It is also acceptable to customize stat sinks based on custom namespace.
But when outputting stats, all custom namespaces are directly removed, which I personally think is questionable.

For example, if I write a new extension and use custom namespace. Then I may also wish to use custom namespace as a prefix for all my custom metrics when outputting stats.

[wbpcode]

I mean, maybe we should make custom namespace more custom. We can do more control over the stats sink based on the custom namespace, such as removing the prefix, modifying the prefix, not outputting, and so on.

[mathetake]

You can just name your metrics as foo.myfavorite.bar and register foo as a custom stat name space. Then all the metrics are exposed as myfavorite.bar. And this is exactly what Wasm does and you can fill control over how your extension metrics are exposed.

[wbpcode]

You can just name your metrics as foo.myfavorite.bar and register foo as a custom stat name space. Then all the metrics are exposed as myfavorite.bar.

I personally feel it is a little bit strange. I need to prepare two custom names, one for registration and one for output (Or just repeat one name). But this is indeed a solution. Thanks.

[jmarantz]

I'm wondering what the semantic is that we really want for sinks other than Prometheus.

Do we really want to strip the custom namespace in those cases? I think it's done in Prometheus because the namespace is a separate part of the data model and we are trying to keep the Envoy and custom ones separate.

Maybe this needs to go into some doc (rst file) explaining what the semantics we are going for are.

[mathetake]

I think the semantics we follow is that "CustomStatNamespaces" are not visible to users - all custom stat namespaces should be only used by Envoy codebase and not perceivable for Envoy users. So maybe we should add more comment about this semantics rather than rst docs (since rst docs are for endusers, right?).

Also this is also about keeping backward compatibility for Wasm plugin users - previously they are able to expose their metrics in NON-prometheus /stats endpoint without being modified, but the current HEAD prefixes all of them with "wasmcustom". (Though the introduction of CustomStatNamespace has already broken the backward compatibility for /stats/prometheus endpoint :D).

wdyt?

[jmarantz]

Good point about "rst" being for end-users. How about in https://github.com/envoyproxy/envoy/blob/main/source/docs/stats.md ?

That's for developers. There's no "sink" section and there should be.

[mathetake]

Will do the doc change later.

/wait

[jmarantz]

From looking at the code I became unsure of whether it was going to achieve a good result, and so I thought it might make more sense to lead with doc so we can evaluate what it's doing (whereas when looking at the code I am sometimes overly focused on the "how").

Let's say someone writing a wasm filter creates a stat which is identical to a builtin one for Envoy. That'd be fine as long as it gets "wasmcustom." prepended. But if we just strip it, won't that create two stats with the same name for a sink? I might be misunderstanding what the strategy is here just from looking at the code, which is why I want doc :)

I understand why this makes sense in Prometheus as it has a namespace out-of-band with the stat name.

[jmarantz]

/wait

[mathetake]

OK now i've come to think that we don't need to do this change to text/json format..:D Thank you for taking a look. I will rework this only for the introduction of "empty()" and use it in prometheus stats.

[jmarantz]

sounds good!

/wait

[jmarantz]

nice & simple!

[jmarantz]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18127 (comment) was created by @jmarantz.

see: more, trace.