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

Consider merging *.cpu.state metric attributes #840

Closed
ChrsMark opened this issue Mar 27, 2024 · 7 comments · Fixed by #1026
Closed

Consider merging *.cpu.state metric attributes #840

ChrsMark opened this issue Mar 27, 2024 · 7 comments · Fixed by #1026
Assignees
Labels
area:container enhancement New feature or request

Comments

@ChrsMark
Copy link
Member

ChrsMark commented Mar 27, 2024

Area(s)

area:container
area:system
area:process

Is your change request related to a problem? Please describe.

At the moment we have 3 different metric attributes representing the cpu.state:

Based on the global flat registry idea it would be nice if we had one single definition for this attribute and then be able to re-use it in the various namespaces.

Describe the solution you'd like

As it is explained at #681 (comment) we could consider merging into one single one called cpu.state similar to the disk.io.direction and network.io.direction.

Describe alternatives you've considered

Maybe the embed concept would be sth to think of here: open-telemetry/build-tools#240

Additional context

No response

cc @open-telemetry/semconv-system-approvers @open-telemetry/semconv-container-approvers @open-telemetry/semconv-k8s-approvers

@ChrsMark
Copy link
Member Author

ChrsMark commented Apr 4, 2024

So to summarize the situation here, we have the following:

System Metrics

Values for system.cpu.state:

  • user
  • system
  • nice
  • idle
  • iowait
  • interrupt
  • steal

Process Metrics

Values for process.cpu.state:

  • user
  • system
  • wait

Container Metrics

Values for container.cpu.state:

  • user
  • system
  • kernel

Based on @braydonk's comment we can change the wait value of the process.cpu.state to idle and hence the process.cpu.state will be a subset of the system.cpu.state. Then we can use the superset for both.

Then the only difference with the container.cpu.state is the kernel value.
Checking the dockerstatsreceiver implementation, this value comes directly from the Docker API. For now I guess we should use it as is unless we find way to properly correlate it with rest of set's values. @jamesmoessis feel free to comment here if you have more information.

My first proposal would be to unify the attribute to cpu.state with possible values being the following [user, system, nice, idle, iowait, interrupt, steal, kernel].

What do you @open-telemetry/semconv-system-approvers think about this?

@AlexanderWert
Copy link
Member

AlexanderWert commented Apr 5, 2024

I like the proposal, it simplifies things.

@lmolkova Can we limit the valid values of an attribute when used in a concrete context to a subset of the overall allowed values through the tooling?

@ishleenk17
Copy link

ishleenk17 commented Apr 8, 2024

@ChrsMark : Primarily for system metrics, we consider the states mentioned here. IThere's an additional state softirq, which I didn't find in the code you referenced.
Just a thought, if we want to consider that as another member of the state as well.

@joaopgrassi
Copy link
Member

Removing the triage label, as this sounds like a good plan. Unifying them makes sense to me.

@ChrsMark
Copy link
Member Author

@lmolkova any thoughts on #840 (comment)?

@mx-psi
Copy link
Member

mx-psi commented Apr 26, 2024

I am in favor of this.

IThere's an additional state softirq, which I didn't find in the code you referenced.

@ishleenk17 I think this is Linux-specific, so it should go into its own separate metric like we did on #323

@trisch-me
Copy link
Contributor

@ChrsMark it's not possible yet but we are discussinga few weeks ago this option, one similar reference would be open-telemetry/build-tools#192 to exend enum from some base one, but option to use specific fields will be most probably also implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:container enhancement New feature or request
Projects
Development

Successfully merging a pull request may close this issue.

6 participants