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

[receiver/windowsperfcounters] fix: include instance index for multiple matches. #32321

Merged

Conversation

alxbl
Copy link
Member

@alxbl alxbl commented Apr 11, 2024

Description: This PR adds the counter's instance index for counters that include multiple instances with the same name. A simple example of this case is for monitoring process ID when there are multiple instances of the same process running, but this can technically happen for any counter that uses instance names.

This can potentially increase cardinality so I am open to adding a configuration to the receiver to toggle this behavior on and leave it disabled as a default.

Link to tracking Issue: #32319

Testing:

  • See issue for details. Short version: two notepad processes running, scraping Process\ID Process to debug and prometheusremotewrite to validate that instance label now matches what is seen in perfmon.exe
  • Added unit test to watcher.go to validate instance name indexing

Documentation: Added enhancement CHANGELOG

Fixes #32319

@alxbl alxbl requested review from dashpole and a team as code owners April 11, 2024 13:28
Copy link

linux-foundation-easycla bot commented Apr 11, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Apr 26, 2024
@alxbl
Copy link
Member Author

alxbl commented Apr 26, 2024

/remove stale

I am in the process of testing another issue, and this PR will need to be adjusted due to the way wildcards work with instance indices.

@github-actions github-actions bot removed the Stale label Apr 27, 2024
@alxbl alxbl force-pushed the windowsperfcounters-instance-index branch from 3a1c722 to 15cde3c Compare April 30, 2024 10:13
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label May 24, 2024
@github-actions github-actions bot removed the Stale label May 25, 2024
Copy link
Contributor

github-actions bot commented Jun 8, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jun 8, 2024
Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

Looks good overall, we just need to be sure to follow the Windows convention for instance names. I did a test and the algorithm as it is would match what a call to PdhExpandCounterPath would return (as long as we use # instead of _ as explained in the comments.

.chloggen/windowsperfcounters-instance-indexing.yaml Outdated Show resolved Hide resolved
pkg/winperfcounters/watcher.go Show resolved Hide resolved
.chloggen/windowsperfcounters-instance-indexing.yaml Outdated Show resolved Hide resolved
pkg/winperfcounters/watcher.go Show resolved Hide resolved
pkg/winperfcounters/watcher.go Outdated Show resolved Hide resolved
Co-authored-by: Paulo Janotti <pjanotti@splunk.com>
Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

LGTM

@alxbl
Copy link
Member Author

alxbl commented Jun 19, 2024

/label "Run Windows"
I think we should add the label Run Windows on this PR and make sure the windows unit tests are passing before completing the PR.

I don't think I can add it myself but I've tried anyway.

@crobert-1 crobert-1 added the Run Windows Enable running windows test on a PR label Jun 20, 2024
@crobert-1
Copy link
Member

Windows failure is unrelated to this change, frequency of #33438

@crobert-1
Copy link
Member

crobert-1 commented Jun 21, 2024

Latest windows test failure is frequency of #33438

@dmitryax dmitryax merged commit d986030 into open-telemetry:main Jul 2, 2024
169 of 170 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg/winperfcounters Run Windows Enable running windows test on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[receiver/windowsperfcounters] When collecting instances with multiple matches, data is lost
5 participants