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/kubeletstatsreceiver] prepend container. metrics with k8s. #24238

Closed
mackjmr opened this issue Jul 12, 2023 · 6 comments
Closed

[receiver/kubeletstatsreceiver] prepend container. metrics with k8s. #24238

mackjmr opened this issue Jul 12, 2023 · 6 comments

Comments

@mackjmr
Copy link
Member

mackjmr commented Jul 12, 2023

Component(s)

receiver/kubeletstats

Describe the issue you're reporting

There are currently metric collisions between the dockerstatsreceiver and kubeletstatsreceiver:

And there may arise additional collisions in the future if the metric naming does not differentiate between the two.

This is a proposal to prepend all the container.* metrics in the kubeletstatsreceiver by k8s.. For example, container.cpu.utilization will become k8s.container.cpu.utilization.

All other metrics in the kubeletstatsreceiver are already prepended by k8s..

This could be done in multiple steps (similarly to what was done in #22823). If we take metric container.cpu.utilization as an example:

  • Release N: Add metric k8s.container.cpu.utilization disabled by default and deprecate container.cpu.utilization
  • Release N+2: Make k8s.container.cpu.utilization enabled by default and container.cpu.utilization disabled by default
  • Release N+4: Remove container.cpu.utilization
@mackjmr mackjmr added the needs triage New item requiring triage label Jul 12, 2023
@github-actions
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@mackjmr mackjmr changed the title [receiver/kubeletstatsreceiver] prepend container metrics with k8s. [receiver/kubeletstatsreceiver] prepend container. metrics with k8s. Jul 12, 2023
mackjmr added a commit to mackjmr/opentelemetry-collector-contrib that referenced this issue Jul 19, 2023
This PR introduces `k8s.container*` metrics, which will replace their `container.*` metrics counterparts. For now, the `k8s.container*` metrics are disabled by default.

The following process will be followed to phase out the old metrics:
- In `v0.82.0`, the new metric is introduced and the old metric is marked as deprecated.
  Only the old metric are emitted by default.
- In `v0.84.0`, the old metric is disabled and the new one enabled by default.
- In `v0.86.0` and up, the old metric is removed.

Please refer to the following issue: open-telemetry#24238 for the reason for this change.
@dmitryax
Copy link
Member

dmitryax commented Jul 20, 2023

This wasn't prepended intentionally because it's essentially the same metric about a container (not an orchestration engine). If you have a container in kubernetes, you extract it with kubelet receiver where there is no docker engine anymore. If you run a container with docker (on ECS or somewhere else), you use docker receiver. Resource attributes for the containers will always be different. Not sure what collision users can encounter. Please elaborate.

@mackjmr
Copy link
Member Author

mackjmr commented Jul 24, 2023

Looking at how container.cpu.utilization is calculated in dockerstatsreceiver (here) vs kubeletstatsreceiver (here), it seems like they don't represent the same thing (please correct me if I am wrong). For the dockerstatsreceiver, it looks like the CPU utilization represents the percentage CPU utilization over a timeframe, whereas for the kubeletstatsreceiver it looks like the CPU utilization represents the CPU usage in Cores at a given time (avg of a sample window).

There are some differences as to how they are computed, as they are not pulled from the same place (container runtime vs kubelet stats) and don't represent the same thing. This doesn't pose an issue if the metric is queried with specific attributes, so that when the metric is being vizualized it only shows values that are pulled from one or the other receiver. That said, I believe having a single metric being pulled from different place can lead to confusion, and having a disctinction between the two allows it to be clearer to the end user as to what the metric they are looking at represents.

@mackjmr
Copy link
Member Author

mackjmr commented Sep 7, 2023

For this point:

If you have a container in kubernetes, you extract it with kubelet receiver where there is no docker engine anymore.

You are still able to use the dockerstatsreceiver for containers running in kubernetes if k8s is using docker as a runtime (although this has been mostly deprecated and removed in k8s). Users can use the dockerstatsreceiver and kubeletstatsreceiver in conjunction.

@dmitryax adding on to this, I agree that there are some cases in which it could make sense to have the same metric name across multiple receivers.

For example, in this case if there were containerdstats / cri-ostats receivers that collect metrics from the container runtime like the dockerstats receiver does, then I believe it could make sense for them to all have the same naming, as they are essentially the same metrics pulled from the container runtime. I think we would still need to make sure that there is consistency around the metrics units.

That being said, in this case, seeing that the kubeletstats receiver pulls metrics from the kubelet, I don't think they should have the same naming as the container runtime metrics, and I think it would be very hard to try making both represent the same values.

I will also open a separate issue for metrics collisions in general. Opened issue: #26499

Copy link
Contributor

github-actions bot commented Nov 8, 2023

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Nov 8, 2023
Copy link
Contributor

github-actions bot commented Jan 7, 2024

This issue has been closed as inactive because it has been stale for 120 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants