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

dims · 2025-01-14T22:00:54Z

Reviving previous effort from: #17974

Added a new test to make sure we are not missing any expected metrics.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

k8s-ci-robot · 2025-01-14T22:01:04Z

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.

ivanvc · 2025-01-14T22:16:49Z

/ok-to-test

dims · 2025-01-14T22:18:37Z

thanks @ivanvc

codecov · 2025-01-14T22:28:36Z

Codecov Report

Attention: Patch coverage is 88.37209% with 5 lines in your changes missing coverage. Please review.

Project coverage is 68.83%. Comparing base (32cfd45) to head (c94b3be).

Files with missing lines	Patch %	Lines
server/etcdmain/grpc_proxy.go	0.00%	4 Missing ⚠️
server/etcdserver/api/v3rpc/metrics.go	96.55%	1 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
server/config/config.go	`80.23% <ø> (ø)`
server/embed/etcd.go	`76.79% <100.00%> (+0.30%)`	⬆️
server/etcdserver/api/v3rpc/grpc.go	`100.00% <100.00%> (ø)`
server/etcdserver/api/v3rpc/metrics.go	`97.05% <96.55%> (-2.95%)`	⬇️
server/etcdmain/grpc_proxy.go	`14.44% <0.00%> (-0.08%)`	⬇️

... and 23 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19195      +/-   ##
==========================================
- Coverage   68.83%   68.83%   -0.01%     
==========================================
  Files         420      420              
  Lines       35678    35714      +36     
==========================================
+ Hits        24560    24582      +22     
- Misses       9688     9709      +21     
+ Partials     1430     1423       -7

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 32cfd45...c94b3be. Read the comment docs.

dims · 2025-01-14T22:31:45Z

/test pull-etcd-integration-2-cpu-amd64

dims · 2025-01-15T01:35:07Z

/assign @ahrtr @serathius

server/embed/etcd.go

tests/integration/clientv3/examples/example_metrics_test.go

ahrtr · 2025-01-15T10:36:25Z

Thanks @dims for the PR.

I did some sanity test on this PR, and compared it with the existing main branch.

Confirmed that this PR can generate the same gRPC counter metrics as the existing main branch, including
- grpc_server_handled_total
- grpc_server_msg_received_total
- grpc_server_msg_sent_total
- grpc_server_started_total
It couldn't generate the histograms metrics, but the existing main branch can.
- grpc_server_handling_seconds_bucket
- grpc_server_handling_seconds_count
- grpc_server_handling_seconds_sum

Also references:

serathius · 2025-01-15T10:36:47Z

Do we need a test to confirm that no metric was removed?

dims · 2025-01-17T16:53:09Z

It couldn't generate the histograms metrics, but the existing main branch can.
grpc_server_handling_seconds_bucket
grpc_server_handling_seconds_count
grpc_server_handling_seconds_sum

@ahrtr did you run with --metrics 'extensive' option? or let it default to 'basic'?

dims · 2025-01-17T16:53:33Z

Do we need a test to confirm that no metric was removed?

i think so for future-proofing!

ahrtr · 2025-01-17T18:18:03Z

@ahrtr did you run with --metrics 'extensive' option?

YES, I executed the same command on this PR and the main branch. The main branch was working as expected, but this PR did not generate the histograms metrics.

k8s-ci-robot · 2025-01-17T19:44:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dims
Once this PR has been reviewed and has the lgtm label, please ask for approval from ahrtr. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

dims · 2025-01-17T19:47:15Z

@ahrtr found the issue and hopefully fixed it. Added a test as well. However, please check if i broke anything in the process of threading the option through to both the test suite and the main binary.

dims · 2025-01-17T20:29:07Z

/test pull-etcd-integration-2-cpu-amd64

dims · 2025-01-17T23:24:33Z

/test pull-etcd-robustness-arm64

dims · 2025-01-17T23:28:50Z

Do we need a test to confirm that no metric was removed?

@serathius Done! see new test case. If there are other metrics we can trigger them and then add to the list.

tests/integration/clientv3/metrics_test.go

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

@serathius @ahrtr the follow up PR once this merges will be https://github.com/etcd-io/etcd/pull/19242/files

dims · 2025-01-21T20:45:23Z

/test pull-etcd-unit-test-386

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

@serathius @ahrtr the follow up PR once this merges will be https://github.com/etcd-io/etcd/pull/19242/files

@dims please see my comment #19242 (comment), thx

…pc-middleware/providers/prometheus Signed-off-by: Davanum Srinivas <davanum@gmail.com>

Signed-off-by: Davanum Srinivas <davanum@gmail.com>

dims · 2025-01-23T18:05:44Z

@ahrtr after #19242 merged, i've rebased and updated this PR as well.

ahrtr · 2025-01-23T18:19:14Z

bill-of-materials.json

+		"project": "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors",
+		"licenses": [
+			{
+				"type": "\"Do What The F*ck You Want To Public License\"",


Do we know where is this coming from? @ivanvc @jmhbnz @dims

@ahrtr ./scripts/updatebom.sh is generating it

https://spdx.org/licenses/WTFPL.html

github.com/grpc-ecosystem/go-grpc-middleware/v2 doesn't use this license.

Looks like it's coming from https://github.com/appscodelabs/license-bill-of-materials/blob/master/assets/wtfpl.txt

@ahrtr correct! look at the confidence score "confidence": 0.14814814814814814

@ahrtr do you want me to add an entry in override.json?

ahrtr · 2025-01-23T18:25:06Z

tests/integration/clientv3/metrics_test.go

+		"etcd_disk_backend_commit_duration_seconds",
+		"etcd_disk_backend_defrag_duration_seconds",
+		"etcd_disk_backend_snapshot_duration_seconds",


This looks like a breaking change? It changes the metrics names.

nope! the _bucket, _count, _sum suffixes are dropped as i am parsing them correctly using the official parser.

you can see lines 266-268 have the correct names.

dims · 2025-01-23T18:30:57Z

tests/integration/clientv3/metrics_test.go

 		"grpc_client_handled_total",
 		"grpc_client_msg_received_total",
 		"grpc_client_msg_sent_total",
 		"grpc_client_started_total",
 		"grpc_server_handled_total",
+		"grpc_server_handling_seconds",


this is the extra metric when we turn on extensive

k8s-ci-robot added area/clientv3 area/testing labels Jan 14, 2025

k8s-ci-robot added needs-ok-to-test size/M labels Jan 14, 2025

dims force-pushed the switch-from-grpc-ecosystem/go-grpc-prometheus-to-grpc-ecosystem/go-grpc-middleware/providers/prometheus-take-2 branch from f7123cd to f80c5b2 Compare January 14, 2025 22:09

k8s-ci-robot added area/etcdctl area/etcdutl size/L and removed size/M labels Jan 14, 2025

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

k8s-ci-robot assigned ahrtr and serathius Jan 15, 2025

serathius reviewed Jan 15, 2025

View reviewed changes

server/embed/etcd.go Outdated Show resolved Hide resolved

ahrtr reviewed Jan 15, 2025

View reviewed changes

tests/integration/clientv3/examples/example_metrics_test.go Outdated Show resolved Hide resolved

dims force-pushed the switch-from-grpc-ecosystem/go-grpc-prometheus-to-grpc-ecosystem/go-grpc-middleware/providers/prometheus-take-2 branch from f80c5b2 to 03554ca Compare January 17, 2025 19:44

dims force-pushed the switch-from-grpc-ecosystem/go-grpc-prometheus-to-grpc-ecosystem/go-grpc-middleware/providers/prometheus-take-2 branch from 03554ca to cafe302 Compare January 17, 2025 20:02

dims force-pushed the switch-from-grpc-ecosystem/go-grpc-prometheus-to-grpc-ecosystem/go-grpc-middleware/providers/prometheus-take-2 branch from cafe302 to d3fe3e2 Compare January 17, 2025 20:09

dims force-pushed the switch-from-grpc-ecosystem/go-grpc-prometheus-to-grpc-ecosystem/go-grpc-middleware/providers/prometheus-take-2 branch 3 times, most recently from f1e0118 to c4bed32 Compare January 17, 2025 23:00

dims force-pushed the switch-from-grpc-ecosystem/go-grpc-prometheus-to-grpc-ecosystem/go-grpc-middleware/providers/prometheus-take-2 branch 4 times, most recently from efbc3fc to 742ce69 Compare January 18, 2025 03:42

serathius reviewed Jan 20, 2025

View reviewed changes

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

dims force-pushed the switch-from-grpc-ecosystem/go-grpc-prometheus-to-grpc-ecosystem/go-grpc-middleware/providers/prometheus-take-2 branch from 742ce69 to da7dd38 Compare January 21, 2025 12:07

dims mentioned this pull request Jan 21, 2025

Test for etcd metrics #19242

Merged

dims added 3 commits January 23, 2025 11:37

Switch from grpc-ecosystem/go-grpc-prometheus to grpc-ecosystem/go-gr…

e5257a6

…pc-middleware/providers/prometheus Signed-off-by: Davanum Srinivas <davanum@gmail.com>

Fix missing metrics and verify metrics are not lost

7585ba5

Signed-off-by: Davanum Srinivas <davanum@gmail.com>

use TextToMetricFamilies instead of custom parser

c94b3be

Signed-off-by: Davanum Srinivas <davanum@gmail.com>

dims force-pushed the switch-from-grpc-ecosystem/go-grpc-prometheus-to-grpc-ecosystem/go-grpc-middleware/providers/prometheus-take-2 branch from da7dd38 to c94b3be Compare January 23, 2025 17:11

ahrtr reviewed Jan 23, 2025

View reviewed changes

dims commented Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dims commented Jan 14, 2025 •

edited

Loading

k8s-ci-robot commented Jan 14, 2025

ivanvc commented Jan 14, 2025

dims commented Jan 14, 2025

codecov bot commented Jan 14, 2025 •

edited

Loading

dims commented Jan 14, 2025

dims commented Jan 15, 2025

ahrtr commented Jan 15, 2025

serathius commented Jan 15, 2025

dims commented Jan 17, 2025

dims commented Jan 17, 2025

ahrtr commented Jan 17, 2025

k8s-ci-robot commented Jan 17, 2025

dims commented Jan 17, 2025

dims commented Jan 17, 2025

dims commented Jan 17, 2025

dims commented Jan 17, 2025

dims commented Jan 21, 2025

dims commented Jan 21, 2025

ahrtr commented Jan 23, 2025

dims commented Jan 23, 2025

ahrtr Jan 23, 2025

dims Jan 23, 2025

dims Jan 23, 2025

ahrtr Jan 23, 2025

dims Jan 23, 2025

dims Jan 23, 2025

ahrtr Jan 23, 2025

dims Jan 23, 2025

dims Jan 23, 2025

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

Are you sure you want to change the base?

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

Conversation

dims commented Jan 14, 2025 • edited Loading

k8s-ci-robot commented Jan 14, 2025

ivanvc commented Jan 14, 2025

dims commented Jan 14, 2025

codecov bot commented Jan 14, 2025 • edited Loading

Codecov Report

dims commented Jan 14, 2025

dims commented Jan 15, 2025

ahrtr commented Jan 15, 2025

serathius commented Jan 15, 2025

dims commented Jan 17, 2025

dims commented Jan 17, 2025

ahrtr commented Jan 17, 2025

k8s-ci-robot commented Jan 17, 2025

dims commented Jan 17, 2025

dims commented Jan 17, 2025

dims commented Jan 17, 2025

dims commented Jan 17, 2025

dims commented Jan 21, 2025

dims commented Jan 21, 2025

ahrtr commented Jan 23, 2025

dims commented Jan 23, 2025

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

dims commented Jan 14, 2025 •

edited

Loading

codecov bot commented Jan 14, 2025 •

edited

Loading