Fix admission metrics in true units #72343

danielqsj · 2018-12-26T15:14:30Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Admission metrics name is *_admission_latencies_seconds and *_admission_latencies_seconds_summary, the units from metrics name are seconds, but actually the return metrics are in microseconds, this PR aims to fix these metrics in seconds.

Which issue(s) this PR fixes:

Fixes #72342

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix admission metrics in seconds.
Add metrics `*_admission_latencies_milliseconds` and `*_admission_latencies_milliseconds_summary` for backward compatible, but will be removed in a future release.

logicalhan

/lgtm

liggitt · 2018-12-27T02:35:40Z

/unassign
/assign @jpbetz @sttts

liggitt · 2018-12-29T23:23:39Z

staging/src/k8s.io/apiserver/pkg/admission/metrics/metrics.go

@@ -206,9 +206,9 @@ func (m *metricSet) reset() {

 // Observe records an observed admission event to all metrics in the metricSet.
 func (m *metricSet) observe(elapsed time.Duration, labels ...string) {
-	elapsedMicroseconds := float64(elapsed / time.Microsecond)
-	m.latencies.WithLabelValues(labels...).Observe(elapsedMicroseconds)


will changing the values here cause problems for monitoring already set up to track/alarm on these?

Mainly, I can think of two ways this metric is (and can be) broken for people who have monitoring set up against these.

The first group would be people who are using this metric and are aware that the metric is emitting in microseconds even though the label is in seconds. If they have set their alerts accordingly (this would be weird but not impossible), then we would break their monitoring with this fix.

The second group are people who are using this metric as if this metric was working correctly, i.e. emitting latency in seconds. In that case, thresholds which are currently set for alerting would be off by orders of magnitude and this fix would actually make those alerts start working as intended.

Personally, I think we should just fix it.

I'm also for changing as this is an actual bug, but can you add an item to the changelog that this is a change?

This is part of the metrics overhaul planned for 1.14 where a number of metrics are changing and we're documenting every single case including what to change. As a middle ground, let's add an already deprecated metric that is called admission_latencies_milliseconds_summary so people who are affected by the break would only have to change the metric name and not do a unit conversion. I think this would work well, as 1.14 is the "metric migration" release, where we have deprecated metrics as well as the (new) best practice following metrics and the deprecated ones will be removed in 1.15.

This one is an interesting case as it's not just not following the best practice, but also incorrectly labels its unit. It will either way need a separate, additional changelog notice.

agree with @brancz proposal.
Added admission_latencies_milliseconds and admission_latencies_milliseconds_summary for Backward compatible. PTAL

danielqsj · 2019-01-08T05:21:00Z

/cc @brancz

danielqsj · 2019-01-18T05:52:05Z

/retest

brancz

Just a small thing on consistency. Otherwise looks good.

brancz · 2019-01-18T08:01:21Z

staging/src/k8s.io/apiserver/pkg/admission/metrics/metrics.go

+				Namespace: namespace,
+				Subsystem: subsystem,
+				Name:      fmt.Sprintf("%s_admission_latencies_milliseconds", name),
+				Help:      fmt.Sprintf(helpTemplate, "latency histogram in milliseconds (deprecated)"),


brancz · 2019-01-18T08:02:20Z

staging/src/k8s.io/apiserver/pkg/admission/metrics/metrics.go

+				Namespace: namespace,
+				Subsystem: subsystem,
+				Name:      fmt.Sprintf("%s_admission_latencies_milliseconds_summary", name),
+				Help:      fmt.Sprintf(helpTemplate, "latency summary in milliseconds (deprecated)"),


We should be consistent with the deprecation warning. Let’s make sure the help text is preceded with (Deprecated) like the other metrics we have already deprecated.

@brancz fixed.

brancz · 2019-01-18T10:03:37Z

/lgtm

danielqsj · 2019-01-21T09:19:06Z

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

jpbetz · 2019-01-22T21:17:10Z

Apologies for mis-labeling this metric. That's clearly my fault.

Note that we'll be doubling the memory utilization of the metrics for admission which might matter for those that use a lot of admission controllers. But given that we reduced the carnality of these metrics in the last release, I don't think that will be a show stopper. Let's just document the deprecation plan so we know when we can remove the old metrics for good.

jpbetz · 2019-01-22T21:20:49Z

staging/src/k8s.io/apiserver/pkg/admission/metrics/metrics.go

+				Namespace: namespace,
+				Subsystem: subsystem,
+				Name:      fmt.Sprintf("%s_admission_latencies_milliseconds_summary", name),
+				Help:      fmt.Sprintf("(Deprecated) "+helpTemplate, "latency summary in milliseconds"),


Add which k8s version these will be removed in here in the help string?

This is consistent with the deprecation warning in other metrics we have deprecated.
But surely, we will announce the metrics migration/deprecation plan in release notes or in other ways.
cc @brancz

1.14 the "old" metrics are deprecated and 1.15 is targeted for removal. Let's explicitly document this in the KEP.

brancz · 2019-01-23T08:55:58Z

@danielqsj let's make sure the deprecation plan is more thoroughly documented in the KEP. Do you want to take care of that?

danielqsj · 2019-01-23T09:01:19Z

@brancz sure. I will update KEP about the deprecation plan and the latest PRs which not covered.

danielqsj · 2019-01-28T06:36:30Z

@sttts @deads2k @smarterclayton if you have time, can you help review this? Thanks

sttts · 2019-01-28T08:10:07Z

/approve

k8s-ci-robot · 2019-01-28T08:10:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielqsj, sttts

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:

~~staging/src/k8s.io/apiserver/OWNERS~~ [sttts]

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

brancz · 2019-01-28T09:50:05Z

/retest

fejta-bot · 2019-01-28T13:44:38Z

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2019-01-28T17:35:41Z

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2019-01-28T21:05:44Z

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2019-01-29T02:20:44Z

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot · 2019-01-29T04:13:35Z

@danielqsj: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`d9c57e7`	link	`/test pull-kubernetes-e2e-kops-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

Return admission metrics with true units

c183646

k8s-ci-robot requested review from CaoShuFeng and liggitt December 26, 2018 15:15

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 26, 2018

k8s-ci-robot assigned logicalhan Dec 26, 2018

logicalhan approved these changes Dec 26, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 26, 2018

k8s-ci-robot assigned jpbetz and sttts Dec 27, 2018

liggitt reviewed Dec 29, 2018

View reviewed changes

k8s-ci-robot requested a review from brancz January 8, 2019 05:21

danielqsj mentioned this pull request Jan 8, 2019

Change existing metrics to conform metrics guidelines #72333

Closed

9 tasks

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 18, 2019

brancz reviewed Jan 18, 2019

View reviewed changes

Add admission_latencies_milliseconds metrics for backward compatible

d9c57e7

danielqsj force-pushed the adm branch from 7050745 to d9c57e7 Compare January 18, 2019 09:56

k8s-ci-robot assigned brancz Jan 18, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 18, 2019

jpbetz reviewed Jan 22, 2019

View reviewed changes

danielqsj mentioned this pull request Jan 28, 2019

Add deprecation plan and new PRs for KEP0031 kubernetes/enhancements#734

Merged

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 28, 2019

k8s-ci-robot merged commit 035332f into kubernetes:master Jan 29, 2019

danielqsj mentioned this pull request Mar 6, 2019

convert latency/latencies in metrics name to duration #74418

Merged

danielqsj mentioned this pull request Apr 8, 2019

Admission metrics value not match their units #72342

Closed

jpbetz mentioned this pull request May 31, 2019

Fix admission metrics bucket sizes #78608

Merged

brancz mentioned this pull request Aug 7, 2019

Kubernetes Metrics Overhaul kubernetes/enhancements#1206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix admission metrics in true units #72343

Fix admission metrics in true units #72343

danielqsj commented Dec 26, 2018 •

edited

Loading

logicalhan left a comment

liggitt commented Dec 27, 2018

liggitt Dec 29, 2018

logicalhan Jan 3, 2019

brancz Jan 8, 2019

brancz Jan 17, 2019

danielqsj Jan 18, 2019

danielqsj commented Jan 8, 2019

danielqsj commented Jan 18, 2019

brancz left a comment

brancz Jan 18, 2019

brancz Jan 18, 2019

danielqsj Jan 18, 2019

brancz commented Jan 18, 2019

danielqsj commented Jan 21, 2019

jpbetz commented Jan 22, 2019

jpbetz Jan 22, 2019

danielqsj Jan 23, 2019

brancz Jan 23, 2019 •

edited

Loading

brancz commented Jan 23, 2019

danielqsj commented Jan 23, 2019

danielqsj commented Jan 28, 2019

sttts commented Jan 28, 2019

k8s-ci-robot commented Jan 28, 2019

brancz commented Jan 28, 2019

fejta-bot commented Jan 28, 2019

fejta-bot commented Jan 28, 2019

fejta-bot commented Jan 28, 2019

fejta-bot commented Jan 29, 2019

k8s-ci-robot commented Jan 29, 2019

Fix admission metrics in true units #72343

Fix admission metrics in true units #72343

Conversation

danielqsj commented Dec 26, 2018 • edited Loading

logicalhan left a comment

Choose a reason for hiding this comment

liggitt commented Dec 27, 2018

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

danielqsj commented Jan 8, 2019

danielqsj commented Jan 18, 2019

brancz left a 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

brancz commented Jan 18, 2019

danielqsj commented Jan 21, 2019

jpbetz commented Jan 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brancz Jan 23, 2019 • edited Loading

Choose a reason for hiding this comment

brancz commented Jan 23, 2019

danielqsj commented Jan 23, 2019

danielqsj commented Jan 28, 2019

sttts commented Jan 28, 2019

k8s-ci-robot commented Jan 28, 2019

brancz commented Jan 28, 2019

fejta-bot commented Jan 28, 2019

fejta-bot commented Jan 28, 2019

fejta-bot commented Jan 28, 2019

fejta-bot commented Jan 29, 2019

k8s-ci-robot commented Jan 29, 2019

danielqsj commented Dec 26, 2018 •

edited

Loading

brancz Jan 23, 2019 •

edited

Loading