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

Change docker metrics to conform metrics guidelines #72323

Merged
merged 3 commits into from
Feb 6, 2019

Conversation

danielqsj
Copy link
Contributor

@danielqsj danielqsj commented Dec 25, 2018

What type of PR is this?

/sig instrumentation

What this PR does / why we need it:

  1. As part of kubernetes metrics overhaul, change docker metrics to conform Kubernetes metrics instrumentation guidelines.

  2. Change docker metrics to histogram for better aggregation.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

This patch does not remove the existing metrics but mark them as deprecated.
We need 2 releases for users to convert monitoring configuration.

Does this PR introduce a user-facing change?:

Change docker metrics to conform metrics guidelines and using histogram for better aggregation.
The following metrics are deprecated, and will be removed in a future release:
* `docker_operations`
* `docker_operations_latency_microseconds`
* `docker_operations_errors`
* `docker_operations_timeout`
* `network_plugin_operations_latency_microseconds`
Please convert to the following metrics:
* `docker_operations_total`
* `docker_operations_latency_seconds`
* `docker_operations_errors_total`
* `docker_operations_timeout_total`
* `network_plugin_operations_latency_seconds`

@k8s-ci-robot k8s-ci-robot added 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/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 25, 2018
@danielqsj danielqsj changed the title Dockershim Change docker metrics to conform metrics guidelines Dec 25, 2018
@k8s-ci-robot k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Dec 25, 2018
@danielqsj
Copy link
Contributor Author

/kind feature
/assign @brancz

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Dec 25, 2018
@liggitt
Copy link
Member

liggitt commented Dec 26, 2018

What is the compatibility guarantee on metric names/values? Changing metrics this way with no advance notice is likely to cause problems with monitoring over upgrade boundaries. I'm not sure if this falls under an aspect of our deprecation policy that would require a longer pre-announce period, but if not, I'd advocate at least one release overlap where both old and new metrics are available.

@liggitt
Copy link
Member

liggitt commented Dec 26, 2018

These questions would be good to answer broadly and include in
https://github.com/kubernetes/enhancements/blob/master/keps/sig-instrumentation/0031-kubernetes-metrics-overhaul.md to inform the other metrics updating efforts

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 26, 2018
@danielqsj
Copy link
Contributor Author

@liggitt
Thanks, totally agree.
I already added the previous metrics back and marked them as deprecated.

@danielqsj
Copy link
Contributor Author

Added Deprecated in metrics description. PTAL

@danielqsj
Copy link
Contributor Author

/retest

@brancz
Copy link
Member

brancz commented Jan 8, 2019

@danielqsj could you open a PR against the metrics overhaul that says that the 1.14 release will have backward compatibility but 1.15 will drop the deprecated metrics and label? Thanks!

@brancz
Copy link
Member

brancz commented Jan 8, 2019

/retest

@brancz
Copy link
Member

brancz commented Jan 8, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 8, 2019
@brancz
Copy link
Member

brancz commented Jan 8, 2019

/retest

2 similar comments
@brancz
Copy link
Member

brancz commented Jan 8, 2019

/retest

@danielqsj
Copy link
Contributor Author

/retest

@brancz
Copy link
Member

brancz commented Jan 10, 2019

/assign @derekwaynecarr

@danielqsj
Copy link
Contributor Author

@derekwaynecarr can you help review this? Thanks

@danielqsj
Copy link
Contributor Author

@Random-Liu @sttts @deads2k if you have time, can you help review this? Thanks

@danielqsj
Copy link
Contributor Author

@danielqsj
Copy link
Contributor Author

@thockin @derekwaynecarr @Random-Liu @sttts @deads2k if you have time, can you help review this? Thanks

@sttts
Copy link
Contributor

sttts commented Feb 4, 2019

/assign @derekwaynecarr

for approval.

@derekwaynecarr
Copy link
Member

/approve

@brancz
Copy link
Member

brancz commented Feb 5, 2019

cc @deads2k @smarterclayton for test/OWNERS approval

@smarterclayton
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielqsj, derekwaynecarr, smarterclayton

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 6, 2019
@k8s-ci-robot k8s-ci-robot merged commit ae45068 into kubernetes:master Feb 6, 2019
This pull request was closed.
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/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. 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. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. 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