-
Notifications
You must be signed in to change notification settings - Fork 40k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve etcd-version-monitor metrics proxying, add etcd 3.1 gprc metr… #56871
Improve etcd-version-monitor metrics proxying, add etcd 3.1 gprc metr… #56871
Conversation
f9e73c0
to
3e3ff95
Compare
abe2934
to
09c80cc
Compare
Help: "Counter of received grpc requests, labeled by the grpc method and service names", | ||
|
||
gatherer = &monitorGatherer{ | ||
// Rewrite rules fo etcd metrics that are exported by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/fo/for/
for _, wl := range strings.Split(whitelisted, ",") { | ||
gatherer.exported[strings.TrimSpace(wl)] = &exportedMetric{} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to register all other metrics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might as well now thatwe have the proxying code needed to do that. Great idea actually. Will add.
09c80cc
to
65c8adb
Compare
65c8adb
to
a087462
Compare
@wojtek-t Thanks for the review. Feedback has been applied, PTAL. |
/assign @roberthbailey |
@mikedanese Do you have time to have a look at this? /assign @mikedanese |
/lgtm |
/approve no-issue |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpbetz, mikedanese, wojtek-t Associated issue: #56869 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/retest Review the full test history for this PR. |
Automatic merge from submit-queue (batch tested with PRs 56650, 55813, 56911, 56921, 56871). If you want to cherry-pick this change to another branch, please follow the instructions here. |
Partially addresses #56869:
etcd-version-monitor
to support etcd 3.1: Add support for the etcd 3.1+ go-grpc-prometheus metrics format, which backward incompatibly replaces the 3.0 legacy grpc metric format. Expose the go-grpc-prometheus metrics both in the 3.1 format and in the 3.0 format so systems consumingetcd-version-monitor
metrics have a clean, simple upgrade path.grpc_server_handling_seconds
from go-grpc-prometheus metrics format). Rewrite etcd 3.0 legacy metric for latency histograms to the etcd 3.1+go-grpc-prometheus
format so there is a single format exported for all etcd versions.etcd 3.0 to 3.1 upgrade path: Continue to use the
etcd_grpc_requests_total
. Once the upgrade is complete and all etcd nodes are running 3.1, migrate to thegrpc_server_handled_total
metric at your leisure.This PR reorganizes the code substantially. Previously, the code to proxy etcd metrics was hard coded and limited to a single counter metric. This has been entirely replaced with code that generically filters, rewrites proxied etcd metrics and then aggregates them with custom metrics such as the etcd version metric.
cc @wojtek-t @mml @shyamjvs @cheftako