-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[receiver/hostmetrics] Use native API instead of WMI to get process.handles
on Windows
#38886
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
Changes from all commits
44f846d
180a4e2
175b7ca
5da9197
4e7b325
d8687b8
08476a0
adba635
d096958
ae5326a
ea0f7b9
adb3d75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
# Use this changelog template to create an entry for release notes. | ||
|
||
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
change_type: enhancement | ||
|
||
# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) | ||
component: hostmetricsreceiver | ||
|
||
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
note: Reduced the CPU cost of collecting the `process.handles` metric on Windows. | ||
|
||
# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. | ||
issues: [38886] | ||
|
||
# (Optional) One or more lines of additional information to render under the primary note. | ||
# These lines will be padded with 2 spaces and then inserted directly into the document. | ||
# Use pipe (|) for multiline entries. | ||
subtext: | | ||
Instead of using WMI to retrieve the number of opened handles by each process | ||
the scraper now uses the GetProcessHandleCount Win32 API which results in | ||
reduced CPU usage when the metric `process.handles` is enabled. | ||
|
||
# If your change doesn't affect end users or the exported elements of any package, | ||
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. | ||
# Optional: The change log or logs in which this entry should be included. | ||
# e.g. '[user]' or '[user, api]' | ||
# Include 'user' if the change is relevant to end users. | ||
# Include 'api' if there is a change to a library API. | ||
# Default: '[user]' | ||
change_logs: [user] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// Copyright The OpenTelemetry Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
//go:build !windows | ||
|
||
package processscraper // import "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/hostmetricsreceiver/internal/scraper/processscraper" | ||
|
||
import ( | ||
"context" | ||
"errors" | ||
) | ||
|
||
const handleCountMetricsLen = 0 | ||
|
||
var ErrHandlesPlatformSupport = errors.New("process handle collection is only supported on Windows") | ||
|
||
func (p *wrappedProcessHandle) GetProcessHandleCountWithContext(_ context.Context) (int64, error) { | ||
return 0, ErrHandlesPlatformSupport | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// Copyright The OpenTelemetry Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
//go:build windows | ||
|
||
package processscraper // import "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/hostmetricsreceiver/internal/scraper/processscraper" | ||
|
||
import ( | ||
"context" | ||
) | ||
|
||
const handleCountMetricsLen = 1 | ||
|
||
func (p *wrappedProcessHandle) GetProcessHandleCountWithContext(ctx context.Context) (int64, error) { | ||
// On Windows NumFDsWithContext returns the number of open handles, since it uses the | ||
// GetProcessHandleCount API. | ||
fds, err := p.Process.NumFDsWithContext(ctx) | ||
return int64(fds), err | ||
} |
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This metric is kind of in flux in Semantic Conventions (see open-telemetry/semantic-conventions#1798) and will likely be designated Unix-exclusive for reasons stated in this comment. We're still in the process of figuring out how we'll handle all the semconv transition, but we like to get ahead of it where we can.
As a result, I think this metric should remain Linux-exclusive as it was already, and produce an error if enabled on Windows. I think this used to be the behaviour, but it inadvertently changed on a
gopsutil
upgrade when this was implemented on Windows upstream. I actually forgot this was done (and was kind of hoping they wouldn't do it this way I wanted them to introduce a different API for it but the codebase wasn't structured to allow for that at the time).I think the best thing to do here would be to introduce some validation checks in the process scraper config to fail if enabling
open_file_descriptors
on Windows, orprocess.handles
on Linux. However, this would be a breaking change; the previous behaviour is that the scrape would fail every time trying to write those metrics. Maybe I'll follow this up with a featuregated move to add validation for these metrics instead of allowing them and failing them every scrape.Sorry for the long comment, but to summarize: I think we should change this to erroring on every scrape if
process.open_file_descriptors
is enabled on Linux, similar toprocess.handles
. It can produce a platform not supported error similar to the handles one.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the context @braydonk - I agree
open_file_descriptors
should error on Windows, asprocess.handles
should error on non-Windows. I tend to think that reporting the error once at startup is better than on every scrape, but, this is something that we can discuss later.I liked your suggestion for the names:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I subscribed to the spec issue, as soon as it is settled, let's move to implement it.