Skip to content

[receiver/hostmetrics] No viable perflib package #38858

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.

Sign up for GitHub

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

Jump to bottom
Closed
braydonk opened this issue Mar 21, 2025 · 7 comments · Fixed by #39916
Closed

[receiver/hostmetrics] No viable perflib package #38858

braydonk opened this issue Mar 21, 2025 · 7 comments · Fixed by #39916
Assignees
@braydonk
Labels
bug Something isn't working os:windows receiver/hostmetrics

Comments

@braydonk
Copy link
Contributor

Component(s)

receiver/hostmetrics

What happened?

Description

The perflib package that was used to create the perfcounter scraper went unmaintained some time ago.
In #33653 we updated to use a copy of the library in windows_exporter. However, in v0.30.0 this package was removed (moved to internal).

As such, we no longer have a viable maintained upstream package that provides perflib_exporter's original functionality for raw-querying Windows performance counters.

The way I see it, there are two options:

  1. Vendor perflib ourselves, since we rely on it and there is no longer a maintained upstream alternative
  2. Reimplement what perflib does with our own code
  3. Request that the windows_exporter maintainers make the perflib package non-internal so we can import it. This is in essence asking them to provide a level of maintenance they may not be interested in, given the effort to remove/internalize the package in the new windows_exporter version

It is possible that what the perfcounters package does can be done in a different way than what perflib does, but I simply don't have the Windows expertise to make that judgement. In the short term, I am inclined toward Option 1.

Collector version

v0.122.0

Environment information

N/A

OpenTelemetry Collector configuration

N/A

Log output

N/A

Additional context

No response
@braydonk braydonk added bug Something isn't working needs triage New item requiring triage labels Mar 21, 2025
@braydonk braydonk self-assigned this Mar 21, 2025
@braydonk braydonk mentioned this issue Mar 21, 2025
Merged
@github-actions GitHub Actions
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions GitHub Actions
Copy link
Contributor

Pinging code owners for receiver/hostmetricsreceiver: @dmitryax @braydonk. See Adding Labels via Comments if you do not have permissions to add labels yourself. For example, comment '/label priority:p2 -needs-triaged' to set the priority and remove the needs-triaged label.

@braydonk
Copy link
Contributor Author

Draft of vendoring in #38859

@braydonk braydonk mentioned this issue Mar 21, 2025
Closed
1 task
@braydonk
Copy link
Contributor Author

Pinging @pjanotti for additional Windows expertise here.

@braydonk braydonk removed the needs triage New item requiring triage label Mar 21, 2025
This was referenced Mar 23, 2025
Closed
Closed
@pjanotti
Copy link
Contributor

Thanks for the ping @braydonk - I will dig into this tomorrow.

@pjanotti
Copy link
Contributor

@braydonk the contrib has a package winperfcounters.PerfCounterWatcher that should be able to collect any performance counter needed by the hostmetricsreceiver on Windows. However, given the different API surface of the 2 approaches it seems reasonable to vendor the code in the short run and then follow-up with targeted PRs in which one by one we migrate the usage of perflib to our own package so eventually we can remove the vendored code.

@github-actions github-actions bot mentioned this issue Mar 25, 2025
Closed
@braydonk
Copy link
Contributor Author

braydonk commented Apr 2, 2025

Thanks for the insight @pjanotti
I'm going to go ahead with the vendoring approach for now then to unblock the dependabot and other issues relating to our dependency on windows_exporter.

