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.

Sign up for GitHub

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

Jump to bottom

change kubelet probe metrics to counter #76074

Merged
merged 2 commits into from Apr 12, 2019
Merged

change kubelet probe metrics to counter #76074

merged 2 commits into from Apr 12, 2019

Conversation

danielqsj
Copy link
Contributor

@danielqsj danielqsj commented Apr 3, 2019 •
edited

What type of PR is this?

/kind feature
/sig instrumentation node

What this PR does / why we need it:

As discussion in #75839, we prefer to using counter type of metrics for kubelet probe rather than gauge type.

Which issue(s) this PR fixes:

Fixes #75839

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Change kubelet probe metrics to counter type.
The metrics `prober_probe_result` is replaced by `prober_probe_total`.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/node Categorizes an issue or PR as relevant to SIG Node. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 3, 2019
@danielqsj
Copy link
Contributor Author

/cc @brancz @logicalhan

@brancz
Copy link
Member

brancz commented Apr 3, 2019

Given that none of the popular alert/dashboard definitions use this metric, I'm inclined to just remove the old metric.

logicalhan
logicalhan reviewed Apr 3, 2019
View reviewed changes
Copy link
Member

@logicalhan logicalhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with just deleting the old metric as well. As mentioned in the issue, I would be surprised if anyone is actually using it for alerting since it is probably pretty noisy.

pkg/kubelet/prober/worker.go
@@ -98,16 +101,27 @@ func newWorker(
w.initialValue = results.Success
}

w.proberResultsMetricLabels = prometheus.Labels{
"probe_type": w.probeType.String(),
"container_name": w.container.Name,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woah, this is weird. I realize you didn't write this, but curious if anyone knows why we would have multiple labels on a metric which would always share the same label value?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you talking about container and contaiber_name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either. The previous implementation of this seems to store the same value for in both the container label and the container_name label. Same thing happens for pod and pod_name, which I find quite odd.

Copy link
Member

@logicalhan logicalhan Apr 3, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can verify it by outputting the actual metric output via curl.

prober_probe_result{container="etcd-container",container_name="etcd-container",namespace="kube-system",pod="etcd-server-events-kubernetes-master",pod_name="etcd-server-events-kubernetes-master",pod_uid="1234",probe_type="Liveness"} 0

Why do we have two labels which always have the same value in either of them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super weird.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was for migration purposes. For a long time these and cadvisor metrics used pod_name and container_name label keys, which violate the instrumentation guidelines. For migration purposes in 1.14 both are present.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that would explain it.

@danielqsj
Copy link
Contributor Author

@brancz @logicalhan removed old metrics prober_probe_result. PTAL

@brancz
Copy link
Member

brancz commented Apr 4, 2019

looks good from instrumentation side

/lgtm

still needs a kubelet approver though @derekwaynecarr @tallclair

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 4, 2019
logicalhan
logicalhan reviewed Apr 4, 2019
View reviewed changes
Copy link
Member

@logicalhan logicalhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@dashpole, thoughts?

@dashpole
Copy link
Contributor

dashpole commented Apr 5, 2019

/lgtm
This looks reasonable to me.

@danielqsj
Copy link
Contributor Author

/retest

@danielqsj
Copy link
Contributor Author

friendly ping @derekwaynecarr @tallclair , could you please help review it?

@tallclair
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Apr 11, 2019
@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.

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

@danielqsj
Copy link
Contributor Author

/retest

@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.

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

@brancz
Copy link
Member

brancz commented Apr 12, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit b7858e3 into kubernetes:master Apr 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@logicalhan logicalhan logicalhan left review comments

@dims dims Awaiting requested review from dims

@yujuhong yujuhong Awaiting requested review from yujuhong

@brancz brancz Awaiting requested review from brancz

Assignees

@logicalhan logicalhan

@dashpole dashpole

@brancz brancz

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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kubelet 'metrics/probes' endpoint uses a gauge instead of counters
7 participants
@danielqsj @brancz @dashpole @tallclair @k8s-ci-robot @fejta-bot @logicalhan