Move process metric attributes to the registry by braydonk · Pull Request #681 · open-telemetry/semantic-conventions

braydonk · 2024-02-01T16:18:46Z

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.yaml spec to the Process registry.

Merge requirement checklist

CONTRIBUTING.md guidelines followed.
CHANGELOG.md updated for non-trivial changes.
[N/A] schema-next.yaml updated with changes to existing conventions.

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.

CHANGELOG.md

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.

joaopgrassi · 2024-02-02T13:07:34Z

model/registry/process.yaml

          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


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?

The above group has type resource, so I thought I had to make a separate group for attribute_group. Is that not the case?

That was a temp fix because of broken build-tools.
There is an open PR to revert that change

With above mentioned PR merged you could define new attribute group under top process

Let's try to push to get that one merged, so we can move forward with this one 👍

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

It should now be unblocked with #820

joaopgrassi · 2024-02-02T13:08:20Z

model/registry/process.yaml

+    prefix: process
+    type: attribute_group
+    brief: >
+        Metric attributes that appear on some process metrics.


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"

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?

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?

This was requested here: #330 (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.

system.cpu.state and process.cpu.state are intentionally different because despite sharing a name, their enums are distinct from one another.

Would by any chance make sense to unify them? Related to #765 (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.

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.

@braydonk I have filed #840 for this one.

joaopgrassi · 2024-02-02T13:09:04Z

model/registry/process.yaml

+        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."


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.

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

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.

trisch-me · 2024-02-07T14:48:32Z

~~I think for this request would be better to create a changelog entry~~ Nevermind, it's pure refactoring, so nothing new is added and in this case we don't add new entry into changelog

trisch-me · 2024-02-08T16:50:14Z

model/metrics/process-metrics.yaml

@@ -1,21 +1,4 @@
 groups:
-  - id: attributes.process.cpu


I think this part should stay here and we are moving into registry only attributes i.e. state

github-actions · 2024-03-20T03:19:29Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

joaopgrassi · 2024-03-20T15:43:49Z

Not stale

github-actions · 2024-04-14T03:41:38Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

ChrsMark · 2024-04-18T08:32:12Z

I guess that's still valid :).

trisch-me · 2024-04-18T15:01:09Z

@braydonk there are a lot of changes in the semconv lately. If you want to proceed with this PR please resolve conflicts

lmolkova · 2024-05-01T03:21:12Z

closing in favor of #988

Move process.cpu.state attribute to the registry

65d788a

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.

braydonk requested review from a team February 1, 2024 16:18

braydonk added 2 commits February 1, 2024 16:19

add PR link to changelog entry

a9d5f6f

fix yamllint error

17574a5

github-actions bot assigned reyang Feb 1, 2024

dmitryax reviewed Feb 1, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Move additional attributes

b72b784

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 ~~Move process.cpu.state attribute to the registry~~ Move process metric attributes to the registry Feb 1, 2024

joaopgrassi requested changes Feb 2, 2024

View reviewed changes

trisch-me reviewed Feb 8, 2024

View reviewed changes

joaopgrassi mentioned this pull request Feb 29, 2024

Align system.cpu.state's definition with this of process.cpu.state #765

Merged

3 tasks

lmolkova mentioned this pull request Mar 11, 2024

Tracking semconv build-tools 0.24.0 release readiness and roll out plan #807

Closed

13 tasks

github-actions bot added the Stale label Mar 20, 2024

joaopgrassi removed the Stale label Mar 20, 2024

ChrsMark mentioned this pull request Mar 27, 2024

Consider merging *.cpu.state metric attributes #840

Closed

ChrsMark mentioned this pull request Apr 3, 2024

[chore] Move system metric attributes to the registry #867

Merged

3 tasks

github-actions bot added the Stale label Apr 14, 2024

trisch-me removed the Stale label Apr 18, 2024

trisch-me mentioned this pull request Apr 30, 2024

[chore] move process metrics to the registry #988

Merged

3 tasks

lmolkova closed this May 1, 2024

ChrsMark mentioned this pull request May 10, 2024

Merge *.cpu.state attributes to a common cpu.mode attribute #1026

Merged

3 tasks

Conversation

braydonk commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Merge requirement checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trisch-me commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 20, 2024

Uh oh!

joaopgrassi commented Mar 20, 2024

Uh oh!

github-actions bot commented Apr 14, 2024

Uh oh!

ChrsMark commented Apr 18, 2024

Uh oh!

trisch-me commented Apr 18, 2024

Uh oh!

lmolkova commented May 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

braydonk commented Feb 1, 2024 •

edited

Loading

trisch-me commented Feb 7, 2024 •

edited

Loading