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.

Sign up for GitHub

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

Jump to bottom

Move process metric attributes to the registry #681

Closed
wants to merge 4 commits into from
Closed

Move process metric attributes to the registry #681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
65d788a
Move process.cpu.state attribute to the registry
braydonk Feb 1, 2024
a9d5f6f
add PR link to changelog entry
braydonk Feb 1, 2024
17574a5
fix yamllint error
braydonk Feb 1, 2024
b72b784
Move additional attributes
braydonk Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 2 additions & 35 deletions model/metrics/process-metrics.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,4 @@
groups:
- id: attributes.process.cpu
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

prefix: process.cpu
type: attribute_group
brief: "Attributes for process CPU metrics."
attributes:
- id: 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."
type:
allow_custom_values: true
members:
- id: system
value: 'system'
- id: user
value: 'user'
- id: wait
value: 'wait'

- id: metric.process.cpu.time
type: metric
metric_name: process.cpu.time
Expand Down Expand Up @@ -92,15 +75,7 @@ groups:
instrument: counter
unit: "{count}"
attributes:
- id: process.context_switch_type
brief: "Specifies whether the context switches for this data point were voluntary or involuntary."
type:
allow_custom_values: true
members:
- id: voluntary
value: 'voluntary'
- id: involuntary
value: 'involuntary'
- ref: process.context_switch_type

- id: metric.process.paging.faults
type: metric
Expand All @@ -109,12 +84,4 @@ groups:
instrument: counter
unit: "{fault}"
attributes:
- id: process.paging.fault_type
brief: "The type of page fault for this data point. Type `major` is for major/hard page faults, and `minor` is for minor/soft page faults."
type:
allow_custom_values: true
members:
- id: major
value: 'major'
- id: minor
value: 'minor'
- ref: process.paging.fault_type
35 changes: 35 additions & 0 deletions model/registry/process.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should now be unblocked with #820

prefix: process
type: attribute_group
brief: >
Metric attributes that appear on some process metrics.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was requested here: #330 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would by any chance make sense to unify them? Related to #765 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@braydonk I have filed #840 for this one.

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."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

type:
allow_custom_values: true
members:
- id: system
value: 'system'
- id: user
value: 'user'
- id: wait
value: 'wait'
- id: paging.fault_type
brief: "The type of page fault for this data point. Type `major` is for major/hard page faults, and `minor` is for minor/soft page faults."
type:
allow_custom_values: true
members:
- id: major
value: 'major'
- id: minor
value: 'minor'
- id: context_switch_type
brief: "Specifies whether the context switches for this data point were voluntary or involuntary."
type:
allow_custom_values: true
members:
- id: voluntary
value: 'voluntary'
- id: involuntary
value: 'involuntary'
Loading