[k8s] Promote k8s.pod.cpu.time, k8s.node.cpu.time and container.cpu.time metrics to release_candidate#3693
Conversation
…trics to release_candidate Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
|
@open-telemetry/specs-semconv-approvers it's still unclear if for metrics to be promoted through stability levels their respective Entitites should be first promoted. Discussing this in the System SemConv SIG last week, @dmitryax mentioned that Entities should not block metrics' graduation while they are still being worked. But we need to verify this. Other than this, we still need to stabilise cpu.mode which is conditionally required for |
dashpole
left a comment
There was a problem hiding this comment.
lgtm, assuming other blockers are resolved
|
@dashpole I spent some time verifying the Issue
(note that This means that Kuberentes/kubeletstats pathOpencontainers cgroup library calcalates the CPU stats at (for cgroupd v1) https://github.com/opencontainers/cgroups/blob/v0.0.6/fs/cpuacct.go#L37: totalUsage, err := fscommon.GetCgroupParamUint(path, "cpuacct.usage")
// → stats.CpuStats.CpuUsage.TotalUsage
userModeUsage, kernelModeUsage, err := getCpuUsageBreakdown(path)
// reads "cpuacct.stat" (user / system lines)
// → stats.CpuStats.CpuUsage.UsageInUsermode
// → stats.CpuStats.CpuUsage.UsageInKernelmodeFrom https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v1/cpuacct.html:
In cgroup V2 the calculation happens in https://github.com/opencontainers/cgroups/blob/v0.0.6/fs2/cpu.go#L79-L101 CPU Stats struct: https://github.com/opencontainers/cgroups/blob/v0.0.6/stats.go#L22-L41 cAdvisorcAdvisor derives the above metrics from the library and sets them at https://github.com/google/cadvisor/blob/v0.56.2/container/libcontainer/handler.go#L769-L772. cAdvisor's internal structs: https://github.com/google/cadvisor/blob/v0.56.2/info/v1/container.go#L303-L319 cAdvisor to PrometheusThose stats are exposed eventually as Prometheus metrics at https://github.com/google/cadvisor/blob/v0.56.2/metrics/prometheus.go#L151-L188:
Notes
Why user + system ≠ total? On cgroup v1 they come from two different kernel accounting mechanisms — cpuacct.usage (nanosecond-resolution, high-fidelity) vs cpuacct.stat (jiffie-based, 10ms resolution). This discrepancy persists on cgroup v2 but is smaller (microsecond vs microsecond, but usage_usec includes more accounting than just user+system). cAdvisor to Kubelet' /stats/summary APIAt cadvisorInfoToCPUandMemoryStats() Kubelet retrieves the Hence the sequence for the K8s path is the following: Docker stats receiver pathThe component uses the Moby library and retrieves the data from the CPUUsage struct. In cgroup v2 the library uses the In cgroup v1 the library uses the cpuacct.usage and cpuacct.stat. Summary
However the So all the underneath libraries and implementations align and we just need to ensure that our model in Otel SemConv is correct:
An example is that on the same system we can emit the In that case, is the following valid Prometheus metrics? Note that If I'm not mistaken @dashpole @jsuereth @open-telemetry/semconv-k8s-approvers thoughts? |
Do we think this is a big enough difference to warrant a new metric? Is it possible to have the docker version populate the attribute, but kubelet doesn't populate it? |
That was the original idea but I think we need 3 different metrics/time-series here anyway. Dockerstats receiver can populate all 3 while kubeletstats only the total. One option here is to keep Then to cover the 2 granular metrics/modes we can introduce a new different metric i.e. This is also close to the notion of croup v1:
While cpuacct.usage -> total. Open to other names/options here too but overall I think we can't avoid the split. |
Sorry, i'm not following. If you only have the total, you just expose the metric without the attribute that splits into user/system. If user + system < total, then we can add an |
It's not clear to me what is the complete suggestion here. Could you share how you think about it both in the Note that today the From my perspective it makes sense to be able to provide all these 3 if they are available. So how are we going to cover this given that (verifying this locally (for cgroup v2) with docker stats API, I found that
Would |
|
Expose one metric: It has an attribute: The kubeletstats receiver omits the |
I guess you mean
Ok, so in that case the where the While the I think that would be fine, if we accept |
|
BTW @ChrsMark my original question from #3693 (comment), is: Is the |
I tried on my end the following (cgroup v2): for i in $(seq 1 100); do
curl -s --unix-socket /var/run/docker.sock \
"http://localhost/containers/kind-control-plane/stats?stream=false" \
| jq '.cpu_stats.cpu_usage'
done | jq -s '.'and fed the results to an llm to analyse. The delta is always exactly 0 or +1000 ns. In approximately 45% of samples, the discrepancy always being exactly 1 µs. Might be a rounding "noise" in the cgroup's v2 accounting or sth. Given that |
|
Yep! Thats what I was hoping for. We won't need the other case after all then |
76a997a
Related to #3001
Changes
This PR aims to advertise the intention for stabilising the K8s/container metrics:
k8s.pod.cpu.timek8s.node.cpu.timecontainer.cpu.timeThese metrics have been in use for long time now in the OpenTelemetry Collector and specifically the kubeletstatsreceiver:
Requirement levels have been discussed at #3001 (comment)
TODOS:
container.cpu.time.Merge requirement checklist
[chore]