atoulme pushed a commit that referenced this issue Apr 7, 2025
@braydonk @dehaansa
vendor perflib in perfcounterscraper (#38859)
818b8d5 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
This PR vendors the `perflib` package from the latest commit of
[`windows_exporter`](https://github.com/prometheus-community/windows_exporter).
Some constants had to be copied from related packages in
`windows_exporter` to make it work. Original license attribution to
Prometheus Authors under MIT license is included in this copy.

<!-- Issue number (e.g. #1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
#38858

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Ran tests locally, running integration tests in CI.

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Sam DeHaan <[email protected]>
dmathieu pushed a commit to dmathieu/opentelemetry-collector-contrib that referenced this issue Apr 8, 2025
@braydonk @dehaansa @dmathieu
vendor perflib in perfcounterscraper (open-telemetry#38859)
cb9cc78 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
This PR vendors the `perflib` package from the latest commit of
[`windows_exporter`](https://github.com/prometheus-community/windows_exporter).
Some constants had to be copied from related packages in
`windows_exporter` to make it work. Original license attribution to
Prometheus Authors under MIT license is included in this copy.

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
open-telemetry#38858

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Ran tests locally, running integration tests in CI.

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Sam DeHaan <[email protected]>
LucianoGiannotti pushed a commit to LucianoGiannotti/opentelemetry-collector-contrib that referenced this issue Apr 9, 2025
@braydonk @dehaansa @LucianoGiannotti
vendor perflib in perfcounterscraper (open-telemetry#38859)
643eec3 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
This PR vendors the `perflib` package from the latest commit of
[`windows_exporter`](https://github.com/prometheus-community/windows_exporter).
Some constants had to be copied from related packages in
`windows_exporter` to make it work. Original license attribution to
Prometheus Authors under MIT license is included in this copy.

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
open-telemetry#38858

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Ran tests locally, running integration tests in CI.

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Sam DeHaan <[email protected]>
Fiery-Fenix pushed a commit to Fiery-Fenix/opentelemetry-collector-contrib that referenced this issue Apr 24, 2025
@braydonk @dehaansa @Fiery-Fenix
vendor perflib in perfcounterscraper (open-telemetry#38859)
2641492 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
This PR vendors the `perflib` package from the latest commit of
[`windows_exporter`](https://github.com/prometheus-community/windows_exporter).
Some constants had to be copied from related packages in
`windows_exporter` to make it work. Original license attribution to
Prometheus Authors under MIT license is included in this copy.

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
open-telemetry#38858

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Ran tests locally, running integration tests in CI.

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Sam DeHaan <[email protected]>
@pjanotti pjanotti mentioned this issue Apr 28, 2025
Merged
dmitryax pushed a commit that referenced this issue Apr 28, 2025
@pjanotti
[chore] Add tests for scrapers using Windows perfcounters (#39690)
1058282 
Adding tests collecting the actual Windows performance counters for
scrapers using `internal/perfcounters`. The intent of these tests is to
facilitate the migration from the vendored code of
`internal/perfcounters` to the existing implementation in
`github.com/open-telemetry/opentelemetry-collector-contrib/pkg/winperfcounters`,
see
#38858 (comment)
for details.
@pjanotti pjanotti mentioned this issue May 2, 2025
Merged
atoulme pushed a commit that referenced this issue May 5, 2025
@pjanotti
[pkg/winperfcounters] Add support to retrieve raw values (#39835)
88697dc 
Adding public methods that allow to retrieve the raw values of Windows
performance counters. This is needed to allow removal of the vendored
code under `receiver/hostmetricsreceiver/internal/perfcounters/` used
only by that component. This vendored code retrieves raw values of
counters instead of calculated ones, e.g.: it doesn't retrieve rate, but
the actual values used to calculate the rate of a counter. See
#38858 (comment)
for details.
vincentfree pushed a commit to ing-bank/opentelemetry-collector-contrib that referenced this issue May 6, 2025
@pjanotti @vincentfree
[chore] Add tests for scrapers using Windows perfcounters (open-telem…
696572c 
…etry#39690)

Adding tests collecting the actual Windows performance counters for
scrapers using `internal/perfcounters`. The intent of these tests is to
facilitate the migration from the vendored code of
`internal/perfcounters` to the existing implementation in
`github.com/open-telemetry/opentelemetry-collector-contrib/pkg/winperfcounters`,
see
open-telemetry#38858 (comment)
for details.
vincentfree pushed a commit to ing-bank/opentelemetry-collector-contrib that referenced this issue May 6, 2025
@pjanotti @vincentfree
[pkg/winperfcounters] Add support to retrieve raw values (open-teleme…
be0bc7b 
…try#39835)

Adding public methods that allow to retrieve the raw values of Windows
performance counters. This is needed to allow removal of the vendored
code under `receiver/hostmetricsreceiver/internal/perfcounters/` used
only by that component. This vendored code retrieves raw values of
counters instead of calculated ones, e.g.: it doesn't retrieve rate, but
the actual values used to calculate the rate of a counter. See
open-telemetry#38858 (comment)
for details.
@pjanotti pjanotti mentioned this issue May 6, 2025
Merged
@atoulme atoulme closed this as completed in 533a1e0 May 9, 2025
vincentfree pushed a commit to ing-bank/opentelemetry-collector-contrib that referenced this issue May 20, 2025
@pjanotti @vincentfree
[pkg/winperfcounters] Add support to retrieve raw values (open-teleme…
64ad068 
…try#39835)

Adding public methods that allow to retrieve the raw values of Windows
performance counters. This is needed to allow removal of the vendored
code under `receiver/hostmetricsreceiver/internal/perfcounters/` used
only by that component. This vendored code retrieves raw values of
counters instead of calculated ones, e.g.: it doesn't retrieve rate, but
the actual values used to calculate the rate of a counter. See
open-telemetry#38858 (comment)
for details.
dragonlord93 pushed a commit to dragonlord93/opentelemetry-collector-contrib that referenced this issue May 23, 2025
@pjanotti @dragonlord93
[pkg/winperfcounters] Add support to retrieve raw values (open-teleme…
3e43054 
…try#39835)

Adding public methods that allow to retrieve the raw values of Windows
performance counters. This is needed to allow removal of the vendored
code under `receiver/hostmetricsreceiver/internal/perfcounters/` used
only by that component. This vendored code retrieves raw values of
counters instead of calculated ones, e.g.: it doesn't retrieve rate, but
the actual values used to calculate the rate of a counter. See
open-telemetry#38858 (comment)
for details.
dragonlord93 pushed a commit to dragonlord93/opentelemetry-collector-contrib that referenced this issue May 23, 2025
@pjanotti @dragonlord93
[receiver/hostmetrics] Migrate from vendored code to pkg/winperfcount…
8328a3a 
…ers (open-telemetry#39916)

Completes the migration to `pkg/winperfcounters` and removes the
vendored third party package, see
open-telemetry#38858 (comment)
for details.. Previous related changes:

- open-telemetry#39690
- open-telemetry#39835

Fixes open-telemetry#38858

Memory and CPU usage by the collector went collect these metrics were
not affected by this change, verified manually running the collector on
Windows.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@braydonk braydonk

Labels
bug Something isn't working os:windows receiver/hostmetrics
Projects
None yet
Milestone
No milestone
3 participants
@pjanotti @crobert-1 @braydonk