process metrics linux: better performance + tests #86
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves performance of Linux process metrics collection by pre-opening process files and using direct system calls instead of repeated file operations. The changes aim to reduce file I/O overhead and add comprehensive test coverage for the metrics functionality.
- Replaces repeated file reads with pre-opened file descriptors and syscall.Pread
- Adds comprehensive test suite with test data files for various process metrics
- Updates error handling to use warnings instead of errors and removes atomic logging
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| process_metrics_linux.go | Core performance improvements with pre-opened FDs and syscall optimization |
| process_metrics_linux_test.go | Complete rewrite adding comprehensive test coverage with mock data |
| testdata/linux.* | Test data files providing mock /proc filesystem data for testing |
| testdata/limits | Removed old test file, replaced with linux-specific variants |
Comments suppressed due to low confidence (2)
process_metrics_linux.go:92
- Global variable name 'STAT_START' doesn't follow Go naming conventions. It should be 'statStart' (camelCase) since it's not exported.
var STAT_START = 0
process_metrics_linux.go:93
- Global variable name 'NO_OUTPUT' doesn't follow Go naming conventions and appears unused. It should be 'noOutput' (camelCase) or removed if unused.
var NO_OUTPUT = false
| if onTest { | ||
| cwd, err := os.Getwd() | ||
| if err != nil { | ||
| panic("Unknwon current working directory: " + err.Error()) |
There was a problem hiding this comment.
There is a spelling error in the panic message. 'Unknwon' should be 'Unknown'.
| panic("Unknwon current working directory: " + err.Error()) | |
| panic("Unknown current working directory: " + err.Error()) |
Copilot uses AI. Check for mistakes.
| return nil | ||
| } | ||
| value *= 1024 | ||
| value <<= 10 |
There was a problem hiding this comment.
[nitpick] Using bit shift operator <<= 10 instead of *= 1024 makes the code less readable. While functionally equivalent, the multiplication is more explicit about converting from kB to bytes.
| value <<= 10 | |
| value *= 1024 |
Copilot uses AI. Check for mistakes.
| pm_file[FD_STAT] = f | ||
| pm_fd[FD_STAT] = int(f.Fd()) | ||
| n, err := syscall.Pread(pm_fd[FD_STAT], | ||
| (*(*[unsafe.Sizeof(data) - 1]byte)(unsafe.Pointer(&data)))[:], 0) |
There was a problem hiding this comment.
Using unsafe.Pointer to convert byte array for syscall could be dangerous. Consider using a safer approach or add bounds checking to ensure the slice doesn't exceed the array size.
| (*(*[unsafe.Sizeof(data) - 1]byte)(unsafe.Pointer(&data)))[:], 0) | |
| data[:], 0) |
Copilot uses AI. Check for mistakes.
| return | ||
| } | ||
| n, err := syscall.Pread(pm_fd[FD_STAT], | ||
| (*(*[unsafe.Sizeof(data) - 1]byte)(unsafe.Pointer(&data)))[:], 0) |
There was a problem hiding this comment.
Similar unsafe pointer usage as in init2(). The unsafe conversion should be validated or replaced with a safer alternative.
| (*(*[unsafe.Sizeof(data) - 1]byte)(unsafe.Pointer(&data)))[:], 0) | |
| data[:], 0) |
Copilot uses AI. Check for mistakes.
|
|
||
| func getOpenFDsCount(path string) (uint64, error) { | ||
| f, err := os.Open(path) | ||
| /** return 0 on error, the number of open files otherwise */ |
There was a problem hiding this comment.
Comment uses non-standard Go documentation format. Should use standard Go comment format: '// getOpenFDsCount returns 0 on error, the number of open files otherwise'
| /** return 0 on error, the number of open files otherwise */ | |
| // getOpenFDsCount returns 0 on error, the number of open files otherwise. |
Copilot uses AI. Check for mistakes.
|
|
||
| func getMaxFilesLimit(path string) (uint64, error) { | ||
| data, err := ioutil.ReadFile(path) | ||
| /* returns 0 on error, -1 for unlimited, the limit otherwise */ |
There was a problem hiding this comment.
Comment uses non-standard Go documentation format. Should use standard Go comment format: '// getMaxFilesLimit returns 0 on error, -1 for unlimited, the limit otherwise'
| /* returns 0 on error, -1 for unlimited, the limit otherwise */ | |
| // getMaxFilesLimit returns 0 on error, -1 for unlimited, the limit otherwise. |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
|
I doubt this change is needed at all - see this comment. |
| ) | ||
|
|
||
| // Testfiles in the same order as above. | ||
| var testfiles = [FD_COUNT]string{ |
There was a problem hiding this comment.
It isn't a good idea to mix the code needed for tests with the production code. Could you move this code into process_metrics_linux_test.go?
| } | ||
|
|
||
| /* | ||
| process metrics related file descriptors for files we always need, and |
There was a problem hiding this comment.
All the comments in Go source code must consistently start with // . The only exception is if the file contains a big multi-line comment. Also it looks like the comment formatting is broken here.
| type ProcFd uint32 | ||
|
|
||
| const ( | ||
| FD_LIMITS ProcFd = iota |
There was a problem hiding this comment.
Private constants in Go code must use camelCase naming instead of CAPITAL_SNAKE_CASE.
| do not want to open/close all the time | ||
| */ | ||
| var pm_fd [FD_COUNT]int |
There was a problem hiding this comment.
Private variables in Go must use camelCase naming instead of snake_case
2 participants
fixes #85