stats: add integration test to flag missing tag extraction rules by ggreenway · Pull Request #21397 · envoyproxy/envoy

ggreenway · 2022-05-20T16:28:44Z

Signed-off-by: Greg Greenway ggreenway@apple.com

Commit Message: This is a rough heuristic, but may catch many cases of missing rules.
Additional Description:
Risk Level: low: test only
Testing: this is a new test
Docs Changes: not needed
Release Notes: not needed
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

This is a rough heuristic, but may catch many cases of missing rules. Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only · 2022-05-20T16:28:47Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #21397 was opened by ggreenway.

see: more, trace.

ggreenway · 2022-05-20T16:32:01Z

@jmarantz after seeing many new stats added without correct tag extraction rules, I wrote this.

Pros:

Gives some awareness to developers if they've missed this, for the most obvious cases

Cons:

Adds some code to every single integration test. Will this cause a measurable degradation in build/test time?
Misses anything that doesn't use stat_prefix in the config
Misses anything that isn't configured in an integration test

What do you think? Is there a better way to do this, that is likely to be hit for new code with a developer who doesn't know about the tag extraction rules?

jmarantz

code seems fine ; I wasn't aware of this stat_prefix convention (or had forgotten) and so I think I'm not understanding the premise of what you are checking.

jmarantz · 2022-05-20T16:41:41Z

test/integration/base_integration_test.cc

