Test for etcd metrics #19242

dims · 2025-01-21T12:10:16Z

Test ONLY change to ensure #19195 does not cause a regression.

NOTE: the following 3 metrics are NOT listed here as there is no way for us to thread in "extensive" metrics in current tests suite. #19195 has the changes needed for that. So when we update that PR, we will add these metrics as the PR has the changes for updating the tests suite.

grpc_server_handling_seconds_bucket
grpc_server_handling_seconds_count
grpc_server_handling_seconds_sum

k8s-ci-robot · 2025-01-21T12:10:29Z

Hi @dims. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

codecov · 2025-01-21T12:32:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.81%. Comparing base (7c56ef2) to head (19d405c).
Report is 23 commits behind head on main.

Additional details and impacted files

see 25 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19242      +/-   ##
==========================================
+ Coverage   68.77%   68.81%   +0.04%     
==========================================
  Files         420      420              
  Lines       35649    35675      +26     
==========================================
+ Hits        24518    24551      +33     
+ Misses       9705     9699       -6     
+ Partials     1426     1425       -1

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c56ef2...19d405c. Read the comment docs.

serathius · 2025-01-22T09:49:56Z

Can we first merge the test?

tests/integration/clientv3/metrics_test.go

ahrtr · 2025-01-23T11:10:31Z

The test being added here will fail until #19195 lands

The test should be an independent change. We should finish & merge the test firstly. Afterwards, you need to rebase #19195.

I think you just need to start an etcd instance (and probably write a couple of key/values), then get all the metrics using /metrics endpoint. Please refer to https://github.com/ahrtr/etcd-diagnosis/blob/main/plugins/metrics/plugin.go

dims · 2025-01-23T13:06:52Z

@serathius @ahrtr updated this PR, this can go first. 3 metrics are not captured currently in this test (see notes in PR text). When i update the other PR will get added.

serathius · 2025-01-23T13:53:16Z

tests/integration/clientv3/metrics_test.go

+	srv := &http.Server{Handler: promhttp.Handler()}
+	srv.SetKeepAlivesEnabled(false)
+
+	ln, err := transport.NewUnixListener(addr)
+	if err != nil {
+		t.Errorf("Error: %v occurred while listening on addr: %v", err, addr)
+	}
+
+	donec := make(chan struct{})
+	defer func() {
+		ln.Close()
+		<-donec
+	}()
+
+	// listen for all Prometheus metrics
+	go func() {
+		defer close(donec)
+
+		serr := srv.Serve(ln)
+		if serr != nil && !transport.IsClosedConnError(serr) {
+			t.Errorf("Err serving http requests: %v", serr)
+		}
+	}()
+
+	url := "unix://" + addr + "/metrics"


Thinking whether e2e test here would better, more accurate and would not require this custom code. But don't want to block on it.

serathius · 2025-01-23T13:55:39Z

tests/integration/clientv3/metrics_test.go

