prometheus: allow stats to be opt out from automatic namespacing

[lizan]

Commit Message:
From envoyproxy/envoy-wasm#138. Just add the conventional prefix for now until we see some reason to make it configurable.

Additional Description:
Risk Level: Low
Testing: CI
Docs Changes: code level doc (not user facing)
Release Notes: N/A
Fixes #7957

[ggreenway]

Can you explain how this is used in practice? What happens if a stat is prefixed with _ and statsd is used instead of prometheus? Who is going to use this convention, and what if we need to change this for some reason?

[mandarjog]

@ggreenway istio stats extensions (s) use it to export metrics with metrics names that are arbitrarily namespaces.

For ex ‘Istio_requests_total’ would be ‘envoy_istio_request_total’ without this change.
Istio plugin exports it as _istio_requests_total.

[mandarjog]

@ggreenway see #7957

[lizan]

@ggreenway it doesn't affect statsd or other stuff, it allows extensions to register stats that not being automatically namespaced in prometheus stats. more context are in the issue.

[ggreenway]

@ggreenway it doesn't affect statsd or other stuff, it allows extensions to register stats that not being automatically namespaced in prometheus stats. more context are in the issue.

What I mean is that if you have stats starting with _, the prefix is stripped off for prometheus, but what if the user configured a different stats sink? Wouldn't the stats be prefixed with _?

In general, I'm not a fan of encoding things like this into strings. It would be better if a flag could be set on the stat when it is created, or possibly create some rules (regex based maybe?) for which stats should be treated differently. @jmarantz is there a way to keep this metadata with a stat, and set it on creation?

[jmarantz]

It should be possible (just annoying due to mocks) to use some spare bits in stats flags for things like this if they are deemed important enough. There are quite a few bits left now in padding. See

envoy/include/envoy/stats/stats.h

Line 96 in 5569744

static const uint8_t NeverImport = 0x04;

There a 16 flag bits of which we are using 3 now, and 2 of those are only used for Gauges.

[lizan]

What I mean is that if you have stats starting with _, the prefix is stripped off for prometheus, but what if the user configured a different stats sink? Wouldn't the stats be prefixed with _?

Yes it will be, it will be up to the extension to prefix or not.

In general, I'm not a fan of encoding things like this into strings. It would be better if a flag could be set on the stat when it is created, or possibly create some rules (regex based maybe?) for which stats should be treated differently. @jmarantz is there a way to keep this metadata with a stat, and set it on creation?

Configurable rules doesn't work well when an extension is loaded dynamically (e.g. wasm).

It should be possible (just annoying due to mocks) to use some spare bits in stats flags for things like this if they are deemed important enough. There are quite a few bits left now in padding. See

@jmarantz @ggreenway my take was it isn't important enough for a bit there, but if you feel so I can use that.

[jmarantz]

I wasn't voting on importance... Was commenting on feasibility :)

[mandarjog]

@ggreenway one requirement is that Changing Filter config thru XDS, does not require changing bootstrap, even with bootstrap

1. Add bootstrap static configuration to peel away _ in stats.
   This is api. We can add a stats.prometheus option to stop name spacing based on prefix.
   Cons: lots of new config
2. Use opencensus / OT which has APIs to do all this.
3. Use this convention that adds least amount of code.
4. Use spare bits per suggestion, but seems unclean.

[ggreenway]

Given that your use-case is for a filter to opt it's metrics out of namespacing, what if we added an API so that filters could register a prefix (at filter load time) to opt out of namespacing? So you could prefix all your metrics with myfiltername_, register that prefix with the admin handler through some API, and optionally have myfiltername_ stripped before the metric is emitted? Then we're not hard-coding a convention that may conflict with future changes we need to make. And no new configuration is needed, bootstrap or otherwise.

[mandarjog]

@ggreenway are you suggesting that a statsScope opts out of namespacing during scope creation?
Last time I checked, this information was not readily available inside the prometheus scraper.

But I do like the solution since it does not add any XDS or bootstrap config.
@lizan @jmarantz ?

[ggreenway]

@ggreenway are you suggesting that a statsScope opts out of namespacing during scope creation?
Last time I checked, this information was not readily available inside the prometheus scraper.

Not exactly. I'm suggesting that for the stats you want to exclude from namespacing, you come up with a prefix specific to your filter that you prepend on all your stat names, eg myfilter_. Then we add an API to the Server or somewhere similar where you can add to a list of prefixes that will not get the envoy_ prefix added on for prometheus stats. Then the code for generating prometheus stats will look similar to what it is in this PR now, except that instead of checking for _, it looks at a list of prefixes for if any of them match.

[mandarjog]

@ggreenway are you suggesting that a statsScope opts out of namespacing during scope creation?
Last time I checked, this information was not readily available inside the prometheus scraper.

Not exactly. I'm suggesting that for the stats you want to exclude from namespacing, you come up with a prefix specific to your filter that you prepend on all your stat names, eg myfilter_. Then we add an API to the Server or somewhere similar where you can add to a list of prefixes that will not get the envoy_ prefix added on for prometheus stats. Then the code for generating prometheus stats will look similar to what it is in this PR now, except that instead of checking for _, it looks at a list of prefixes for if any of them match.

The details of the prefix_list is populated will make a difference.
But I can see us calling registerPrometheusPrefixList() somewhere in the beginning. This will have to be called on a singleton.

@lizan @jplevyak wdyt?

[jplevyak]

We can call registerPrometheusPrefixList the first time any Wasm module loads.

[ggreenway]

We can call registerPrometheusPrefixList the first time any Wasm module loads.

Yes, that sounds find. If that's tricky to track, you could maybe use something like absl::call_once. Also, I'd name the function registerPrometheusNamespacePrefix.

[ggreenway]

/wait

[ggreenway]

I like this approach. Looks good overall, just a few minor points, and CI needs to be fixed.

/wait

[lizan]

@ggreenway ping?