+  std::vector<std::string> stat_prefixes;
+  Json::ObjectCallback find_stat_prefix = [&](const std::string& name,
+                                              const Json::Object& root) -> bool {
+    if (name == "stat_prefix") {


where does this magic "stat_prefix" come from? Could this be a string-constant?

stat_prefix is the conventional name in the config protos.

$ grep -R stat_prefix api/envoy/* api/envoy/api/v2/core/grpc_service.proto: string stat_prefix = 4 [(validate.rules).string = {min_bytes: 1}]; api/envoy/config/listener/v3/listener.proto: // `listener.<stat_prefix>.`. api/envoy/config/listener/v3/listener.proto: string stat_prefix = 28; api/envoy/config/core/v3/grpc_service.proto: string stat_prefix = 4 [(validate.rules).string = {min_len: 1}]; api/envoy/config/filter/udp/udp_proxy/v2alpha/udp_proxy.proto: string stat_prefix = 1 [(validate.rules).string = {min_bytes: 1}]; api/envoy/config/filter/network/rbac/v2/rbac.proto: string stat_prefix = 3 [(validate.rules).string = {min_bytes: 1}]; api/envoy/config/filter/network/tcp_proxy/v2/tcp_proxy.proto: string stat_prefix = 1 [(validate.rules).string = {min_bytes: 1}]; api/envoy/config/filter/network/mongo_proxy/v2/mongo_proxy.proto: string stat_prefix = 1 [(validate.rules).string = {min_bytes: 1}]; api/envoy/config/filter/network/zookeeper_proxy/v1alpha1/zookeeper_proxy.proto: string stat_prefix = 1 [(validate.rules).string = {min_bytes: 1}]; api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto: string stat_prefix = 1 [(validate.rules).string = {min_bytes: 1}]; api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto: string stat_prefix = 2 [(validate.rules).string = {min_bytes: 1}]; api/envoy/config/filter/network/dubbo_proxy/v2alpha1/dubbo_proxy.proto: string stat_prefix = 1 [(validate.rules).string = {min_bytes: 1}]; api/envoy/config/filter/network/ext_authz/v2/ext_authz.proto: string stat_prefix = 1 [(validate.rules).string = {min_bytes: 1}]; api/envoy/config/filter/network/local_rate_limit/v2alpha/local_rate_limit.proto: string stat_prefix = 1 [(validate.rules).string = {min_bytes: 1}]; api/envoy/config/filter/network/kafka_broker/v2alpha1/kafka_broker.proto: string stat_prefix = 1 [(validate.rules).string = {min_bytes: 1}]; api/envoy/config/filter/network/rate_limit/v2/rate_limit.proto: string stat_prefix = 1 [(validate.rules).string = {min_bytes: 1}]; api/envoy/config/filter/network/client_ssl_auth/v2/client_ssl_auth.proto: string stat_prefix = 2 [(validate.rules).string = {min_bytes: 1}]; api/envoy/config/filter/network/mysql_proxy/v1alpha1/mysql_proxy.proto: string stat_prefix = 1 [(validate.rules).string = {min_bytes: 1}]; api/envoy/config/filter/network/thrift_proxy/v2alpha1/thrift_proxy.proto: string stat_prefix = 1 [(validate.rules).string = {min_bytes: 1}]; api/envoy/extensions/matching/input_matchers/ip/v3/ip.proto: string stat_prefix = 2 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/udp/dns_filter/v3/dns_filter.proto: string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/udp/udp_proxy/v3/udp_proxy.proto: string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/network/rbac/v3/rbac.proto: string shadow_rules_stat_prefix = 5; api/envoy/extensions/filters/network/rbac/v3/rbac.proto: string stat_prefix = 3 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/network/local_ratelimit/v3/local_rate_limit.proto: string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/network/connection_limit/v3/connection_limit.proto: string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/network/tcp_proxy/v3/tcp_proxy.proto: string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/network/mongo_proxy/v3/mongo_proxy.proto: string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/network/zookeeper_proxy/v3/zookeeper_proxy.proto: string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto: string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto: string stat_prefix = 2 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/network/dubbo_proxy/v3/dubbo_proxy.proto: string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/network/ext_authz/v3/ext_authz.proto: string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/network/ratelimit/v3/rate_limit.proto: string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/network/client_ssl_auth/v3/client_ssl_auth.proto: string stat_prefix = 2 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/network/meta_protocol_proxy/v3/meta_protocol_proxy.proto: string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/network/thrift_proxy/v3/thrift_proxy.proto: string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/http/rbac/v3/rbac.proto: string shadow_rules_stat_prefix = 3; api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto: string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/http/compressor/v3/compressor.proto: // `<stat_prefix>.compressor.<compressor_library.name>.<compressor_library_stat_prefix>.response.*` api/envoy/extensions/filters/http/compressor/v3/compressor.proto: // `<stat_prefix>.compressor.<compressor_library.name>.<compressor_library_stat_prefix>.*`. api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto: // stat_prefix: waf # This emits ext_authz.waf.ok, ext_authz.waf.denied, etc. api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto: // stat_prefix: blocker # This emits ext_authz.blocker.ok, ext_authz.blocker.denied, etc. api/envoy/extensions/filters/http/ext_authz/v3/ext_authz.proto: string stat_prefix = 13; api/envoy/extensions/filters/http/bandwidth_limit/v3/bandwidth_limit.proto: string stat_prefix = 1 [(validate.rules).string = {min_len: 1}]; api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto: string stat_prefix = 8;

Maybe add a comment here that this is based on precedent but isn't guaranteed to be exhaustive? We might need to expand this in the future?

jmarantz · 2022-05-20T16:42:23Z

test/integration/base_integration_test.cc

+      }
+    }
+  };
+  check_metric_array(test_server_->counters());


nit; could use test_server_->forEachCounter here and void making the intermediate array. Not sure if that would be material.

jmarantz · 2022-05-20T17:05:14Z

test/integration/base_integration_test.cc

+    for (const auto& metric : vec) {
+      const std::string tag_extracted_name = metric->tagExtractedName();
+      for (const std::string& stat_prefix : stat_prefixes) {
+        EXPECT_EQ(tag_extracted_name.find(stat_prefix), std::string::npos)


I don't understand how just doing a total substring search for the prefixes in the tag-extracted name tells you whether the tag-extraction regexes are complete.

The idea is that if the stat_prefix is in the tag-extracted name, then it wasn't extracted, which I believe is always an error (although there are many such cases in the codebase right now).

Or at least that's how I use stat_prefix. Maybe others use it completely differently, and this isn't correct. I'd like to hear opinions on this.

Also, this assumes that the configured stat_prefix in the integration tests is a value that is unique enough that it isn't a substring of any other part of the stat name.

Another approach to this test, that may be more robust, is to instead look at only the extracted tags, and verify that the value configured for stat_prefix is in one of the extracted tags, and ignore the tagExtractedName. That may more clearly express what I'm trying to accomplish.

…traction

Signed-off-by: Greg Greenway <ggreenway@apple.com>

…traction

Signed-off-by: Greg Greenway <ggreenway@apple.com>

jmarantz · 2022-05-27T02:33:27Z

/retest

repokitteh-read-only · 2022-05-27T02:33:30Z

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

🐱

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

see: more, trace.

jmarantz · 2022-05-27T12:47:47Z

Greg should I review now or wait till CI is clean?

jmarantz · 2022-05-30T20:22:23Z

/wait

Signed-off-by: Greg Greenway <ggreenway@apple.com>

…traction

Signed-off-by: Greg Greenway <ggreenway@apple.com>

jmarantz

lgtm basically modulo minor nits.

Does it make sense to just get one other maintainer's opinion on this plan generally? I'll leave it up to you on all counts.

jmarantz · 2022-05-31T23:57:55Z

test/extensions/access_loggers/open_telemetry/access_log_integration_test.cc

-  AccessLogIntegrationTest() : HttpIntegrationTest(Http::CodecType::HTTP1, ipVersion()) {}
+  AccessLogIntegrationTest() : HttpIntegrationTest(Http::CodecType::HTTP1, ipVersion()) {
+    // TODO(ggreenway): add tag extraction rules.
+    // Missing stat tag-extraction rule for stat 'grpc.accesslog.streams_closed_1' and stat_prefix


is that the right stat-name or is this copy/pasted from another file?

I believe this is correct. A bunch of tests with outgoing grpc sideband connections have stats in the grpc. namespace.

jmarantz · 2022-06-01T00:05:20Z

test/integration/base_integration_test.cc

+  auto check_metric = [&](auto& metric) {
+    // Validate that the `stat_prefix` string doesn't appear in the tag-extracted name, indicating
+    // that it wasn't extracted.
+    const std::string tag_extracted_name = metric.tagExtractedName();


you didn't want to iterate over the tags instead? Maybe explain why in a code comment?

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway · 2022-06-01T15:38:08Z

Does it make sense to just get one other maintainer's opinion on this plan generally? I'll leave it up to you on all counts.

Yes, I think so. @mattklein123 what do you think of this approach?

mattklein123

Thanks this is awesome. I left some small comments. Can you also open a tracking beginner issue for fixing the missing extractions? Seems like a great one for new contributors.

/wait

mattklein123 · 2022-06-06T16:11:15Z

test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc

    return makeSslClientConnection({});
  };
+
+  // Disable for this test because it messes up the connection IDs.


Can you expand? Unclear how. Same elsewhere.

test/integration/base_integration_test.cc

mattklein123 · 2022-06-06T16:13:57Z

test/integration/base_integration_test.cc

+  std::vector<std::string> stat_prefixes;
+  Json::ObjectCallback find_stat_prefix = [&](const std::string& name,
+                                              const Json::Object& root) -> bool {
+    if (name == "stat_prefix") {


Maybe add a comment here that this is based on precedent but isn't guaranteed to be exhaustive? We might need to expand this in the future?

…traction

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway · 2022-06-06T20:10:29Z

Filed #21595 to track the remediation

mattklein123

Nice!

…oyproxy#21397) This is a rough heuristic, but may catch many cases of missing rules. Signed-off-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: Tianyu Xia <tyxia@google.com>

…oyproxy#21397) This is a rough heuristic, but may catch many cases of missing rules. Signed-off-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>

alyssawilk · 2022-11-03T20:57:54Z

hey @ggreenway @jmarantz can we have this off by default or maybe only on for one CI? It makes it significantly more complicated to debug integration tests when there's "spurious" e2e requests hitting the config dump URL

jmarantz · 2022-11-04T03:12:33Z

I'm not clear on why this would cause debug complexity but running it in only one CI seems OK to me.

alyssawilk · 2022-11-07T15:01:09Z

as the person who debugs most of our integration test flakes, it means for every test there's a ton of spurious data plane work going on which makes it significantly harder to debug what's actually broken. I've noticed this a number of times helping people who got confused on test debug and was inspired to root case recently.

jmarantz · 2022-11-07T15:18:01Z

this makes sense. Thanks Alyssa.

stats: add integration test to flag missing tag extraction rules

b3d1142

This is a rough heuristic, but may catch many cases of missing rules. Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway assigned jmarantz May 20, 2022

ggreenway changed the title ~~stats: add integration test to flag missing tag extraction rules~~ RFC: stats: add integration test to flag missing tag extraction rules May 20, 2022

jmarantz reviewed May 20, 2022

View reviewed changes

ggreenway mentioned this pull request May 24, 2022

ThriftProxy extension is not generating correct prometheus tags #20201

Closed

ggreenway added 3 commits May 24, 2022 11:55

Merge remote-tracking branch 'upstream/main' into test-missing-tag-ex…

7013b4b

…traction

comments, use forEachMetric, fixes

26eab11

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Add exceptions for existing failures with TODOs

a98e9aa

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway changed the title ~~RFC: stats: add integration test to flag missing tag extraction rules~~ stats: add integration test to flag missing tag extraction rules May 26, 2022

ggreenway marked this pull request as ready for review May 26, 2022 17:36

ggreenway requested review from alyssawilk, mattklein123, wbpcode, yanavlasov and zuercher as code owners May 26, 2022 17:36

ggreenway added 2 commits May 26, 2022 15:29

Merge remote-tracking branch 'upstream/main' into test-missing-tag-ex…

073e34e

…traction

more test fixes

3894614

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway requested a review from lizan as a code owner May 27, 2022 00:14

ggreenway added 5 commits May 31, 2022 11:48

Attempt to get more information when test fails early

b37a266

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge remote-tracking branch 'upstream/main' into test-missing-tag-ex…

2c95aa9

…traction

disble a test to fix windows CI

7cee9b1

Signed-off-by: Greg Greenway <ggreenway@apple.com>

spelling

7ca3766

Signed-off-by: Greg Greenway <ggreenway@apple.com>

fix the correct test

b651b9c

Signed-off-by: Greg Greenway <ggreenway@apple.com>

jmarantz previously approved these changes Jun 1, 2022

View reviewed changes

comments

12aa121

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway dismissed jmarantz’s stale review via 12aa121 June 1, 2022 15:37

ggreenway assigned mattklein123 Jun 1, 2022

mattklein123 requested changes Jun 6, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 6, 2022

ggreenway added 4 commits June 6, 2022 09:58

Merge remote-tracking branch 'upstream/main' into test-missing-tag-ex…

c75e614

…traction

better comment about connection IDs

0b46d5f

Signed-off-by: Greg Greenway <ggreenway@apple.com>

disable on a newly added test

8adc74f

Signed-off-by: Greg Greenway <ggreenway@apple.com>

add comment; don't allow json failures; will resolve in specific tests

0feef8b

Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only bot removed the waiting label Jun 6, 2022

ggreenway mentioned this pull request Jun 6, 2022

Tag extraction rules missing for many stats #21595

Open

24 tasks

mattklein123 approved these changes Jun 6, 2022

View reviewed changes

ggreenway merged commit ccf1f9c into envoyproxy:main Jun 6, 2022

alyssawilk mentioned this pull request Nov 7, 2022

don't run tag extraction checks by default #23863

Closed

Conversation

ggreenway commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented May 20, 2022

Uh oh!

ggreenway commented May 20, 2022

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!

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!

Choose a reason for hiding this comment

Uh oh!

jmarantz commented May 27, 2022

Uh oh!

repokitteh-read-only bot commented May 27, 2022

Uh oh!

jmarantz commented May 27, 2022

Uh oh!

jmarantz commented May 30, 2022

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!

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Jun 1, 2022

Uh oh!

mattklein123 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!

ggreenway commented Jun 6, 2022

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz commented Nov 4, 2022

Uh oh!

alyssawilk commented Nov 7, 2022

Uh oh!

jmarantz commented Nov 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ggreenway commented May 20, 2022 •

edited

Loading

alyssawilk commented Nov 3, 2022 •

edited

Loading