+	}
+}
+
+func getMetricsList(t *testing.T, url string) []string {


FYI "github.com/prometheus/common/expfmt" is package has a parser for metrics. Example usage https://github.com/kubernetes-sigs/metrics-server/blob/0dcee07ea614e8354ec740dfc3f528a572cacec4/test/e2e_test.go#L315-L329

@serathius I will update the implementation of this test as suggested in the other PR when i rebase!

ahrtr · 2025-01-23T14:03:08Z

This PR follows the existing integration test cases.

I am not sure how other maintainers think, but personally I don't think the existing integration tests on metrics (see below) are appropriate,

For integration test, I think it would be better to simulate the embedded use case (start etcd by calling StartEtcd); but the current integration test creates the EtcdServer, provision all listeners, etc. directly, it does too much hack on etcd itself. It's hard to maintain, also hard to ensure it's testing exactly the same implementation as production environment. Even in the test, it also does too much hack. (i.e. register/listen prometheus metrics, etc.)

etcd/server/embed/etcd.go

Line 110 in 2471e41

func StartEtcd(inCfg *Config) (e *Etcd, err error) {

Ideally, we should test etcd as a black box, so e2e test is much better. If we test it in integrate test, we should start etcd via StartEtcd.

Of course, it's not this PR's problem. So I am OK to let this PR in and we can discuss it separately and probably add an e2e test separately.

dims · 2025-01-23T15:14:44Z

So I am OK to let this PR in and we can discuss it separately and probably add an e2e test separately.

thanks @ahrtr !

dims · 2025-01-23T15:15:12Z

/ok-to-test

k8s-ci-robot · 2025-01-23T15:15:28Z

@dims: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

dims · 2025-01-23T15:17:46Z

/ok-to-test

Signed-off-by: Davanum Srinivas <[email protected]>

k8s-ci-robot · 2025-01-23T16:20:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, dims, serathius

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ahrtr,serathius]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added do-not-merge/work-in-progress area/testing needs-ok-to-test labels Jan 21, 2025

k8s-ci-robot added the size/L label Jan 21, 2025

ahrtr reviewed Jan 23, 2025

View reviewed changes

tests/integration/clientv3/metrics_test.go Outdated Show resolved Hide resolved

ahrtr mentioned this pull request Jan 23, 2025

Switch from grpc-ecosystem/go-grpc-prometheus to grpc-ecosystem/go-grpc-middleware/providers/prometheus #19195

Merged

dims force-pushed the test-for-etcd-metrics branch from f55b375 to 040fbaa Compare January 23, 2025 13:00

dims changed the title ~~[WIP] Test for etcd metrics~~ Test for etcd metrics Jan 23, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress label Jan 23, 2025

dims force-pushed the test-for-etcd-metrics branch from 040fbaa to 9a2cb80 Compare January 23, 2025 13:05

serathius reviewed Jan 23, 2025

View reviewed changes

serathius approved these changes Jan 23, 2025

View reviewed changes

k8s-ci-robot added the approved label Jan 23, 2025

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Jan 23, 2025

Test for etcd metrics

19d405c

Signed-off-by: Davanum Srinivas <[email protected]>

dims force-pushed the test-for-etcd-metrics branch from 9a2cb80 to 19d405c Compare January 23, 2025 15:30

ahrtr approved these changes Jan 23, 2025

View reviewed changes

ahrtr merged commit c8646cd into etcd-io:main Jan 23, 2025
34 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test for etcd metrics #19242

Test for etcd metrics #19242

dims commented Jan 21, 2025 •

edited

Loading

k8s-ci-robot commented Jan 21, 2025

codecov bot commented Jan 21, 2025 •

edited

Loading

serathius commented Jan 22, 2025

ahrtr commented Jan 23, 2025

dims commented Jan 23, 2025

serathius Jan 23, 2025

dims Jan 23, 2025

serathius Jan 23, 2025 •

edited

Loading

dims Jan 23, 2025

ahrtr commented Jan 23, 2025 •

edited

Loading

dims commented Jan 23, 2025

dims commented Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

dims commented Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

Test for etcd metrics #19242

Test for etcd metrics #19242

Conversation

dims commented Jan 21, 2025 • edited Loading

k8s-ci-robot commented Jan 21, 2025

codecov bot commented Jan 21, 2025 • edited Loading

Codecov Report

serathius commented Jan 22, 2025

ahrtr commented Jan 23, 2025

dims commented Jan 23, 2025

serathius Jan 23, 2025

Choose a reason for hiding this comment

dims Jan 23, 2025

Choose a reason for hiding this comment

serathius Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

dims Jan 23, 2025

Choose a reason for hiding this comment

ahrtr commented Jan 23, 2025 • edited Loading

dims commented Jan 23, 2025

dims commented Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

dims commented Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

dims commented Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

serathius Jan 23, 2025 •

edited

Loading

ahrtr commented Jan 23, 2025 •

edited

Loading