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

[receiver/kubeletstats] Add k8s.{container,pod}.memory.node.utilization metrics #33591

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ChrsMark
Copy link
Member

Description:

Similar to #32295 and #33390, this PR adds the k8s.{container,pod}.memory.node.utilization metrics.

Link to tracking Issue: #27885

Testing: Added unit test.

Documentation: Added

Manual testing

  1. Using the following target Pod:
apiVersion: v1
kind: Pod
metadata:
  name: memory-demo
spec:
  containers:
  - name: memory-demo-ctr
    image: polinux/stress
    resources:
      requests:
        memory: "8070591Ki"
      limits:
        memory: "9070591Ki"
    command: ["stress"]
    args: ["--vm", "1", "--vm-bytes", "800M", "--vm-hang", "4"]

memGood

On a node of 32,5G memory the 800Mb container/Pod consumes the 0.8/32.5=0.0246...=0.025.

@ChrsMark ChrsMark force-pushed the add_utilization_k8s_node_metrics branch 5 times, most recently from e2caba0 to df81840 Compare June 17, 2024 09:50
@ChrsMark ChrsMark marked this pull request as ready for review June 17, 2024 10:21
@ChrsMark ChrsMark requested a review from a team as a code owner June 17, 2024 10:21
@dmitryax
Copy link
Member

I missed why did we go with the dots instead of memory_node_utilization. All the parts in k8s.{container,pod}.memory are proper namespaces, but none of memory.node.utilization part sound like namespaces

@ChrsMark
Copy link
Member Author

I missed why did we go with the dots instead of memory_node_utilization. All the parts in k8s.{container,pod}.memory are proper namespaces, but none of memory.node.utilization part sound like namespaces

A valid alternative would be k8s.{container,pod}.memory.node_utilization since k8s.{container,pod}.memory must be a namespace to accommodate for other memory metrics as well, right?

Based on this, instead of k8s.{container,pod}.memory.node_utilization choosing k8s.{container,pod}.memory.node.utilization preserves us the space for the (unlikely but existing) scenario where we would need to add extra metrics like the node.allocatable_utilization or whatever (based on Node's limits/characteristics/status), either emitted by the Collector or calculated and stored by a back-end.
Also based on the issues that affect SemConvs that have underscores (open-telemetry/semantic-conventions#1118) I think going with dots is a safer option here since we don't violate any other rule (ie a metric cannot be a namespace at the same time)

@ChrsMark ChrsMark force-pushed the add_utilization_k8s_node_metrics branch from b5f9c5b to 5ae764d Compare June 25, 2024 07:31
…on metrics

Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark force-pushed the add_utilization_k8s_node_metrics branch from 5ae764d to 2b853bd Compare June 25, 2024 07:32
@ChrsMark
Copy link
Member Author

ChrsMark commented Jul 3, 2024

@TylerHelmuth @dmitryax Anything else missing here?

Copy link
Contributor

@rogercoll rogercoll left a comment

Choose a reason for hiding this comment

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

LGTM

@andrzej-stencel
Copy link
Member

I missed why did we go with the dots instead of memory_node_utilization. All the parts in k8s.{container,pod}.memory are proper namespaces, but none of memory.node.utilization part sound like namespaces

A valid alternative would be k8s.{container,pod}.memory.node_utilization since k8s.{container,pod}.memory must be a namespace to accommodate for other memory metrics as well, right?

Based on this, instead of k8s.{container,pod}.memory.node_utilization choosing k8s.{container,pod}.memory.node.utilization preserves us the space for the (unlikely but existing) scenario where we would need to add extra metrics like the node.allocatable_utilization or whatever (based on Node's limits/characteristics/status), either emitted by the Collector or calculated and stored by a back-end. Also based on the issues that affect SemConvs that have underscores (open-telemetry/semantic-conventions#1118) I think going with dots is a safer option here since we don't violate any other rule (ie a metric cannot be a namespace at the same time)

Also see this discussion in similar PR:

There, the metric name evolved from k8s.container.cpu_limit_utilization to k8s.container.cpu.node_utilization to k8s.container.cpu.node.utilization. The last change was motivated by the fact that you could have different kinds of "utilization", based either on the node's full CPU capacity or only the node's allocatable CPU resources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants