Replace WMI query to get the system/diskio metrics for Windows #11635

narph · 2019-04-03T16:40:28Z

Replace the current WMI query to get the system/diskio metrics for Windows

(SELECT * FROM Win32_PerfFormattedData_PerfDisk_LogicalDisk)

with DeviceIOControl Win32 API method using IOCTL_DISK_PERFORMANCE control code.

Fixes: #3798 and #2842

metricbeat/module/system/diskio/diskio_windows.go

narph · 2019-04-03T16:44:56Z

@andrewkroh , based on our recent conversation regarding the WMI query and the relevance of the diskio counters on Windows I have implemented an alternative option to get the raw metrics using DeviceIOControl instead.
Would be great to hear your feedback on this.

ruflin

Could you add a changelog entry?
Any chance to get some tests for these new methods?
Could you make hound happy where it makes sense?

@andrewkroh I wonder if some of this code should go in go-sysinfo or one of these repos?

metricbeat/module/system/diskio/diskstat_linux.go

metricbeat/module/system/diskio/diskstat_windows.go

metricbeat/module/system/diskio/diskstat_linux.go

metricbeat/module/system/diskio/diskstat_windows.go

metricbeat/module/system/diskio/diskstat_windows_helper.go

metricbeat/module/system/diskio/diskstat_windows.go

andrewkroh · 2019-04-04T16:52:17Z

@andrewkroh I wonder if some of this code should go in go-sysinfo or one of these repos?

We could expose the low-level Windows API via the https://github.com/elastic/go-windows package.

ruflin

I suggest we follow up with a PR extracting the logic into go-windows but first have it this PR for simplicity.

CHANGELOG.next.asciidoc

metricbeat/module/system/diskio/diskstat_linux.go

metricbeat/module/system/diskio/diskstat_other.go

metricbeat/module/system/diskio/diskstat_windows.go

metricbeat/module/system/diskio/diskstat_windows_helper.go

metricbeat/module/system/diskio/diskstat_other.go

metricbeat/module/system/diskio/diskstat_linux.go

metricbeat/module/system/diskio/diskstat_other.go

metricbeat/module/system/diskio/diskstat_linux.go

metricbeat/module/system/diskio/diskstat_windows_helper.go

metricbeat/module/system/diskio/diskstat_windows.go

metricbeat/module/system/diskio/diskstat_windows_helper.go

andrewkroh · 2019-04-05T15:18:32Z

metricbeat/module/system/diskio/diskstat_windows_helper.go

+	if err != nil || logicalDisks == nil {
+		return nil, err
+	}
+	ret := make(map[string]disk.IOCountersStat, 0)


You can drop the 0 parameter.

andrewkroh · 2019-04-05T15:18:46Z

metricbeat/module/system/diskio/diskstat_windows_helper.go

+		if len(drive.Name) > 3 {
+			continue
+		}
+		//filter by included devices


Suggested change

//filter by included devices

// filter by included devices

andrewkroh · 2019-04-05T15:20:40Z

metricbeat/module/system/diskio/diskstat_windows_helper.go

+			continue
+		}
+
+		var counter, err = iOCounter(drive.UNCPath)


Suggested change

var counter, err = iOCounter(drive.UNCPath)

counter, err := iOCounter(drive.UNCPath)

metricbeat/module/system/diskio/diskstat_windows_helper.go

andrewkroh

I ran the unit tests inside the win2012 vagrant VM and they failed.

PS C:\Gopath\src\github.com\elastic\beats\metricbeat\module\system\diskio> go test -v .
=== RUN   TestCDriveFilterOnWindowsTestEnv
--- FAIL: TestCDriveFilterOnWindowsTestEnv (0.00s)
    diskstat_windows_test.go:40:
                Error Trace:    diskstat_windows_test.go:40
                Error:          Should be empty, but was [disk io counters: The system cannot find the file specified.]
                Test:           TestCDriveFilterOnWindowsTestEnv
    diskstat_windows_test.go:41:
                Error Trace:    diskstat_windows_test.go:41
                Error:          Not equal:
                                expected: 1
                                actual  : 0
                Test:           TestCDriveFilterOnWindowsTestEnv
