stats: introduce CustomStatNamespaces. #17357

mathetake · 2021-07-15T07:24:30Z

This commit adds Stats::CustomStatNamespaces, and the concept of custom stat namespaces used by
extensions and stat sinks. Custom stat namespaces are registered by extensions
via the Stats::CustomStatNamespaces object owned by the API object, and user-defined metrics
created by these extensions are all prefixed by the namespace. For example,
Wasm extension registers "wasmcustom" as a custom stat namespace,
and all the metrics created by user Wasm programs are prefixed by "wasmcustom." internally.
This is mainly for distinguishing these "custom metrics" defined outside Envoy codebase from
the native metrics defined by Envoy codebase, and this way stat sinks are able to determine
how to expose these two kinds of metrics.

Notably this commit fixes #14920 and istio/istio#27635 by making changes to
prometheus stat handler so that it strips the metrics names prefixed
by custom stat namespaces and exposes the stripped ones as-is in the output.
Previously in the prometheus stat handler, Envoy prefixes all the metrics by "envoy_" for those who do not have
the prefixes registered statistically via PrometheusStatsFormatter::registerPrometheusNamespace.
That was not a good UX because, for example, Wasm extension users cannot register their own
prometheus namespaces at runtime and therefore they end up having
all of their own metrics prefixed by "envoy_" unless they have their own Envoy builds.
Consequently, we no longer need the static prometheus namespace registration mechanism,
so this commit also deletes the unneeded API.

Signed-off-by: Takeshi Yoneda [email protected]

mathetake · 2021-07-15T07:26:34Z

@lizan could you take a quick view to see if my approach to thread-safety looks ok? I guess something like "dispatch on main thread" would be another approach rather than taking mutex like this, but I couldn't find the proper way to do that.

lizan

I'm not keen into this approach, doesn't the wasm foreign function implementation has access to dispatcher?

source/server/admin/prometheus_stats.cc

mathetake · 2021-07-29T23:59:41Z

yeah actually we can access to the main thread dispatcher here. Let me rework later.

mathetake · 2021-07-30T09:55:55Z

~~will do after the refactoring work here: proxy-wasm/proxy-wasm-cpp-host#183~~

mathetake · 2021-08-02T01:30:36Z

changed so we dispatch on main, rather than taking locks.

PiotrSikora · 2021-08-03T03:55:06Z

Since the overall goal seems to be avoiding the envoy_ prefix in Wasm metrics, couldn't we automatically do that for all metrics defined in Wasm, instead of adding those implementation-specific hostcalls?

mathetake · 2021-08-03T03:57:41Z

Since the overall goal seems to be avoiding the envoy_ prefix in Wasm metrics, couldn't we automatically do that for all metrics defined in Wasm, instead of adding those implementation-specific hostcalls?

ah that's good point and I think it's better UX. Let me rework.

PiotrSikora · 2021-08-03T06:54:55Z

source/extensions/common/wasm/context.cc

+      // Register the whole name if we couldn't find "_" in it.
+      [ns = stat_namespace.empty() ? std::string(name) : std::string(stat_namespace)]() -> void {
+        Server::PrometheusStatsFormatter::registerPrometheusNamespace(ns);
+      });


I don't know enough about the current stats to know for sure, but isn't this too greedy? e.g. if someone defines cluster_foo metric in Wasm, wouldn't it automatically remove envoy_ prefix from all "native" metrics as well?

yeah that's true.. maybe better have something like "is_native" flag in Metric class, and prefix "envoy_" based on it, rather than using registerPrometheusNamespace?

Yeah, that sounds good to me... although, it seems that such solution was discussed in #11808, but the prefix list was added instead. cc @lizan @mandarjog @ggreenway @jmarantz

I'm not familiar with wasm APIs at all, but could you require the plugin to register it's namespace, and clearly document that it should be specific to this plugin and not overlap with any existing native stat names, and that all plugin stats should be under that namespace? There's still the possibility of a plugin incorrectly registering for an incorrect prefix, but there's at least documentation about how to behave correctly to avoid such issues.

I guess my point is why do we have those "namespaces" in the first place, when in reality it's only about separating native Envoy metrics that append envoy_ prefix from those that don't?

Sorry I'm on vacation but I noticed this.

Oh sorry to interrupt you, and thanks for taking your time.

If this is really specific to wasm then it seems unclean to put a bit in the general stats structure to handle a special case.

This bit, say "is_custom_metric" flag, will be only set by Wasm for now since afaik Wasn extension is the only extension which defines non-Envoy-native metrics dynamically. But in the future, maybe other extensions would have that ability to define custom metrics.

ping @jmarantz , wdyt ^^? Do you think the purpose is not worth a bit?

@mathetake I think that we should do it, using flags to differentiate between Envoy's internal and Wasm/Lua/whatever metrics is much better UX than the need to register and use Prometheus namespaces.

I'm fine with this.

OK thank you all, I will rework this PR and add a "is_custom_metric" flag in Metric class to differentiate Envoy native metrics from others.

source/server/admin/prometheus_stats.cc

test/per_file_coverage.sh

