Move process metric attributes to the registry #681
Conversation
The guidance is for all attributes to be moved to global registry instead of being in metric specification files. This PR moves the attribute from `process.yaml` spec to the Process registry.
I was only focused on one attribute in this PR originally, but I realized there are two other attributes that also need to be moved. Moved them in this commit. Also removed the changelog as this is not a user-facing change.
braydonk
changed the title
| @@ -76,3 +76,38 @@ groups: | |||
| An additional description about the runtime of the process, for example | |||
| a specific vendor customization of the runtime environment. | |||
| examples: 'Eclipse OpenJ9 Eclipse OpenJ9 VM openj9-0.21.0' | |||
| - id: registry.process.attributes | |||
There was a problem hiding this comment.
Why isn't this nested together with the rest of attributes here? At the top there's already
- id: registry.process
prefix: process
You can define cpu.state already below the others, no?
There was a problem hiding this comment.
The above group has type resource, so I thought I had to make a separate group for attribute_group. Is that not the case?
There was a problem hiding this comment.
That was a temp fix because of broken build-tools.
There is an open PR to revert that change
There was a problem hiding this comment.
With above mentioned PR merged you could define new attribute group under top process
There was a problem hiding this comment.
Let's try to push to get that one merged, so we can move forward with this one 👍
There was a problem hiding this comment.
We found out we are still blocked by the resource/group type issue with the build tools. Folks are working on it hopefully soon we can unblock this
There was a problem hiding this comment.
It should now be unblocked with #820
| prefix: process | ||
| type: attribute_group | ||
| brief: > | ||
| Metric attributes that appear on some process metrics. |
There was a problem hiding this comment.
Since this is in the registry, it's not about only metrics anymore. Either remove this or maybe change the sentence so it's "generic"
There was a problem hiding this comment.
I don't see these attributes being used on anything other than process metrics, so I'm not sure how to reword either one to be generic. These are attributes that appear on some process metrics, and that's probably it. How should this be reworded?
There was a problem hiding this comment.
Hum.. the whole point of the registry is for re-usability. Then I'm wondering if we really need to move all of these attributes to the registry? 🤔 I'm actually now wondering where the need for this came now. I don't think other metrics have their attributes defined in the registry. Could you clarify maybe?
There was a problem hiding this comment.
This was requested here: #330 (comment)
There was a problem hiding this comment.
Hum.. the whole point of the registry is for re-usability. Then I'm wondering if we really need to move all of these attributes to the registry? 🤔 I'm actually now wondering where the need for this came now. I don't think other metrics have their attributes defined in the registry. Could you clarify maybe?
We are currently requiring all attributes in semconv to be part of the registry. This is to enable x-signal journeys where something my start as an EVENT then become a METRIC.
I think your concern is related to #394. Let's fix that issue, but no block metrics moving their attributes to the registry.
There was a problem hiding this comment.
system.cpu.state and process.cpu.state are intentionally different because despite sharing a name, their enums are distinct from one another.
There was a problem hiding this comment.
Would by any chance make sense to unify them? Related to #765 (comment)
There was a problem hiding this comment.
There are some CPU states for process.cpu.state that I don't think any realistic implementation will use. Perhaps they could be added, but they wouldn't make much sense.
In a procfs context, a process will have times for user, system, iowait, and idle. On Windows, the implementation used by hostmetrics doesn't get iowait (maybe there's a perfcounter for it or something but I'm not sure offhand, but the implementation I'm familiar with uses GetProcessTimes).
So for process.cpu.state, the attribute currently supports system, user, and wait. I think wait could be renamed to idle just fine, which would make it so process.cpu.state's enum values are a subset of system.cpu.state. From a semantic conventions perspective, maybe it's fine for cpu.state to be one attribute shared by both system and process, and when it's used in process metrics we make a note that there's a few values of the enum that are very unlikely to be recorded. Not sure how that fits from a semantic conventions perspective, but if it makes things easier maybe it's fine.
There was a problem hiding this comment.
Having one's allowed values being a subset of the other one's values makes sense to me. Not sure if this can somehow be "templated" with SemConv's rulings.
| Metric attributes that appear on some process metrics. | ||
| attributes: | ||
| - id: cpu.state | ||
| brief: "The CPU state for this data point. A process SHOULD be characterized _either_ by data points with no `state` labels, _or only_ data points with `state` labels." |
There was a problem hiding this comment.
Also here: It talks about metric things (data points) but we are in the registry. This should be adapted when you use the attribute ref in the process-metrics yaml file.
There was a problem hiding this comment.
Is it possible to add a brief with a ref? In that case I will just delete it here and add the briefs in with the refs in process-metrics.yaml
There was a problem hiding this comment.
Yeah, you can override when using ref. Like here for example: https://github.com/open-telemetry/semantic-conventions/blob/main/model/trace/http.yaml#L66
But if this is just used in metrics as you mentioned above, then leaving here is fine.
|
|
| @@ -1,21 +1,4 @@ | |||
| groups: | |||
| - id: attributes.process.cpu | |||
There was a problem hiding this comment.
I think this part should stay here and we are moving into registry only attributes i.e. state
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Not stale |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
I guess that's still valid :). |
|
@braydonk there are a lot of changes in the semconv lately. If you want to proceed with this PR please resolve conflicts |
|
closing in favor of #988 |
Fixes #650
Changes
The guidance is for all attributes to be moved to global registry instead of being in metric specification files. This PR moves all attributes from
process.yamlspec to the Process registry.Merge requirement checklist