[receiver/hostmetrics] Cheaper parent PID and number of threads retrieval on Windows #38589

pjanotti · 2025-03-12T23:41:36Z

(To replace PR #35337)

Reduces the cost of getting number of threads and parent process ID by calling CreateToolhelp32Snapshot to do process enumeration. The data returned by this function already includes the number of threads and parent process ID - and avoiding calling CreateToolhelp32Snapshot for every enumerated process.

Fix #32947

> go  test -benchmem -run=^$ -bench ^BenchmarkGetProcessMetadata$ -benchtime 10s
goos: windows
goarch: amd64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/receiver/hostmetricsreceiver/internal/scraper/processscraper
cpu: Intel(R) Core(TM) Ultra 7 165H
BenchmarkGetProcessMetadata/Old-IncludeParentPid-22                    3        3928594600 ns/op        28357514 B/op      13029 allocs/op
BenchmarkGetProcessMetadata/New-IncludeParentPid-22                  171          69861262 ns/op        28222817 B/op      12591 allocs/op
BenchmarkGetProcessMetadata/Old-ExcludeParentPid-22                  172          68011864 ns/op        28232550 B/op      12614 allocs/op
BenchmarkGetProcessMetadata/New-ExcludeParentPid-22                  169          72172583 ns/op        28351193 B/op      12647 allocs/op
PASS
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/hostmetricsreceiver/internal/scraper/processscraper  81.722s
> go  test -benchmem -run=^$ -bench ^BenchmarkGetProcessMetadata$ -benchtime 30s
goos: windows
goarch: amd64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/receiver/hostmetricsreceiver/internal/scraper/processscraper
cpu: Intel(R) Core(TM) Ultra 7 165H
BenchmarkGetProcessMetadata/Old-IncludeParentPid-22                    8        4115364238 ns/op        28797698 B/op      13220 allocs/op
BenchmarkGetProcessMetadata/New-IncludeParentPid-22                  514          70165003 ns/op        28702086 B/op      12800 allocs/op
BenchmarkGetProcessMetadata/Old-ExcludeParentPid-22                  552          70230804 ns/op        28558975 B/op      12750 allocs/op
BenchmarkGetProcessMetadata/New-ExcludeParentPid-22                  504          73404366 ns/op        28507275 B/op      12707 allocs/op
PASS
ok      github.com/open-telemetry/opentelemetry-collector-contrib/receiver/hostmetricsreceiver/internal/scraper/processscraper  170.587s

braydonk · 2025-03-16T14:07:58Z

Thank you for making this PR @pjanotti! It was helpful for me to understand what you meant; I didn't realize CreateToolhelp32Snapshot could be used in this way. This is awesome, I like it a lot better than what I did.

Comparing to the performance data I posted in the other PR, here's a pprof profile pulled using a similar setup.
config
profile

I accidentally removed old builds of my PR, but based on the profile I'm not sure it's particularly necessary to do a perfmon comparison; the profile shows this PR is either the same or better without increasing WMI usage.

braydonk

I'm not sure what your plan is for integrating this, but here's my thoughts.

The existing getProcessHandlesInternal function gets renamed getProcessHandlesGopsutil
getProcessHandlesInternal becomes a new function in each process_scraper_*.go platform specific file. Most just shell out to getProcessHandlesGopsutil, whereas on Windows it will be the contents of getProcessHandlesInternalNew from this PR
Debatably, this might be enough of a deviation in behaviour to constitute a featuregate. I think this is safe enough that it could start as a Beta featuregate. If we do go with a featuregate, getProcessHandlesInternal in process_scraper_windows.go would check if the featuregate enabled, and if it's not then shell out to getProcessHandlesGopsutil.

I think with those things in place, the PR would be ready to come out of draft.

braydonk · 2025-03-16T14:11:07Z

receiver/hostmetricsreceiver/internal/scraper/processscraper/process_scraper.go

-		if err != nil {
-			errs.AddPartial(0, fmt.Errorf("error reading parent pid for process %q (pid %v): %w", executable.name, pid, err))
+		parentProcessID := int32(0)
+		if !s.config.ExcludeParentPid {


I think you're right that we don't need a specific config for this. I would like to still make the check conditional, but we can make it conditional on whether the process.parent_pid resource attribute is enabled or not.

While on Windows with the new process enumeration method getting ppid no longer costs anything, on other platforms it will still be the old style of doing a certain operation per-process. It's cheap on other platforms, but as a matter of principle I do think any work this scraper does should be conditional on whether the user actually enabled that attribute/metric.

pjanotti · 2025-03-17T16:19:18Z

@braydonk I agree with your comments. I will start working on those changes later today.

pjanotti · 2025-03-18T05:07:23Z

receiver/hostmetricsreceiver/internal/scraper/processscraper/process_metadata_benchmark_test.go

+
+//go:build windows
+
+package processscraper


This file could be removed when we remove the featureflag to fallback to the old behavior.

receiver/hostmetricsreceiver/internal/scraper/processscraper/get_process_handles_windows.go

dmitryax · 2025-03-21T18:53:05Z

receiver/hostmetricsreceiver/internal/scraper/processscraper/process_scraper.go

@@ -250,7 +250,7 @@ func (s *processScraper) getProcessMetadata(ctx context.Context) ([]*processMeta
 		}

 		command, err := getProcessCommand(ctx, handle)
-		if err != nil {
+		if err != nil && !s.config.MuteProcessAllErrors {


Why do we add muting here but the related change on line 277 still unmuted? Maybe remove this from this PR to fix consistently as a separate bug_fix if needed? Just filing an issue is fine as well

Good point that is a leftover from benchmarking...

crobert-1 · 2025-03-21T19:16:49Z

Failing unittest on windows is frequency of #38860, unrelated to this PR.

…eval on Windows (open-telemetry#38589) (To replace PR open-telemetry#35337) Reduces the cost of getting number of threads and parent process ID by calling `CreateToolhelp32Snapshot` to do process enumeration. The data returned by this function already includes the number of threads and parent process ID - and avoiding calling `CreateToolhelp32Snapshot` for every enumerated process. Fix open-telemetry#32947 ```terminal > go test -benchmem -run=^$ -bench ^BenchmarkGetProcessMetadata$ -benchtime 10s goos: windows goarch: amd64 pkg: github.com/open-telemetry/opentelemetry-collector-contrib/receiver/hostmetricsreceiver/internal/scraper/processscraper cpu: Intel(R) Core(TM) Ultra 7 165H BenchmarkGetProcessMetadata/Old-IncludeParentPid-22 3 3928594600 ns/op 28357514 B/op 13029 allocs/op BenchmarkGetProcessMetadata/New-IncludeParentPid-22 171 69861262 ns/op 28222817 B/op 12591 allocs/op BenchmarkGetProcessMetadata/Old-ExcludeParentPid-22 172 68011864 ns/op 28232550 B/op 12614 allocs/op BenchmarkGetProcessMetadata/New-ExcludeParentPid-22 169 72172583 ns/op 28351193 B/op 12647 allocs/op PASS ok github.com/open-telemetry/opentelemetry-collector-contrib/receiver/hostmetricsreceiver/internal/scraper/processscraper 81.722s > go test -benchmem -run=^$ -bench ^BenchmarkGetProcessMetadata$ -benchtime 30s goos: windows goarch: amd64 pkg: github.com/open-telemetry/opentelemetry-collector-contrib/receiver/hostmetricsreceiver/internal/scraper/processscraper cpu: Intel(R) Core(TM) Ultra 7 165H BenchmarkGetProcessMetadata/Old-IncludeParentPid-22 8 4115364238 ns/op 28797698 B/op 13220 allocs/op BenchmarkGetProcessMetadata/New-IncludeParentPid-22 514 70165003 ns/op 28702086 B/op 12800 allocs/op BenchmarkGetProcessMetadata/Old-ExcludeParentPid-22 552 70230804 ns/op 28558975 B/op 12750 allocs/op BenchmarkGetProcessMetadata/New-ExcludeParentPid-22 504 73404366 ns/op 28507275 B/op 12707 allocs/op PASS ok github.com/open-telemetry/opentelemetry-collector-contrib/receiver/hostmetricsreceiver/internal/scraper/processscraper 170.587s ``` ![image](https://github.com/user-attachments/assets/95cb7f75-8ffb-43c0-ac9a-cce1c673d576)

Prototyping cheaper parent PID retrieval on Windows

b00fe53

github-actions bot requested review from braydonk and dmitryax March 12, 2025 23:41

Lint

ad957d5

pjanotti mentioned this pull request Mar 13, 2025

hostmetrics: use WMI to fetch ppid #35337

Closed

braydonk reviewed Mar 16, 2025

View reviewed changes

pjanotti added 7 commits March 17, 2025 18:25

Polishing per initial feedback

febe799

Merge branch 'main' into win-hostmetricsreceiver-cheaper-get-parent-pid

a4cd0bc

changelog

d4afa91

Change the featuregate

6cf9009

Clean initialization of wrappedProcessHandle

89700f4

Merge branch 'main' into win-hostmetricsreceiver-cheaper-get-parent-pid

5e37d3a

Fix formatting

cb86156

pjanotti marked this pull request as ready for review March 18, 2025 05:04

pjanotti requested a review from a team as a code owner March 18, 2025 05:04

github-actions bot assigned crobert-1 Mar 18, 2025

pjanotti changed the title ~~[receiver/hostmetrics] DRAFT: Prototyping cheaper parent PID retrieval on Windows~~ [receiver/hostmetrics] Cheaper parent PID and number of threads retrieval on Windows Mar 18, 2025

pjanotti commented Mar 18, 2025

View reviewed changes

pjanotti added 2 commits March 18, 2025 08:10

Merge branch 'main' into win-hostmetricsreceiver-cheaper-get-parent-pid

842dee2

Merge branch 'main' into win-hostmetricsreceiver-cheaper-get-parent-pid

9ad27b6

atoulme added the waiting-for-code-owners label Mar 20, 2025

pjanotti added the receiver/hostmetrics/internal/scraper/process label Mar 20, 2025

braydonk reviewed Mar 21, 2025

View reviewed changes

receiver/hostmetricsreceiver/internal/scraper/processscraper/get_process_handles_windows.go Outdated Show resolved Hide resolved

receiver/hostmetricsreceiver/internal/scraper/processscraper/get_process_handles_windows.go Show resolved Hide resolved

pjanotti added 2 commits March 21, 2025 10:08

Fix casing of feature flag

4ae28a7

Merge branch 'main' into win-hostmetricsreceiver-cheaper-get-parent-pid

24577fd

braydonk approved these changes Mar 21, 2025

View reviewed changes

crobert-1 removed the waiting-for-code-owners label Mar 21, 2025

dmitryax reviewed Mar 21, 2025

View reviewed changes

crobert-1 added the Run Windows Enable running windows test on a PR label Mar 21, 2025

dmitryax requested a review from braydonk March 21, 2025 19:03

Remove the extra MuteProcessAllErrors added for easy benchmarking

d43aa79

dmitryax approved these changes Mar 21, 2025

View reviewed changes

dmitryax merged commit 0c454f8 into open-telemetry:main Mar 21, 2025
198 of 200 checks passed

github-actions bot added this to the next release milestone Mar 21, 2025

pjanotti mentioned this pull request Mar 23, 2025

[receiver/hostmetrics] Use native API instead of WMI to get process.handles on Windows #38886

Merged

pjanotti deleted the win-hostmetricsreceiver-cheaper-get-parent-pid branch March 24, 2025 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[receiver/hostmetrics] Cheaper parent PID and number of threads retrieval on Windows #38589

[receiver/hostmetrics] Cheaper parent PID and number of threads retrieval on Windows #38589

pjanotti commented Mar 12, 2025 •

edited

Loading

braydonk commented Mar 16, 2025

braydonk left a comment

braydonk Mar 16, 2025

pjanotti commented Mar 17, 2025

pjanotti Mar 18, 2025

dmitryax Mar 21, 2025 •

edited

Loading

pjanotti Mar 21, 2025

pjanotti Mar 21, 2025

crobert-1 commented Mar 21, 2025


		//go:build windows

		package processscraper

[receiver/hostmetrics] Cheaper parent PID and number of threads retrieval on Windows #38589

[receiver/hostmetrics] Cheaper parent PID and number of threads retrieval on Windows #38589

Conversation

pjanotti commented Mar 12, 2025 • edited Loading

braydonk commented Mar 16, 2025

braydonk left a comment

Choose a reason for hiding this comment

braydonk Mar 16, 2025

Choose a reason for hiding this comment

pjanotti commented Mar 17, 2025

pjanotti Mar 18, 2025

Choose a reason for hiding this comment

dmitryax Mar 21, 2025 • edited Loading

Choose a reason for hiding this comment

pjanotti Mar 21, 2025

Choose a reason for hiding this comment

pjanotti Mar 21, 2025

Choose a reason for hiding this comment

crobert-1 commented Mar 21, 2025

pjanotti commented Mar 12, 2025 •

edited

Loading

dmitryax Mar 21, 2025 •

edited

Loading