mathetake · 2021-08-24T00:35:08Z

coverage failure in http/cache seems caused by this https://github.com/envoyproxy/envoy/runs/3402140077 https://dev.azure.com/cncf/envoy/_build/results?buildId=86179&view=logs&j=bbe4b42d-86e6-5e9c-8a0b-fea01d818a24&t=e00c5a13-c6dc-5e9a-6104-69976170e881 .. will raise another PR: #17817

mathetake · 2021-08-24T00:41:54Z

anyway I believe this is ready for review.

jmarantz · 2021-09-10T12:24:06Z

Sorry, one more thing: please refrain from force-pushing in Envoy github; that has the property of dropping review comments. I have never found it necessary to force-push in this workflow other than for fixing DCO.

mathetake · 2021-09-10T12:25:58Z

sorry the one is actually for DCO - I used git revert xxx for reverting the DynamicName stuff (and pushed without realizing that would miss signed-off).

* return optional<view> for stripCustom... * use StatNamePool instead of *Set * add a TODO in prometheus_stats.cc for safer regex * not use inlined CustomStatNamespaces in tests Signed-off-by: Takeshi Yoneda <[email protected]>

mathetake · 2021-09-10T12:53:27Z

How can we continue the dialog about getting a symbol-table-friendly way of making stats in wasm so we don't have catastrophic contention at scale?

I will open an issue for this! cc @PiotrSikora Do you want to say anything?

Signed-off-by: Takeshi Yoneda <[email protected]>

jmarantz

Looks great; couple of minor points and one concern around lifetime in your new StatName member var. But assuming at least the lifetime issue is commented on or just made safe by inspection, this is good from my end, and I'm ready for @mattklein123 to take another look.

source/extensions/common/wasm/wasm.cc

source/server/admin/prometheus_stats.cc

test/server/admin/prometheus_stats_test.cc

…pace

Signed-off-by: Takeshi Yoneda <[email protected]>

mattklein123

Thanks for iterating on this!

mathetake · 2021-09-14T00:41:39Z

Thank you so much for your reviews! @mattklein123 @jmarantz

mergeconflict · 2021-09-14T16:39:31Z

envoy/stats/custom_stat_namespaces.h

+
+#include "envoy/common/pure.h"
+
+#include "absl/container/flat_hash_set.h"


unused here (only used in the impl)

Following the change in the Envoy upstream: envoyproxy/envoy#17357 Signed-off-by: Takeshi Yoneda <[email protected]>

This was referenced Jul 15, 2021

wasm: allow customization of prometheus namespaces #14920

Closed

wasm: stat prefix is hard-coded istio/istio#27635

Closed

wasm: remove unused Istio specific Wasm code under src. istio/proxy#3413

Merged

mathetake marked this pull request as ready for review July 22, 2021 13:27

mathetake requested review from lizan and PiotrSikora as code owners July 22, 2021 13:27

htuch assigned PiotrSikora and lizan Jul 23, 2021

lizan reviewed Jul 29, 2021

View reviewed changes

source/server/admin/prometheus_stats.cc Outdated Show resolved Hide resolved

mathetake requested a review from lizan August 2, 2021 01:30

mathetake changed the title ~~wasm: add foreign calls to un/register prometheus namespaces.~~ wasm: register prometheus namespaces for Wasm plugin defined metrics. Aug 3, 2021

PiotrSikora reviewed Aug 3, 2021

View reviewed changes

lizan reviewed Aug 3, 2021

View reviewed changes

source/server/admin/prometheus_stats.cc Outdated Show resolved Hide resolved

lizan added the waiting label Aug 12, 2021

mathetake marked this pull request as draft August 23, 2021 14:18

mathetake force-pushed the wasm-prometheus-namespace branch from e8b4a24 to a8536dc Compare August 23, 2021 14:58

repokitteh-read-only bot removed the waiting label Aug 23, 2021

mathetake changed the title ~~wasm: register prometheus namespaces for Wasm plugin defined metrics.~~ stats: introduce is_custom_metric flag. Aug 23, 2021

jmarantz reviewed Aug 23, 2021

View reviewed changes

test/per_file_coverage.sh Outdated Show resolved Hide resolved

mathetake marked this pull request as ready for review August 24, 2021 00:41

mathetake requested review from jmarantz and PiotrSikora August 24, 2021 00:42

following reviews

563dcc2

* return optional<view> for stripCustom... * use StatNamePool instead of *Set * add a TODO in prometheus_stats.cc for safer regex * not use inlined CustomStatNamespaces in tests Signed-off-by: Takeshi Yoneda <[email protected]>

mathetake added 5 commits September 10, 2021 21:55

fix tests.

47abd9b

Signed-off-by: Takeshi Yoneda <[email protected]>

fix tests part2

2248ed5

Signed-off-by: Takeshi Yoneda <[email protected]>

review: add isMainThread assertions.

4415936

Signed-off-by: Takeshi Yoneda <[email protected]>

remove unnecessary copy of string

d0049de

Signed-off-by: Takeshi Yoneda <[email protected]>

fix tests part3

a05537f

