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

Is possible to align process.cpu.time,utilization metrics defintion to system.cpu.time,utilization #563

Closed
david-luna opened this issue Nov 21, 2023 · 9 comments · Fixed by #765

Comments

@david-luna
Copy link

david-luna commented Nov 21, 2023

What are you trying to achieve?

I've noticed these metrics are quite similar since they are measuring the same but process has a smaller scope. There is only a couple of differences.

  • Process metrics have no prefix in their attribute names
    System metrics are using system.cpu.state & system.cpu.logical_number and process is using state as attribute name
    System ref: system-metrics.md
    Process ref: process-metrics.md

  • Process metrics have a constraint about labels whereas system metrics doesn't have it

A process SHOULD be characterized either by data points with no state labels, or only data points with state labels.

What did you expect to see?

IMHO aligning the semantic conventions makes sense. I'd propose to make process metrics conventions closer to system metrics conventions by

  • adding prefix on the state attribute and becoming process.cpu.state
  • removing the constraint mentioned above

Additional context.

System metrics semconv had a recent change which added prefixes
#89

Maybe this something that is planned also fro process and hardware metrics? I could help on that

@david-luna
Copy link
Author

@open-telemetry/semconv-system-approvers could you check if this request makes sense?

@mx-psi
Copy link
Member

mx-psi commented Nov 21, 2023

Can we transfer this to semantic-conventions @arminru?

@david-luna I think the prefix part is being covered in /pull/330. I am not sure what the benefit of removing the constraint is, could you elaborate?

@david-luna
Copy link
Author

Hi @mx-psi

  • sorry for posting this issue to thew wrong repo :_)
  • about attribute names you're right. It is covered by the PR you shared 🎉

the goal is alignment so I do not have strong opinion on removing it. maybe adding the constraint to system metrics?

from a data consumer perspective I guess I'd expect to treat *.cpu.time and *.cpu.utilization in a similar way so one metric being reported with attributes and the other without could be misleading

@trask
Copy link
Member

trask commented Nov 27, 2023

Can we transfer this to semantic-conventions @arminru?

@open-telemetry/technical-committee can you transfer this issue? thx

@yurishkuro yurishkuro transferred this issue from open-telemetry/opentelemetry-specification Nov 27, 2023
@yurishkuro
Copy link
Member

transferred

@david-luna
Copy link
Author

While looking for references I came across this issue about cardinality open-telemetry/opentelemetry-js-contrib#1700

I guess this is why we want to have the option of not sending *.cpu.state labels.

@mx-psi
Copy link
Member

mx-psi commented Jan 18, 2024

Discussed on January 18th System Semantic Conventions WG meeting, adding this as a blocker since it would potentially change attribute names

@ChrsMark
Copy link
Member

Reading the constraint A process SHOULD be characterized either by data points with no state labels, or only data points with state labels I understand how it makes sense being specific for the process. However, I wonder if the same constraint could be applied to the system.cpu.state which is more generic for the whole system.

@open-telemetry/semconv-system-approvers what do you think?

@ChrsMark
Copy link
Member

Discussed on February 22th System Semantic Conventions WG meeting, and we agree that it makes sense to do the alignment and add the constraint for system.cpu.state as well. I will try to file a PR soon for this.

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