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

remove the deprecated admission metrics #75279

Merged
merged 1 commit into from Mar 20, 2019

Conversation

@danielqsj
Copy link
Member

commented Mar 12, 2019

What type of PR is this?

/kind cleanup
/sig instrumentation

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

As discussed in #74418, we prefer to directly remove the deprecated admission metrics which doesn't seem to help much.

We can convert to use the new and correct metrics instead of them.

/assign @liggitt @brancz

Does this PR introduce a user-facing change?:

Replace *_admission_latencies_milliseconds_summary and *_admission_latencies_milliseconds metrics due to reporting wrong unit (was labelled milliseconds, but reported seconds), and multiple naming guideline violations (units should be in base units and "duration" is the best practice labelling to measure the time a request takes). Please convert to use *_admission_duration_seconds and *_admission_duration_seconds_summary, these now report the unit as described, and follow the instrumentation best practices.
@brancz

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Code wise looks good, should we explain the decision in release notes or should people refer to this PR and the linked discussions?

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 12, 2019

@danielqsj

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

Thanks @brancz . Corrected the release notes.

@smarterclayton

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

For the release note describe a bit more detail about the metric being removed and why (actually name the old metrics that are removed). The best way to get people comfortable with this change is to have an excellent release note - clearly communicating mitigates the impact and helps admins get the right metrics.

@brancz

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Agreed. Maybe we should describe the whole situation:

Replace *_admission_latencies_milliseconds_summary and *_admission_latencies_milliseconds metrics due to reporting wrong unit (was labelled milliseconds, but reported seconds), and multiple naming guideline violations (units should be in base units and "duration" is the best practice labelling to measure the time a request takes). Please convert to use *_admission_duration_seconds and *_admission_duration_seconds_summary, these now report the unit as described, and follow the instrumentation best practices.

@caesarxuchao

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

/assign @logicalhan

@danielqsj

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

Thanks @smarterclayton and @brancz . The suggestion from @brancz is pretty excellent. I have changed the release note as it. PTAL

@brancz

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

/lgtm

@logicalhan
Copy link
Contributor

left a comment

/lgtm

@danielqsj

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

@smarterclayton friendly ping :)

@smarterclayton

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Approve for 1.15 once we reopen

@smarterclayton

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielqsj, 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 merged commit 5229bce into kubernetes:master Mar 20, 2019

17 checks passed

cla/linuxfoundation danielqsj authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.