stats: Add universal stats tag from CLI by davinci26 · Pull Request #18668 · envoyproxy/envoy

davinci26 · 2021-10-18T22:15:02Z

Commit Message:

Fixes #1975
Adds a CLI parameter that can be repeated that acts as universal tags for all stats

Additional Description:
Risk Level: Low, additional parameter
Testing: Unit + integration + manual
Docs Changes: Provided
Release Notes: Pending
Platform Specific Features: N/A

Signed-off-by: Sotiris Nanopoulos sonanopo@microsoft.com

repokitteh-read-only · 2021-10-18T22:15:09Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #18668 was opened by davinci26.

see: more, trace.

davinci26 · 2021-10-18T22:35:47Z

/retest

repokitteh-read-only · 2021-10-18T22:35:50Z

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

🐱

Caused by: a #18668 (comment) was created by @davinci26.

see: more, trace.

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 · 2021-10-19T23:35:56Z

I am investigating the failure in the memory tests but the PR should be ready to review while I fix the failing test

jmarantz

Looks fine; really just a collection of small nits.

jmarantz · 2021-10-20T12:34:24Z

docs/root/operations/cli.rst

+
+.. option:: --stats-tag
+
+  *(optional)* This flag provides a universal tag for all stats generated by Envoy. The format is ``stat:value``.


Should we have restrictions about what sort of characters are allowed in stat or value here?

Obviously : cannot be allowed in stat. What about .?

Are multiple values for the same tag allowed, e.g. --stat foo:bar --stat foo:baz ? From the impl it looks like the answer is no so should we specify it here?

Should we have restrictions about what sort of characters are allowed in stat or value here?

From what I saw in the docs for TagSpecifier there shouldn't be any limitations.

Are multiple values for the same tag allowed, e.g. --stat foo:bar --stat foo:baz ?
I need to call out explicitly that duplicates are not allowed

source/common/config/utility.h

envoy/server/options.h

source/server/options_impl.cc

test/common/config/utility_test.cc

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

source/server/options_impl.cc

envoy/server/options.h

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

test/common/config/utility_test.cc

source/common/stats/tag_producer_impl.cc

source/server/options_impl.cc

test/common/config/utility_test.cc

jmarantz · 2021-10-22T13:54:21Z

/wait

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

jmarantz · 2021-10-25T01:28:41Z

/wait

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

source/server/options_impl.cc

test/server/options_impl_test.cc

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

repokitteh-read-only · 2021-10-29T01:40:58Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #18668 was synchronize by davinci26.

see: more, trace.

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

jmarantz

Nice. Looking much better. A few minor nits remain.

jmarantz · 2021-11-01T23:00:19Z

source/common/stats/tag_utility.cc

 }
