-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[receiver/windowsperfcounters] fix: include instance index for multiple matches. #32321
Conversation
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
/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. |
pkg/winperfcounters/internal/third_party/telegraf/win_perf_counters/performance_query.go
Outdated
Show resolved
Hide resolved
3a1c722
to
15cde3c
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
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.
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.
pkg/winperfcounters/internal/third_party/telegraf/win_perf_counters/performance_query.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Paulo Janotti <pjanotti@splunk.com>
Co-authored-by: Paulo Janotti <pjanotti@splunk.com>
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.
LGTM
/label "Run Windows" I don't think I can add it myself but I've tried anyway. |
Windows failure is unrelated to this change, frequency of #33438 |
Latest windows test failure is frequency of #33438 |
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:
Process\ID Process
todebug
andprometheusremotewrite
to validate thatinstance
label now matches what is seen inperfmon.exe
Documentation: Added enhancement CHANGELOG
Fixes #32319