Signed-off-by: Takeshi Yoneda <[email protected]>

jmarantz previously approved these changes Sep 10, 2021

View reviewed changes

source/extensions/common/wasm/wasm.cc Outdated Show resolved Hide resolved

source/server/admin/prometheus_stats.cc Outdated Show resolved Hide resolved

test/server/admin/prometheus_stats_test.cc Outdated Show resolved Hide resolved

mathetake added 2 commits September 10, 2021 23:27

Merge remote-tracking branch 'origin/main' into wasm-prometheus-names…

f8b8d53

…pace

following reviews.

b744586

Signed-off-by: Takeshi Yoneda <[email protected]>

mathetake dismissed jmarantz’s stale review via b744586 September 10, 2021 23:40

fix typo

6b9408c

Signed-off-by: Takeshi Yoneda <[email protected]>

mathetake requested a review from mattklein123 September 10, 2021 23:55

add more comment

5c735e9

Signed-off-by: Takeshi Yoneda <[email protected]>

jmarantz approved these changes Sep 13, 2021

View reviewed changes

jmarantz mentioned this pull request Sep 13, 2021

Prometheus stats for listener with stat_prefix set have wrong labels/extraction #16103

Closed

mattklein123 approved these changes Sep 13, 2021

View reviewed changes

mattklein123 merged commit 0bbf23c into envoyproxy:main Sep 13, 2021

bianpengyuan mentioned this pull request Sep 13, 2021

Automator: update envoy@ in istio/proxy@master istio/proxy#3491

Merged

mathetake deleted the wasm-prometheus-namespace branch September 13, 2021 23:30

This was referenced Sep 14, 2021

Update Envoy SHA to 09-13. istio/proxy#3493

Merged

stats: use re2 for prometheus metrics sanitization. #18110

Merged

mergeconflict reviewed Sep 14, 2021

View reviewed changes

This was referenced Sep 15, 2021

stats: check emptiness in stripRegisteredPrefix before search. #18127

Merged

test: use prometheus endpoint for metrics test. tetratelabs/proxy-wasm-go-sdk#212

Merged

mathetake added a commit to tetratelabs/proxy-wasm-go-sdk that referenced this pull request Sep 17, 2021

test: use prometheus endpoint for metrics test. (#212)

de254aa

Following the change in the Envoy upstream: envoyproxy/envoy#17357 Signed-off-by: Takeshi Yoneda <[email protected]>

karanvasnani mentioned this pull request Nov 23, 2021

Inconsistent handling of Stat Prefix between StatsD sinks and prometheus when a Custom Stat Namespace is registered. #19087

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stats: introduce CustomStatNamespaces. #17357

stats: introduce CustomStatNamespaces. #17357

mathetake commented Jul 15, 2021 •

edited

Loading

mathetake commented Jul 15, 2021

lizan left a comment

mathetake commented Jul 29, 2021

mathetake commented Jul 30, 2021 •

edited

Loading

mathetake commented Aug 2, 2021

PiotrSikora commented Aug 3, 2021

mathetake commented Aug 3, 2021

PiotrSikora Aug 3, 2021

mathetake Aug 3, 2021

PiotrSikora Aug 3, 2021

ggreenway Aug 3, 2021

PiotrSikora Aug 3, 2021

mathetake Aug 11, 2021

mathetake Aug 21, 2021

PiotrSikora Aug 21, 2021

jmarantz Aug 21, 2021

mathetake Aug 22, 2021

mathetake commented Aug 24, 2021 •

edited

Loading

mathetake commented Aug 24, 2021

jmarantz commented Sep 10, 2021

mathetake commented Sep 10, 2021 •

edited

Loading

mathetake commented Sep 10, 2021 •

edited

Loading

jmarantz left a comment

mattklein123 left a comment

mathetake commented Sep 14, 2021

mergeconflict Sep 14, 2021


		#include "envoy/common/pure.h"

		#include "absl/container/flat_hash_set.h"

stats: introduce CustomStatNamespaces. #17357

stats: introduce CustomStatNamespaces. #17357

Conversation

mathetake commented Jul 15, 2021 • edited Loading

mathetake commented Jul 15, 2021

lizan left a comment

Choose a reason for hiding this comment

mathetake commented Jul 29, 2021

mathetake commented Jul 30, 2021 • edited Loading

mathetake commented Aug 2, 2021

PiotrSikora commented Aug 3, 2021

mathetake commented Aug 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathetake commented Aug 24, 2021 • edited Loading

mathetake commented Aug 24, 2021

jmarantz commented Sep 10, 2021

mathetake commented Sep 10, 2021 • edited Loading

mathetake commented Sep 10, 2021 • edited Loading

jmarantz left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

mathetake commented Sep 14, 2021

Choose a reason for hiding this comment

mathetake commented Jul 15, 2021 •

edited

Loading

mathetake commented Jul 30, 2021 •

edited

Loading

mathetake commented Aug 24, 2021 •

edited

Loading

mathetake commented Sep 10, 2021 •

edited

Loading

mathetake commented Sep 10, 2021 •

edited

Loading