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

Add process.handles to process metrics spec #142

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
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
Add process.handles to process metrics spec
Adding a new metric to the `process-metric.md` spec for process handle
count.

Signed-off-by: braydonk <braydonk@google.com>
  • Loading branch information
braydonk committed Jun 27, 2023
commit 52b69fc00ffc64b40a87569940fa0a9bed4d562e
28 changes: 15 additions & 13 deletions specification/metrics/semantic_conventions/process-metrics.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ metrics](runtime-environment-metrics.md).

<!-- toc -->

- [Metric Instruments](#metric-instruments)
* [Process](#process)
- [Attributes](#attributes)
- [Semantic Conventions for OS Process Metrics](#semantic-conventions-for-os-process-metrics)
- [Metric Instruments](#metric-instruments)
- [Process](#process)
- [Attributes](#attributes)

<!-- tocstop -->

Expand All @@ -31,17 +32,18 @@ metrics](runtime-environment-metrics.md).

Below is a table of Process metric instruments.

| Name | Instrument Type ([\*](README.md#instrument-types)) | Unit | Description | Labels |
|---------------------------------|----------------------------------------------------|-----------|-------------------------------------------------------------------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `process.cpu.time` | Counter | s | Total CPU seconds broken down by different states. | `state`, if specified, SHOULD be one of: `system`, `user`, `wait`. A process SHOULD be characterized _either_ by data points with no `state` labels, _or only_ data points with `state` labels. |
| `process.cpu.utilization` | Gauge | 1 | Difference in process.cpu.time since the last measurement, divided by the elapsed time and number of CPUs available to the process. | `state`, if specified, SHOULD be one of: `system`, `user`, `wait`. A process SHOULD be characterized _either_ by data points with no `state` labels, _or only_ data points with `state` labels. |
| `process.memory.usage` | UpDownCounter | By | The amount of physical memory in use. | |
| `process.memory.virtual` | UpDownCounter | By | The amount of committed virtual memory. | |
| `process.disk.io` | Counter | By | Disk bytes transferred. | `direction` SHOULD be one of: `read`, `write` |
| `process.network.io` | Counter | By | Network bytes transferred. | `direction` SHOULD be one of: `receive`, `transmit` |
| Name | Instrument Type ([\*](README.md#instrument-types)) | Unit | Description | Labels |
| ------------------------------- | -------------------------------------------------- | -------- | ----------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My editor did this formatting I think, should I try to revert it? Not sure if this is expected.

| `process.cpu.time` | Counter | s | Total CPU seconds broken down by different states. | `state`, if specified, SHOULD be one of: `system`, `user`, `wait`. A process SHOULD be characterized _either_ by data points with no `state` labels, _or only_ data points with `state` labels. |
| `process.cpu.utilization` | Gauge | 1 | Difference in process.cpu.time since the last measurement, divided by the elapsed time and number of CPUs available to the process. | `state`, if specified, SHOULD be one of: `system`, `user`, `wait`. A process SHOULD be characterized _either_ by data points with no `state` labels, _or only_ data points with `state` labels. |
| `process.memory.usage` | UpDownCounter | By | The amount of physical memory in use. | |
| `process.memory.virtual` | UpDownCounter | By | The amount of committed virtual memory. | |
| `process.disk.io` | Counter | By | Disk bytes transferred. | `direction` SHOULD be one of: `read`, `write` |
| `process.network.io` | Counter | By | Network bytes transferred. | `direction` SHOULD be one of: `receive`, `transmit` |
| `process.threads` | UpDownCounter | {thread} | Process threads count. | |
| `process.open_file_descriptors` | UpDownCounter | {count} | Number of file descriptors in use by the process. | |
| `process.context_switches` | Counter | {count} | Number of times the process has been context switched. | `type` SHOULD be one of: `involuntary`, `voluntary` |
| `process.open_file_descriptors` | UpDownCounter | {count} | Number of file descriptors in use by the process. | |
| `process.handles` | UpDownCounter | {count} | Number of [handles](https://learn.microsoft.com/en-us/windows/win32/sysinfo/handles-and-objects) held by the process. Windows Only. | |
Copy link
Member

Choose a reason for hiding this comment

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

For system metrics we have https://github.com/open-telemetry/semantic-conventions/blob/main/specification/metrics/semantic_conventions/system-metrics.md#systemos---os-specific-system-metrics for OS-specific metrics. If we were to apply the rule here this would be process.windows.handles. Should we follow the same guidance for process metrics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question that I don't know the answer to. Anecdotally, I'd be surprised if there were other system-exclusive process metrics, so it may look weird to follow that pattern just for one metric. But I can't conclusively say that this won't happen again, so it may be worth doing.

Copy link
Member

Choose a reason for hiding this comment

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

maybe process.win32_handles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it will be desirable to differentiate this metric from everything else, and we decide that the platform exclusive pattern isn't worth following for process metrics, then I could get behind process.win32_handles.

Copy link
Contributor

Choose a reason for hiding this comment

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

Our naming conventions would suggest either process.windows.handles or process.win32.handles.

So, this DOES look like an OS-specific metric, but also one tied to processes. I think I'd prefer to see OS-extensions directly here in the list of Process metrics, similar for OS-specific disk extensions being in the disk section.

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 wasn't sure if that was a naming convention across all of these specs or only in the system metrics, since that lived in the system metrics spec. If that is a rule we intend to apply more universally then we can apply it here.

I assume the answer to this is yes, but I'll ask anyway: is it worth doing even in the potential future where this is the only metric listed in the windows extension, and potentially the only OS extension in this document? If we are following something that's a proper rule we intend to apply across the board, then it probably is worth doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Circling back on this, should I do this? If so, I should change this in the metric definition that was contributed to hostmetricsreceiver in the collector.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we're in the process of making some breaking changes and shoring up consistency. I'd recommend we do this in as small a timeframe as possible. Additionally, like HTTP semconv, there should be a transitionary period where users can select (with config or env variable or other flag mechanism) which semconv they want (old or new)

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| `process.handles` | UpDownCounter | {count} | Number of [handles](https://learn.microsoft.com/en-us/windows/win32/sysinfo/handles-and-objects) held by the process. Windows Only. | |
| `process.handles` | UpDownCounter | {count} | Number of [handles](https://learn.microsoft.com/windows/win32/sysinfo/handles-and-objects) held by the process. Windows Only. | |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point thanks, will include this change when I rebase.

| `process.context_switches` | Counter | {count} | Number of times the process has been context switched. | `type` SHOULD be one of: `involuntary`, `voluntary` |
| `process.paging.faults` | Counter | {fault} | Number of page faults the process has made. | `type`, if specified, SHOULD be one of: `major` (for major, or hard, page faults), `minor` (for minor, or soft, page faults). |

## Attributes
Expand Down