+
+bool isTagValueValid(absl::string_view name) {
+  std::regex regex{Config::NAME_REGEX, std::regex::optimize};


In the existing code, NAME_REGEX is evaluated by RE2, not std::regex, so if you want consistency you should. use RE2 to test it.

source/common/config/well_known_names.h

jmarantz · 2021-11-01T23:05:47Z

source/common/stats/tag_utility.cc

+bool isTagValueValid(absl::string_view name) {
+  std::regex regex{Config::NAME_REGEX, std::regex::optimize};
+  int cntr = 0;
+  for (auto i = std::regex_iterator<absl::string_view::iterator>(name.begin(), name.end(), regex);


rather than iterating I'd just use a regex with begin/end anchors.

jmarantz · 2021-11-01T23:08:26Z

source/server/options_impl.cc

-      "However, multiple stats with different values are not allowed.",
+      "``tag:value``. Only alphanumeric values are allowed for tag names. For tag values all "
+      "characters are permitted except for '.' (dot). This flag can be repeated multiple times to "
+      "set multiple universal tags. However, multiple stats with different values are not allowed.",


Maybe this is enough to establish the "envoy standard" but let's make sure @ggreenway and @snowp buy into this syntax.

I wasn't sure what you meant by the last sentence. Did you mean "multiple values for the same tag name are not allowed"?

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 · 2021-11-02T16:45:01Z

/retest

repokitteh-read-only · 2021-11-02T16:45:06Z

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

🐱

Caused by: a #18668 (comment) was created by @davinci26.

see: more, trace.

davinci26 · 2021-11-02T16:45:13Z

(unrelated test issues)

jmarantz · 2021-11-02T17:08:06Z

source/common/config/well_known_names.cc

 namespace {

+const absl::string_view NAME_REGEX = R"([^\.]+)";
+


constexpr absl::string_view TAG_VALUE_REGEX = R"([^\.]+)";

and s/NAME_REGEX/TAG_VALUE_REGEX throughout?

jmarantz · 2021-11-02T17:09:16Z

source/common/config/well_known_names.cc


 } // namespace

+const Regex::CompiledGoogleReMatcher& validTagValue() {


expose a bool function rather than returning the complex interface which the caller then has to call. This also allows the freedom of changing the implementation without changing the uses.

You will still need a function that returns the interface as a private helper.

expose a bool function rather than returning the complex interface which the caller then has to call. This also allows the freedom of changing the implementation without changing the uses.

This is basically what I am doing but the function with the bool signature is in tag_utility header. I like it there more because it seems more intuitive but not a strong opinion

That makes sense -- and you can have an interface in tag_utility.h, which can be an inline call to a bool method exposed here.

But exposing a CompiledGoogleReMatcher& here is a thicker coupling than necessary, IMO.

jmarantz · 2021-11-02T17:10:23Z

source/common/config/well_known_names.cc

 } // namespace

+const Regex::CompiledGoogleReMatcher& validTagValue() {
+  CONSTRUCT_ON_FIRST_USE(Regex::CompiledGoogleReMatcher, std::string(NAME_REGEX) + "$", false);


absl::StrCat("^", NAME_REGEX, "$")

this includes the begin-anchor, and absl::StrCat pre-allocates the right-size string for the 3-arg concat.

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

jmarantz

2 tiny nits remaining. This looks great.

I think I'll ask @ggreenway to do the final review as he may have an opinion about tag syntax.

source/common/config/well_known_names.cc

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 · 2021-11-02T18:48:57Z

thanks for all the reviews, it indeed looks much better now @jmarantz

ggreenway

Looks good overall. A few nits.

/wait

api/envoy/admin/v3/server_info.proto

source/server/options_impl.cc

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

ggreenway

LGTM

davinci26 · 2021-11-03T14:36:26Z

/retest

repokitteh-read-only · 2021-11-03T14:36:29Z

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

🐱

Caused by: a #18668 (comment) was created by @davinci26.

see: more, trace.

- as of envoyproxy/envoy#18668 `Envoy::Config::Utility::createTagProducer` now takes a new parameter `Stats::TagVector`. - removed workaround that allowed the HTTP3 integration test to pass while Envoy assumed that `--max-concurrent-streams` is always 100. This was fixed in envoyproxy/envoy#18879. - no changes in `.bazelrc`, `.bazelversion` or `ci/run_envoy_docker.sh`. Signed-off-by: Jakub Sobon <mumak@google.com>

- as of envoyproxy/envoy#18668 `Envoy::Config::Utility::createTagProducer` now takes a new parameter `Stats::TagVector`. - removed workaround that allowed the HTTP3 integration test to pass while Envoy assumed that `--max-concurrent-streams` is always 100. This was fixed in envoyproxy/envoy#18879. - updated `tools/update_cli_readme_documentation.py` to match on custom markers rather than the backtick character, since one of the flags now contains backticks in its description. Marked sections for replacement with the custom edit markers in our documents. - executed `update_cli_readme_documentation --mode fix`. - no changes in `.bazelrc`, `.bazelversion` or `ci/run_envoy_docker.sh`. Signed-off-by: Jakub Sobon <mumak@google.com>

repokitteh-read-only bot added the api label Oct 18, 2021

repokitteh-read-only bot assigned markdroth Oct 18, 2021

davinci26 assigned anrossi and jmarantz Oct 18, 2021

Fixes envoyproxy#1975

44bc363

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 force-pushed the statsPrefix branch from 5d5b48c to 44bc363 Compare October 19, 2021 19:07

Sotiris Nanopoulos added 3 commits October 19, 2021 14:13

fix remaining compilation issues

fe4dc28

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix formatting

47213a7

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix benchmark test

01af8ae

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

jmarantz reviewed Oct 20, 2021

View reviewed changes

Address comments part 1

36f26c7

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 commented Oct 20, 2021

View reviewed changes

source/server/options_impl.cc Outdated Show resolved Hide resolved

jmarantz reviewed Oct 21, 2021

View reviewed changes

source/server/options_impl.cc Outdated Show resolved Hide resolved

source/server/options_impl.cc Outdated Show resolved Hide resolved

source/server/options_impl.cc Outdated Show resolved Hide resolved

envoy/server/options.h Outdated Show resolved Hide resolved

Sotiris Nanopoulos added 3 commits October 21, 2021 11:31

change to use Stats::TagVector, verifying format

6593ad3

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

formating and reverting changes in tests

3786950

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix order in includes

08fe30b

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

jmarantz reviewed Oct 22, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 22, 2021

fix all comments. Still using walnum

da5398b

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

repokitteh-read-only bot removed the waiting label Oct 25, 2021

proposed limitation to only . and whitespace characters

7db7ea5

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

jmarantz reviewed Oct 27, 2021

View reviewed changes

source/server/options_impl.cc Outdated Show resolved Hide resolved

test/server/options_impl_test.cc Outdated Show resolved Hide resolved

use absl version of isspace

4f0dd6f

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

repokitteh-read-only bot removed the waiting label Nov 1, 2021

fix whitespace change

76a51de

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

jmarantz reviewed Nov 1, 2021

View reviewed changes

Sotiris Nanopoulos added 2 commits November 1, 2021 19:11

avoid static variables and use re2

0511d8a

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

remove commented out code

9185f89

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

jmarantz reviewed Nov 2, 2021

View reviewed changes

Sotiris Nanopoulos added 2 commits November 2, 2021 11:10

make the interface depend on bool values rather than regex impl

b50bef6

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

format

1fd046a

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

jmarantz previously approved these changes Nov 2, 2021

View reviewed changes

source/common/config/well_known_names.cc Show resolved Hide resolved

source/common/config/well_known_names.cc Outdated Show resolved Hide resolved

jmarantz assigned ggreenway Nov 2, 2021

fix two nits

1c12312

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 dismissed jmarantz’s stale review via 1c12312 November 2, 2021 18:45

jmarantz previously approved these changes Nov 2, 2021

View reviewed changes

ggreenway requested changes Nov 2, 2021

View reviewed changes

api/envoy/admin/v3/server_info.proto Show resolved Hide resolved

source/server/options_impl.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Nov 2, 2021

switch to string split instead of find

efd3b5a

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 dismissed jmarantz’s stale review via efd3b5a November 2, 2021 19:55

repokitteh-read-only bot removed the waiting label Nov 2, 2021

ggreenway approved these changes Nov 2, 2021

View reviewed changes

jmarantz approved these changes Nov 2, 2021

View reviewed changes

jmarantz merged commit c75c141 into envoyproxy:main Nov 4, 2021

mum4k mentioned this pull request Nov 9, 2021

Update envoy to a5a4251 (November 8, 2021). envoyproxy/nighthawk#758

Merged


		.. option:: --stats-tag

		(optional) This flag provides a universal tag for all stats generated by Envoy. The format is ``stat:value``.

		namespace {

		const absl::string_view NAME_REGEX = R"([^\.]+)";


		} // namespace

		const Regex::CompiledGoogleReMatcher& validTagValue() {

Conversation

davinci26 commented Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Oct 18, 2021

Uh oh!

davinci26 commented Oct 18, 2021

Uh oh!

repokitteh-read-only bot commented Oct 18, 2021

Uh oh!

davinci26 commented Oct 19, 2021

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarantz commented Oct 22, 2021

Uh oh!

jmarantz commented Oct 25, 2021

Uh oh!

Uh oh!

Uh oh!

repokitteh-read-only bot commented Oct 29, 2021

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davinci26 commented Nov 2, 2021

Uh oh!

repokitteh-read-only bot commented Nov 2, 2021

Uh oh!

davinci26 commented Nov 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davinci26 commented Nov 2, 2021

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

davinci26 commented Oct 18, 2021 •

edited

Loading