Skip to content

add metric for per-cpu, per-stage timing #516

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
Merged
merged 1 commit into from
Feb 2, 2021
Merged

add metric for per-cpu, per-stage timing #516

merged 1 commit into from
Feb 2, 2021

Conversation

@karan
Copy link
Contributor

@karan karan commented Jan 22, 2021 •
edited
Loading

Tested on a COS VM:

$ curl -s localhost:20257/metrics  | grep -i ^system_cpu_stat
system_cpu_stat{cpu="cpu0",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="guest"} 0
system_cpu_stat{cpu="cpu0",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="guestNice"} 0
system_cpu_stat{cpu="cpu0",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="iRQ"} 0
system_cpu_stat{cpu="cpu0",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="idle"} 226616.13
system_cpu_stat{cpu="cpu0",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="iowait"} 23.92
system_cpu_stat{cpu="cpu0",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="nice"} 63.08
system_cpu_stat{cpu="cpu0",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="softIRQ"} 100.91
system_cpu_stat{cpu="cpu0",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="steal"} 67.03999999999999
system_cpu_stat{cpu="cpu0",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="system"} 2134.91
system_cpu_stat{cpu="cpu0",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="user"} 5909.54
system_cpu_stat{cpu="cpu1",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="guest"} 0
system_cpu_stat{cpu="cpu1",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="guestNice"} 0
system_cpu_stat{cpu="cpu1",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="iRQ"} 0
system_cpu_stat{cpu="cpu1",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="idle"} 226315.40000000002
system_cpu_stat{cpu="cpu1",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="iowait"} 339.48
system_cpu_stat{cpu="cpu1",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="nice"} 63.510000000000005
system_cpu_stat{cpu="cpu1",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="softIRQ"} 102.99
system_cpu_stat{cpu="cpu1",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="steal"} 67.47
system_cpu_stat{cpu="cpu1",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="system"} 2167.6800000000003
system_cpu_stat{cpu="cpu1",kernel_version="5.4.49+",os_version="cos 85-13310.1041.24",stage="user"} 5827.360000000001

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 22, 2021
@karan
Copy link
Contributor Author

karan commented Jan 22, 2021

/assign @vteratipally
/assign @Random-Liu

@vteratipally
Copy link
Contributor

/lgtm

@vteratipally
Copy link
Contributor

/assign @Random-Liu

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 26, 2021
vteratipally
vteratipally reviewed Jan 26, 2021
View reviewed changes
pkg/systemstatsmonitor/cpu_collector.go
if cc.mSystemProcsBlocked == nil {
return
}
if cc.mSystemInterruptsTotal == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

will the cpu_collector break if the config is not applied?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No - this is a nil check for the metric (as opposed to the config). The metric will only be initialized if the config is provided:

node-problem-detector/pkg/systemstatsmonitor/system_stats_monitor.go

Lines 79 to 81 in 1a7aa65

if len(ssm.config.CPUConfig.MetricsConfigs) > 0 {
ssm.cpuCollector = NewCPUCollectorOrDie(&ssm.config.CPUConfig)
}

So this will always evaluate to false.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 28, 2021
Random-Liu
Random-Liu reviewed Jan 29, 2021
View reviewed changes
pkg/systemstatsmonitor/cpu_collector.go
}

func NewCPUCollectorOrDie(cpuConfig *ssmtypes.CPUStatsConfig) *cpuCollector {
cc := cpuCollector{tags: map[string]string{}, config: cpuConfig}
Copy link
Member

Choose a reason for hiding this comment

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

Why removing the kernel and os labels? Can you explain it in the PR description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I meant to but forgot amongst oncall chaos. :) Will add now

@Random-Liu
Copy link
Member

In theory removing labels may cause potential backward compatibility issues?

@karan
Copy link
Contributor Author

karan commented Feb 1, 2021

In theory yes, however these schemas were not used.

@Random-Liu
Copy link
Member

In theory yes, however these schemas were not used.

However, we don't know whether they are used by some OSS users. In theory, it is possible that someone is using it.

We don't have deprecation policy defined for the metrics today.

Let's at least leave it one release. We can announce deprecation for those labels first in the next release, and remove them in the future.

We should file an issue to track that, so that we don't forget about it when we cut the next release.

@karan
Copy link
Contributor Author

karan commented Feb 1, 2021

I suggest the following:

  1. Send a notification to sig-node (and mention this in the sig-node meeting tomorrow)
  2. If someone is using NPD, we can follow your plan. Otherwise, I'd like to not block this PR any longer for theoretical reasons.

How does that sound?

@Random-Liu
Copy link
Member

Random-Liu commented Feb 1, 2021 •
edited
Loading

Otherwise, I'd like to not block this PR any longer for theoretical reasons.

I think you can at least separate the label removal from the new metrics change. We don't need to block the new metrics because of the label removal.

@karan
Copy link
Contributor Author

karan commented Feb 1, 2021

Done

@Random-Liu
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: karan, Random-Liu, vteratipally

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 2, 2021
@k8s-ci-robot k8s-ci-robot merged commit 422c088 into kubernetes:master Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Random-Liu Random-Liu Random-Liu left review comments

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@wangzhen127 wangzhen127 Awaiting requested review from wangzhen127

+1 more reviewer

@vteratipally vteratipally vteratipally approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Random-Liu Random-Liu

@vteratipally vteratipally

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@karan @vteratipally @Random-Liu @k8s-ci-robot