=== RUN   TestAllDrivesOnWindowsTestEnv
--- FAIL: TestAllDrivesOnWindowsTestEnv (0.00s)
    diskstat_windows_test.go:52:
                Error Trace:    diskstat_windows_test.go:52
                Error:          Should be empty, but was [disk io counters: The system cannot find the file specified.]
                Test:           TestAllDrivesOnWindowsTestEnv
    diskstat_windows_test.go:53:
                Error Trace:    diskstat_windows_test.go:53
                Error:          Should be true
                Test:           TestAllDrivesOnWindowsTestEnv
FAIL
FAIL    github.com/elastic/beats/metricbeat/module/system/diskio        0.038s

andrewkroh · 2019-04-05T15:41:01Z

metricbeat/module/system/diskio/diskstat_windows_helper.go

+	defer syscall.CloseHandle(hFile)
+
+	err = syscall.DeviceIoControl(hFile,
+		ioctlDiskPerformance,


Should we return each of the devices back to their original state when the metricset stops? Like send IOCTL_DISK_PERFORMANCE_OFF if it was previously off.

As previously discussed, for now it will be enabled as there could be other processes running on the machine that need this performance counter enabled.
As a somewhat workaround we are letting the users know in the logs that we are enabling these performance counters by adding the EnableCounterForIoctl key in the windows registry.
The function disablePerformanceCounters which will disable there perf counters has been added in the code for testing purposes only.

… counters

CHANGELOG.next.asciidoc

metricbeat/module/system/diskio/diskstat_windows_helper.go

…gistry

metricbeat/module/system/diskio/diskstat_windows_helper.go

ruflin · 2019-04-25T14:03:57Z

metricbeat/module/system/diskio/diskstat_windows.go

+
+// CloseSampling stub for linux implementation
+func (stat *DiskIOStat) CloseSampling() {
+	return


not addressed yet but also not an issue ;-)

ruflin · 2019-04-25T14:05:39Z

metricbeat/module/system/diskio/diskstat_windows_helper.go

+	return false
+}
+
+// filterLogicalDrives should filter CD-ROM type drives based on https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-getdrivetypew


This got renamed

…ub.com/narph/beats into fix-system-diskio-counters-for-windows

narph requested a review from a team as a code owner April 3, 2019 16:40

narph requested a review from andrewkroh April 3, 2019 16:40

houndci-bot reviewed Apr 3, 2019

View reviewed changes

exekias added Team:Integrations Label for the Integrations team :Windows review labels Apr 3, 2019

narph added in progress Pull request is currently in progress. and removed review labels Apr 3, 2019

ruflin reviewed Apr 4, 2019

View reviewed changes

houndci-bot reviewed Apr 4, 2019

View reviewed changes

metricbeat/module/system/diskio/diskstat_linux.go Outdated Show resolved Hide resolved

houndci-bot reviewed Apr 4, 2019

View reviewed changes

metricbeat/module/system/diskio/diskstat_windows.go Outdated Show resolved Hide resolved

houndci-bot reviewed Apr 4, 2019

View reviewed changes

metricbeat/module/system/diskio/diskstat_windows.go Outdated Show resolved Hide resolved

houndci-bot reviewed Apr 4, 2019

View reviewed changes

narph added review and removed in progress Pull request is currently in progress. labels Apr 4, 2019

narph force-pushed the fix-system-diskio-counters-for-windows branch from fbef30a to 6767dbd Compare April 5, 2019 10:38

narph requested a review from ruflin April 5, 2019 10:39

narph added the Metricbeat Metricbeat label Apr 5, 2019

ruflin reviewed Apr 5, 2019

View reviewed changes

alvarolobato added the [zube]: In Review label Apr 5, 2019

alvarolobato assigned narph Apr 5, 2019

houndci-bot reviewed Apr 5, 2019

View reviewed changes

metricbeat/module/system/diskio/diskstat_other.go Show resolved Hide resolved

metricbeat/module/system/diskio/diskstat_linux.go Show resolved Hide resolved

houndci-bot reviewed Apr 5, 2019

View reviewed changes

metricbeat/module/system/diskio/diskstat_other.go Show resolved Hide resolved

metricbeat/module/system/diskio/diskstat_linux.go Show resolved Hide resolved

narph requested a review from ruflin April 5, 2019 14:30

andrewkroh reviewed Apr 5, 2019

View reviewed changes

narph force-pushed the fix-system-diskio-counters-for-windows branch from ed5caf2 to ddc6de1 Compare April 11, 2019 08:36

narph added 8 commits April 19, 2019 14:52

Fix goimports and houndci-bot requests

