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

process: add process.status attribute #1212

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

braydonk
Copy link
Contributor

@braydonk braydonk commented Jul 8, 2024

Related Issue: #1181

Changes

This PR deprecates system.process.status and replaces it with process.status. The previous usage of system.process.status has been replaced with process.status, and process.status has also been add as a resource attribute to the process resource.

Merge requirement checklist

@braydonk braydonk requested review from a team July 8, 2024 14:02
@braydonk braydonk marked this pull request as draft July 8, 2024 14:02
This PR deprecates `system.process.status` and replaces it with
`process.status`. The previous usage of `system.process.status` has been
replaced with `process.status`, and `process.status` has also been add
as a resource attribute to the `process` resource.
@braydonk braydonk marked this pull request as ready for review July 8, 2024 14:15
attribute_map:
system.process.status: process.status
apply_to_metrics:
- system.processes.count
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we include all the other process metrics here?

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 attribute was only previously used on system.processes.count, so the rename in particular is only to that usage of the attribute. Its usage as a resource attribute is new, which means it doesn't get represented in this file (only renames not additive changes are in schema-next).

examples: [ "idle", "interrupt" ]
- id: system.process.status
type:
allow_custom_values: true
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
allow_custom_values: true

value is ignored and the flag will be removed at some point

@@ -196,3 +196,23 @@ groups:
value: 'minor'
stability: experimental
stability: experimental
- id: status
type:
allow_custom_values: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
allow_custom_values: true

same here, value is ignored

- ref: process.owner
- ref: process.status
requirement_level:
conditionally_required: See [Selecting process attributes](#selecting-process-attributes) for details.
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 Selecting process attributes applies to things that identify the process, I guess status is not one of them and would be recommended: when applicable or just recommended?

@@ -8,6 +8,7 @@ groups:
- ref: process.pid
- ref: process.parent_pid
- ref: process.executable.name
- ref: process.owner
Copy link
Contributor

Choose a reason for hiding this comment

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

oops, this change resulted in changing requirement_level on process.executable.name.

Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

Let's wait for an outcome from #1181 (comment).

@braydonk braydonk marked this pull request as draft July 17, 2024 13:07
@lmolkova
Copy link
Contributor

Based on the discussion in SemConv SIG on 8/12, the only blocker is listing process.status as a resource attribute. We can unblock this PR (if it makes sense) by defining process.status attribute in the registry, but not referencing it in process resource definition. It can also be used on metrics as a regular attribute.

@braydonk
Copy link
Contributor Author

I could do that if it would help. I don't suspect anything in the process or system namespace will use this attribute, but if there may be a use case for it I can adjust this PR to just be a definition in the registry.

CodeBlanch pushed a commit to CodeBlanch/semantic-conventions that referenced this pull request Aug 16, 2024
* Remove ordering for attributes.

* Fill in CHANGELOG link
CodeBlanch pushed a commit to CodeBlanch/semantic-conventions that referenced this pull request Aug 16, 2024
* Remove ordering for attributes.

* Fill in CHANGELOG link
@ChrsMark
Copy link
Member

Hey @braydonk, I wonder if this could be modeled as a metric instead. It was also suggested at #1032 (comment) and it seems it's already done for the hw.status : https://opentelemetry.io/docs/specs/semconv/system/hardware-metrics/#hwbattery---battery-metrics

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

Successfully merging this pull request may close these issues.

6 participants