Merge *.cpu.state attributes to a common cpu.state attribute #1026
Conversation
ChrsMark
force-pushed
the
merge_cpu_states
branch
2 times, most recently
from
de27ecd
to
dbd49a4
Compare
| members: | ||
| - id: user | ||
| value: 'user' | ||
| brief: "When tasks of the cgroup are in user mode (Linux). When all container processes are in user mode (Windows)." |
There was a problem hiding this comment.
btw we are missing these briefs now in actual attributes. You can add metric specific brief additionally to the ref
There was a problem hiding this comment.
Well, these are the briefs of the specific members not the attributes.
If we want to provide specific brief per metric family we would need to redefine the whole attribute's members I guess?
That sounds weird to me, that's why I propose to completely merge them and make the descriptions generic.
@open-telemetry/semconv-system-approvers what do you think about this?
There was a problem hiding this comment.
I understand, you are correct we can't currently redefine specific members of enums
There was a problem hiding this comment.
Keeping this open as I would love to get feedback from @open-telemetry/semconv-system-approvers here.
If we really want to provide specific descriptions for the enum's members per namespace then maybe this merging we are attempting here does not really make sense 🤔 ?
There was a problem hiding this comment.
To me the deciding factor here is whether the different descriptions are clarifications or if they are truly different things. At a first glance, it sounds to me like they are clarifications
There was a problem hiding this comment.
@braydonk @jamesmoessis any thoughts here?
There was a problem hiding this comment.
We discussed about it during the last System Metrics WG (May 30th).
What we conclude on is that we would like to have the option to override the briefs and only use a subset of the defined members.
In that case the merging makes sense.
I see that open-telemetry/build-tools#192 would potentially solve this.
So I wonder if we should wait for it then.
/cc @trisch-me @lmolkova
ChrsMark
force-pushed
the
merge_cpu_states
branch
2 times, most recently
from
57eee6d
to
beb92b9
Compare
|
Thank's @joaopgrassi. For some reason the |
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
ChrsMark
force-pushed
the
merge_cpu_states
branch
from
848fa37
to
a7e9c02
Compare
|
@open-telemetry/semconv-system-approvers could you take a look and specifically into the #1026 (comment)? Thank's. |
| <!-- NOTE: THIS FILE IS AUTOGENERATED. DO NOT EDIT BY HAND. --> | ||
| <!-- see templates/registry/markdown/attribute_namespace.md.j2 --> | ||
|
|
||
| # Cpu |
There was a problem hiding this comment.
you need to update templates/registry/markdown/weaver.yaml and add CPU to the list of acronyms
| brief: "When tasks of the cgroup are in kernel mode (Linux). When all container processes are in kernel mode (Windows)." | ||
| stability: experimental | ||
| stability: experimental | ||
| deprecated: 'Removed, report cpu state with `cpu.state` common attribute' |
There was a problem hiding this comment.
| deprecated: 'Removed, report cpu state with `cpu.state` common attribute' | |
| deprecated: 'Replaced by `cpu.state`' |
| attributes: | ||
| - id: process.cpu.state | ||
| brief: "Deprecated, use `cpu.state` instead." | ||
| deprecated: 'Removed, report cpu state with `cpu.state` common attribute' |
There was a problem hiding this comment.
| deprecated: 'Removed, report cpu state with `cpu.state` common attribute' | |
| deprecated: 'Replaced by `cpu.state`' |
| stability: experimental | ||
| brief: "Deprecated, use `cpu.state` instead." | ||
| stability: experimental | ||
| deprecated: 'Removed, report cpu state with `cpu.state` common attribute' |
There was a problem hiding this comment.
| deprecated: 'Removed, report cpu state with `cpu.state` common attribute' | |
| deprecated: 'Replaced by `cpu.state`' |
Fixes #840
Changes
This PR introduces a common
cpu.stateattribute which is a converged attribute from thesystem.cpu.state,process.cpu.stateandcontainer.cpu.state.Merge requirement checklist
[chore]