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

Conversation

braydonk
Copy link
Contributor

Adding a new metric to the process-metric.md spec for process handle count.

Related Issues:
open-telemetry/opentelemetry-collector-contrib#22813
open-telemetry/opentelemetry-specification#3556

Note: This is the first metric in this part of the spec that is platform specific; this concept only exists on Windows, and thus the metric will inherently never be implemented on another platform. I'm not sure if anything more should be done than the little "Windows only" note in the description.

Adding a new metric to the `process-metric.md` spec for process handle
count.

Signed-off-by: braydonk <braydonk@google.com>
@braydonk braydonk requested review from a team June 27, 2023 17:36
| `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.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)

@joaopgrassi
Copy link
Member

We have now the tooling to generate the metrics from YAML files. I know it's not part of this change, but I think as much as we delay, as much painful it will be to change.

I already started doing it for a few of them. If you want to tackle it, you can see this PR I have for System Metrics #89

@braydonk
Copy link
Contributor Author

@joaopgrassi Sure, I can do that for these metrics. Might open a separate PR then rebase this one on that.

| `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.

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.

@jsuereth
Copy link
Contributor

jsuereth commented Oct 9, 2023

Hey @braydonk what's the status of this PR? Are you planning to update/make more progress here?

@braydonk
Copy link
Contributor Author

braydonk commented Oct 10, 2023

We're still going to have it in the process semantic conventions, but I'm going to re-do this PR pursuant to #160 merging so that it's in YAML instead. So I'll close this one for now, since it won't ever get merged in this form.

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

Successfully merging this pull request may close these issues.

6 participants