Skip to content
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

Merged
merged 1 commit into from
Dec 16, 2017

Conversation

jpbetz
Copy link
Contributor

@jpbetz jpbetz commented Dec 5, 2017

Partially addresses #56869:

  • Fix 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 consuming etcd-version-monitor metrics have a clean, simple upgrade path.
  • Expose all etcd metrics by default, making this a one stop shop for all etcd metrics.
  • Expose grpc request latency histogram metrics (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 the grpc_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

Fix `etcd-version-monitor` to backward compatibly support etcd 3.1 [go-grpc-prometheus](https://github.com/grpc-ecosystem/go-grpc-prometheus) metrics format.

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 5, 2017
@jpbetz jpbetz self-assigned this Dec 5, 2017
@jpbetz jpbetz force-pushed the etcd-version-monitor-3.1 branch from f9e73c0 to 3e3ff95 Compare December 5, 2017 23:57
@jpbetz jpbetz added area/etcd sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. labels Dec 5, 2017
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 6, 2017
@jpbetz jpbetz force-pushed the etcd-version-monitor-3.1 branch 8 times, most recently from abe2934 to 09c80cc Compare December 6, 2017 06:17
@jpbetz jpbetz added this to the v1.10 milestone Dec 6, 2017
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.
Copy link
Member

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{}
}

Copy link
Member

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?

Copy link
Contributor Author

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.

@jpbetz jpbetz force-pushed the etcd-version-monitor-3.1 branch from 09c80cc to 65c8adb Compare December 6, 2017 17:33
@jpbetz jpbetz force-pushed the etcd-version-monitor-3.1 branch from 65c8adb to a087462 Compare December 6, 2017 17:34
@jpbetz
Copy link
Contributor Author

jpbetz commented Dec 6, 2017

@wojtek-t Thanks for the review. Feedback has been applied, PTAL.

@jpbetz
Copy link
Contributor Author

jpbetz commented Dec 6, 2017

/assign @roberthbailey

@jpbetz jpbetz removed their assignment Dec 6, 2017
@jpbetz
Copy link
Contributor Author

jpbetz commented Dec 7, 2017

@mikedanese Do you have time to have a look at this?

/assign @mikedanese

@wojtek-t wojtek-t self-assigned this Dec 7, 2017
@wojtek-t
Copy link
Member

wojtek-t commented Dec 7, 2017

/lgtm
/approve no-issue

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 7, 2017
@mikedanese
Copy link
Member

/approve no-issue

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@k8s-github-robot
Copy link

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.

@k8s-github-robot k8s-github-robot merged commit 8ae6202 into kubernetes:master Dec 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/etcd cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants