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.

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

fix: don't fail merticbeat/windows/perfmon when no data is available #42803

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+7 −1
Open

fix: don't fail merticbeat/windows/perfmon when no data is available #42803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fefed1a
fix: don't fail merticbeat/windows/perfmon when no data is available
stefans-elastic Feb 20, 2025
5a8da2a
Merge branch 'main' into iis-module-agent-issue
stefans-elastic Feb 20, 2025
f74f286
add CHANGELOG.next.asciidoc entry
stefans-elastic Feb 20, 2025
79b36e9
fix: linter issue
stefans-elastic Feb 20, 2025
4c55130
fix: linter issue
stefans-elastic Feb 20, 2025
14a215f
Merge branch 'main' into iis-module-agent-issue
pierrehilbert Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.next.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ otherwise no tag is added. {issue}42208[42208] {pull}42403[42403]
- Continue collecting metrics even if the Cisco Meraki `getDeviceLicenses` operation fails. {pull}42397[42397]
- Fixed errors in the `elasticsearch.index` metricset when index settings are missing. {issue}42424[42424] {pull}42426[42426]
- Fixed panic caused by uninitialized meraki device wifi0 and wifi1 struct pointers in the device WiFi data fetching. {issue}42745[42745] {pull}42746[42746]
- Fixed an issue in Metricbeat's Windows module where data collection would fail if the data was unavailable. {issue}42802[42802] {pull}42803[42803]

*Osquerybeat*

Expand Down
7 changes: 6 additions & 1 deletion metricbeat/module/windows/perfmon/reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,13 @@ func (re *Reader) Read() ([]mb.Event, error) {
if err := re.query.CollectData(); err != nil {
// users can encounter the case no counters are found (services/processes stopped), this should not generate an event with the error message,
//could be the case the specific services are started after and picked up by the next RefreshCounterPaths func
if err == pdh.PDH_NO_COUNTERS { //nolint:errorlint // Bad linter! This is always errno or nil.
if err == pdh.PDH_NO_COUNTERS { //nolint:errorlint // linter complains about comparing error using '==' operator but here error is always of type pdh.PdhErrno (or nil) so `errors.Is` is redundant here
re.log.Warnf("%s %v", collectFailedMsg, err)
} else if err == pdh.PDH_NO_DATA { //nolint:errorlint // the same thing as above ^
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance I could convince you to turn this into a switch statement? Usually once you need an else if a switch ends up being a little cleaner.

re.log.Warnf("%s %v", collectFailedMsg, err)

// without the return statement here it still fails when trying to get counter values
return nil, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Like for PDH_NO_COUNTERS, we still try to get values, which would return error.
Why are we trying getvalues here for this case ?

IMO, we are doing the right thing by returning when PDH_NO_DATA is hit and not propagating the error to getvalues.
thoughts on this behaviour difference between the two error handling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thoughts on this behaviour difference between the two error handling?

I'm not sure. It is the reason I've implemented it this way (instead of doing something like if err == pdh.PDH_NO_COUNTERS || err == pdh.PDH_NO_DATA) - I din't want to change original behavior since I'm not sure 'why' it was done this way. Although now that I'm thinking about this again I think it is a bug in original behavior and I probably should change to something like

if err == pdh.PDH_NO_COUNTERS ||  err == pdh.PDH_NO_DATA {
    re.log.Warnf("%s %v", collectFailedMsg, err)

    // without the return statement here it still fails when trying to get counter values
    return nil, nil
}

WDYT?

} else {
return nil, fmt.Errorf("%v: %w", collectFailedMsg, err)
}
Expand Down
Loading