diff --git a/libpf/convenience_test.go b/libpf/convenience_test.go index 1fd7a74f7..cf187a8e6 100644 --- a/libpf/convenience_test.go +++ b/libpf/convenience_test.go @@ -11,42 +11,6 @@ import ( "github.com/stretchr/testify/assert" ) -func TestHexTo(t *testing.T) { - tests := map[string]struct { - result uint64 - }{ - "0": {result: 0}, - "FFFFFF": {result: 16777215}, - "42": {result: 66}, - } - - for name, testcase := range tests { - name := name - testcase := testcase - t.Run(name, func(t *testing.T) { - assert.Equal(t, testcase.result, util.HexToUint64(name)) - }) - } -} - -func TestDecTo(t *testing.T) { - tests := map[string]struct { - result uint64 - }{ - "0": {result: 0}, - "123": {result: 123}, - "42": {result: 42}, - } - - for name, testcase := range tests { - name := name - testcase := testcase - t.Run(name, func(t *testing.T) { - assert.Equal(t, testcase.result, util.DecToUint64(name)) - }) - } -} - func TestIsValidString(t *testing.T) { tests := map[string]struct { input []byte diff --git a/metrics/ids.go b/metrics/ids.go index 491c2d4ff..e51b7f0cf 100644 --- a/metrics/ids.go +++ b/metrics/ids.go @@ -638,6 +638,9 @@ const ( // Number of times a trace event read failed (trace_events) IDTraceEventReadError = 274 + // Number of parsing errors seen during processing /proc//maps + IDErrProcParse = 275 + // max number of ID values, keep this as *last entry* - IDMax = 275 + IDMax = 276 ) diff --git a/metrics/metrics.json b/metrics/metrics.json index 285804981..9eba7f415 100644 --- a/metrics/metrics.json +++ b/metrics/metrics.json @@ -1978,5 +1978,12 @@ "name": "TraceEventReadError", "field": "agent.errors.trace_event_read_error", "id": 274 + }, + { + "description": "Number of parsing errors seen during processing /proc//maps", + "type": "counter", + "name": "ErrProcParse", + "field": "agent.errors.proc_parse", + "id": 275 } ] diff --git a/process/coredump.go b/process/coredump.go index c20d96af6..d868f5c82 100644 --- a/process/coredump.go +++ b/process/coredump.go @@ -258,8 +258,8 @@ func (cd *CoredumpProcess) GetMachineData() MachineData { } // GetMappings implements the Process interface -func (cd *CoredumpProcess) GetMappings() ([]Mapping, error) { - return cd.mappings, nil +func (cd *CoredumpProcess) GetMappings() ([]Mapping, uint32, error) { + return cd.mappings, 0, nil } // GetThreadInfo implements the Process interface diff --git a/process/process.go b/process/process.go index 760462bbb..e7c5e08a5 100644 --- a/process/process.go +++ b/process/process.go @@ -11,16 +11,17 @@ import ( "fmt" "io" "os" + "strconv" "strings" "sync" + log "github.com/sirupsen/logrus" "golang.org/x/sys/unix" "go.opentelemetry.io/ebpf-profiler/libpf" "go.opentelemetry.io/ebpf-profiler/libpf/pfelf" "go.opentelemetry.io/ebpf-profiler/remotememory" "go.opentelemetry.io/ebpf-profiler/stringutil" - "go.opentelemetry.io/ebpf-profiler/util" ) // systemProcess provides an implementation of the Process interface for a @@ -79,12 +80,13 @@ func trimMappingPath(path string) string { return path } -func parseMappings(mapsFile io.Reader) ([]Mapping, error) { +func parseMappings(mapsFile io.Reader) ([]Mapping, uint32, error) { + numParseErrors := uint32(0) mappings := make([]Mapping, 0, 32) scanner := bufio.NewScanner(mapsFile) scanBuf := bufPool.Get().(*[]byte) if scanBuf == nil { - return mappings, errors.New("failed to get memory from sync pool") + return mappings, 0, errors.New("failed to get memory from sync pool") } defer func() { // Reset memory and return it for reuse. @@ -101,14 +103,17 @@ func parseMappings(mapsFile io.Reader) ([]Mapping, error) { line := stringutil.ByteSlice2String(scanner.Bytes()) if stringutil.FieldsN(line, fields[:]) < 5 { + numParseErrors++ continue } if stringutil.SplitN(fields[0], "-", addrs[:]) < 2 { + numParseErrors++ continue } mapsFlags := fields[1] if len(mapsFlags) < 3 { + numParseErrors++ continue } flags := elf.ProgFlag(0) @@ -126,12 +131,31 @@ func parseMappings(mapsFile io.Reader) ([]Mapping, error) { if flags&(elf.PF_R|elf.PF_X) == 0 { continue } - inode := util.DecToUint64(fields[4]) + inode, err := strconv.ParseUint(fields[4], 10, 64) + if err != nil { + log.Debugf("inode: failed to convert %s to uint64: %v", fields[4], err) + numParseErrors++ + continue + } + path := fields[5] if stringutil.SplitN(fields[3], ":", devs[:]) < 2 { + numParseErrors++ continue } - device := util.HexToUint64(devs[0])<<8 + util.HexToUint64(devs[1]) + major, err := strconv.ParseUint(devs[0], 16, 64) + if err != nil { + log.Debugf("major device: failed to convert %s to uint64: %v", devs[0], err) + numParseErrors++ + continue + } + minor, err := strconv.ParseUint(devs[1], 16, 64) + if err != nil { + log.Debugf("minor device: failed to convert %s to uint64: %v", devs[1], err) + numParseErrors++ + continue + } + device := major<<8 + minor if inode == 0 { if path == "[vdso]" { @@ -149,18 +173,38 @@ func parseMappings(mapsFile io.Reader) ([]Mapping, error) { path = strings.Clone(path) } - vaddr := util.HexToUint64(addrs[0]) + vaddr, err := strconv.ParseUint(addrs[0], 16, 64) + if err != nil { + log.Debugf("vaddr: failed to convert %s to uint64: %v", addrs[0], err) + numParseErrors++ + continue + } + vend, err := strconv.ParseUint(addrs[1], 16, 64) + if err != nil { + log.Debugf("vend: failed to convert %s to uint64: %v", addrs[1], err) + numParseErrors++ + continue + } + length := vend - vaddr + + fileOffset, err := strconv.ParseUint(fields[2], 16, 64) + if err != nil { + log.Debugf("fileOffset: failed to convert %s to uint64: %v", fields[2], err) + numParseErrors++ + continue + } + mappings = append(mappings, Mapping{ Vaddr: vaddr, - Length: util.HexToUint64(addrs[1]) - vaddr, + Length: length, Flags: flags, - FileOffset: util.HexToUint64(fields[2]), + FileOffset: fileOffset, Device: device, Inode: inode, Path: path, }) } - return mappings, scanner.Err() + return mappings, numParseErrors, scanner.Err() } // GetMappings will process the mappings file from proc. Additionally, @@ -168,14 +212,14 @@ func parseMappings(mapsFile io.Reader) ([]Mapping, error) { // OpenELF opening ELF files using the corresponding proc map_files entry. // WARNING: This implementation does not support calling GetMappings // concurrently with itself, or with OpenELF. -func (sp *systemProcess) GetMappings() ([]Mapping, error) { +func (sp *systemProcess) GetMappings() ([]Mapping, uint32, error) { mapsFile, err := os.Open(fmt.Sprintf("/proc/%d/maps", sp.pid)) if err != nil { - return nil, err + return nil, 0, err } defer mapsFile.Close() - mappings, err := parseMappings(mapsFile) + mappings, numParseErrors, err := parseMappings(mapsFile) if err == nil { fileToMapping := make(map[string]*Mapping, len(mappings)) for idx := range mappings { @@ -189,7 +233,7 @@ func (sp *systemProcess) GetMappings() ([]Mapping, error) { } sp.fileToMapping = fileToMapping } - return mappings, err + return mappings, numParseErrors, err } func (sp *systemProcess) GetThreads() ([]ThreadInfo, error) { diff --git a/process/process_test.go b/process/process_test.go index 3cd2b7d1a..9e5689db7 100644 --- a/process/process_test.go +++ b/process/process_test.go @@ -22,11 +22,16 @@ var testMappings = `55fe82710000-55fe8273c000 r--p 00000000 fd:01 1068432 55fe82836000-55fe8283d000 r--p 00125000 fd:01 1068432 /tmp/usr_bin_seahorse 55fe8283d000-55fe8283e000 rw-p 0012c000 fd:01 1068432 /tmp/usr_bin_seahorse 7f63c8c3e000-7f63c8de0000 r-xp 00085000 08:01 1048922 /tmp/usr_lib_x86_64-linux-gnu_libcrypto.so.1.1 -7f63c8ebf000-7f63c8fef000 r-xp 0001c000 1fd:01 1075944 /tmp/usr_lib_x86_64-linux-gnu_libopensc.so.6.0.0` +7f63c8ebf000-7f63c8fef000 r-xp 0001c000 1fd:01 1075944 /tmp/usr_lib_x86_64-linux-gnu_libopensc.so.6.0.0 +7f63c8eef000-7f63c8fdf000 r-xp 0001c000 1fd:01 +7f63c8eef000-7f63c8fdf000 r-xp 0001c000 1fd.01 1075944 +7f63c8eef000-7f63c8fdf000 r- 0001c000 1fd:01 1075944 +7f63c8eef000 r-xp 0001c000 1fd:01 1075944` func TestParseMappings(t *testing.T) { - mappings, err := parseMappings(strings.NewReader(testMappings)) + mappings, numParseErrors, err := parseMappings(strings.NewReader(testMappings)) require.NoError(t, err) + require.Equal(t, uint32(4), numParseErrors) assert.NotNil(t, mappings) expected := []Mapping{ @@ -101,7 +106,8 @@ func TestNewPIDOfSelf(t *testing.T) { pr := New(libpf.PID(os.Getpid())) assert.NotNil(t, pr) - mappings, err := pr.GetMappings() + mappings, numParseErrors, err := pr.GetMappings() require.NoError(t, err) + require.Equal(t, uint32(0), numParseErrors) assert.NotEmpty(t, mappings) } diff --git a/process/types.go b/process/types.go index 55460b2e3..ffe9e59c3 100644 --- a/process/types.go +++ b/process/types.go @@ -101,7 +101,7 @@ type Process interface { GetMachineData() MachineData // GetMappings reads and parses process memory mappings - GetMappings() ([]Mapping, error) + GetMappings() ([]Mapping, uint32, error) // GetThreads reads the process thread states GetThreads() ([]ThreadInfo, error) diff --git a/processmanager/manager.go b/processmanager/manager.go index 8ea4cdf4e..fa3e083ee 100644 --- a/processmanager/manager.go +++ b/processmanager/manager.go @@ -174,6 +174,8 @@ func collectInterpreterMetrics(ctx context.Context, pm *ProcessManager, metrics.MetricValue(pm.mappingStats.maxProcParseUsec.Swap(0)) summary[metrics.IDTotalProcParseUsec] = metrics.MetricValue(pm.mappingStats.totalProcParseUsec.Swap(0)) + summary[metrics.IDErrProcParse] = + metrics.MetricValue(pm.mappingStats.numProcParseErrors.Swap(0)) mapsMetrics := pm.ebpf.CollectMetrics() for _, metric := range mapsMetrics { diff --git a/processmanager/manager_test.go b/processmanager/manager_test.go index 92f31f89d..1ca36f26e 100644 --- a/processmanager/manager_test.go +++ b/processmanager/manager_test.go @@ -48,8 +48,8 @@ func (d *dummyProcess) GetMachineData() process.MachineData { return process.MachineData{} } -func (d *dummyProcess) GetMappings() ([]process.Mapping, error) { - return nil, errors.New("not implemented") +func (d *dummyProcess) GetMappings() ([]process.Mapping, uint32, error) { + return nil, 0, errors.New("not implemented") } func (d *dummyProcess) GetThreads() ([]process.ThreadInfo, error) { diff --git a/processmanager/processinfo.go b/processmanager/processinfo.go index 9582e72a6..fa8dcf169 100644 --- a/processmanager/processinfo.go +++ b/processmanager/processinfo.go @@ -600,8 +600,9 @@ func (pm *ProcessManager) SynchronizeProcess(pr process.Process) { pm.mappingStats.numProcAttempts.Add(1) start := time.Now() - mappings, err := pr.GetMappings() + mappings, numParseErrors, err := pr.GetMappings() elapsed := time.Since(start) + pm.mappingStats.numProcParseErrors.Add(numParseErrors) if err != nil { if os.IsPermission(err) { @@ -622,7 +623,7 @@ func (pm *ProcessManager) SynchronizeProcess(pr process.Process) { pm.mappingStats.errProcNotExist.Add(1) } else if e, ok := err.(*os.PathError); ok && e.Err == syscall.ESRCH { // If the process exits while reading its /proc/$PID/maps, the kernel will - // return ESRCH. Handle it as if the process did not exists. + // return ESRCH. Handle it as if the process did not exist. pm.mappingStats.errProcESRCH.Add(1) } return diff --git a/processmanager/types.go b/processmanager/types.go index 70f7e528c..b088752bf 100644 --- a/processmanager/types.go +++ b/processmanager/types.go @@ -78,6 +78,7 @@ type ProcessManager struct { numProcAttempts atomic.Uint32 maxProcParseUsec atomic.Uint32 totalProcParseUsec atomic.Uint32 + numProcParseErrors atomic.Uint32 } // elfInfoCache provides a cache to quickly retrieve the ELF info and fileID for a particular diff --git a/util/util.go b/util/util.go index 26fa723f7..d29d87a91 100644 --- a/util/util.go +++ b/util/util.go @@ -5,36 +5,13 @@ package util // import "go.opentelemetry.io/ebpf-profiler/util" import ( "math/bits" - "strconv" "sync/atomic" "unicode" "unicode/utf8" - "github.com/sirupsen/logrus" - "go.opentelemetry.io/ebpf-profiler/libpf/hash" ) -// HexToUint64 is a convenience function to extract a hex string to a uint64 and -// not worry about errors. Essentially a "mustConvertHexToUint64". -func HexToUint64(str string) uint64 { - v, err := strconv.ParseUint(str, 16, 64) - if err != nil { - logrus.Fatalf("Failure to hex-convert %s to uint64: %v", str, err) - } - return v -} - -// DecToUint64 is a convenience function to extract a decimal string to a uint64 -// and not worry about errors. Essentially a "mustConvertDecToUint64". -func DecToUint64(str string) uint64 { - v, err := strconv.ParseUint(str, 10, 64) - if err != nil { - logrus.Fatalf("Failure to dec-convert %s to uint64: %v", str, err) - } - return v -} - // IsValidString checks if string is UTF-8-encoded and only contains expected characters. func IsValidString(s string) bool { if s == "" {