6d65aa2

Fix support for osx

3d0d1ab

Re-run build, address houndci-bot messages

158682a

Update changelog file

22594d5

Addressed PR comments

73394b9

Addressing review notes

96e00d7

Enrich test and implement separate function to enable the performance…

1119a28

… counters

Add disable performance counters functionality for testing

82acba3

narph force-pushed the fix-system-diskio-counters-for-windows branch from 032ae2d to 82acba3 Compare April 19, 2019 12:54

andrewkroh reviewed Apr 22, 2019

View reviewed changes

CHANGELOG.next.asciidoc Show resolved Hide resolved

metricbeat/module/system/diskio/diskstat_windows_helper.go Outdated Show resolved Hide resolved

metricbeat/module/system/diskio/diskstat_windows_helper.go Outdated Show resolved Hide resolved

Log meesage when the EnableCounterForIoctl is added/updated in the re…

648d50f

…gistry

narph requested a review from andrewkroh April 23, 2019 09:59

andrewkroh reviewed Apr 24, 2019

View reviewed changes

metricbeat/module/system/diskio/diskstat_windows_helper.go Outdated Show resolved Hide resolved

Check for registry key value before updating it

bd04c35

narph requested a review from andrewkroh April 24, 2019 13:25

andrewkroh approved these changes Apr 24, 2019

View reviewed changes

ruflin reviewed Apr 25, 2019

View reviewed changes

Address new code reviews

eb0bf3a

narph requested a review from ruflin April 25, 2019 14:43

Merge branch 'master' into fix-system-diskio-counters-for-windows

3f0cb23

ruflin approved these changes Apr 26, 2019

View reviewed changes

narph added 2 commits April 26, 2019 13:29

small refactoring of deviceiocontrol functions

f4dae13

Merge branch 'fix-system-diskio-counters-for-windows' of https://gith…

9c219c2

…ub.com/narph/beats into fix-system-diskio-counters-for-windows

narph merged commit 9b57261 into elastic:master Apr 26, 2019

narph deleted the fix-system-diskio-counters-for-windows branch April 26, 2019 13:40

zube bot added [zube]: Done and removed [zube]: In Review labels Apr 26, 2019

narph mentioned this pull request Apr 26, 2019

Metricbeat fstat on Windows always shows zero values #2842

Closed

alvarolobato removed the [zube]: Done label Apr 29, 2019

narph mentioned this pull request Jun 27, 2019

Gosigar's WMI/COM library is leaking memory under Windows #12703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace WMI query to get the system/diskio metrics for Windows #11635

Replace WMI query to get the system/diskio metrics for Windows #11635

narph commented Apr 3, 2019 •

edited by alvarolobato

Loading

narph commented Apr 3, 2019 •

edited

Loading

ruflin left a comment

andrewkroh commented Apr 4, 2019

ruflin left a comment

andrewkroh Apr 5, 2019

andrewkroh Apr 5, 2019

andrewkroh Apr 5, 2019

andrewkroh left a comment

andrewkroh Apr 5, 2019

narph Apr 25, 2019

ruflin Apr 25, 2019

ruflin Apr 25, 2019

	var counter, err = iOCounter(drive.UNCPath)
	counter, err := iOCounter(drive.UNCPath)

Replace WMI query to get the system/diskio metrics for Windows #11635

Replace WMI query to get the system/diskio metrics for Windows #11635

Conversation

narph commented Apr 3, 2019 • edited by alvarolobato Loading

narph commented Apr 3, 2019 • edited Loading

ruflin left a comment

Choose a reason for hiding this comment

andrewkroh commented Apr 4, 2019

ruflin left a comment

Choose a reason for hiding this comment

andrewkroh Apr 5, 2019

Choose a reason for hiding this comment

andrewkroh Apr 5, 2019

Choose a reason for hiding this comment

andrewkroh Apr 5, 2019

Choose a reason for hiding this comment

andrewkroh left a comment

Choose a reason for hiding this comment

andrewkroh Apr 5, 2019

Choose a reason for hiding this comment

narph Apr 25, 2019

Choose a reason for hiding this comment

ruflin Apr 25, 2019

Choose a reason for hiding this comment

ruflin Apr 25, 2019

Choose a reason for hiding this comment

narph commented Apr 3, 2019 •

edited by alvarolobato

Loading

narph commented Apr 3, 2019 •

edited

Loading