Try to address review issues

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

cmd/parca-agent/main.go

@@ -49,9 +49,7 @@ import (
 	"github.com/parca-dev/parca-agent/pkg/agent"
 	"github.com/parca-dev/parca-agent/pkg/debuginfo"
 	"github.com/parca-dev/parca-agent/pkg/discovery"
-	"github.com/parca-dev/parca-agent/pkg/ksym"
 	"github.com/parca-dev/parca-agent/pkg/logger"
-	"github.com/parca-dev/parca-agent/pkg/objectfile"
 	"github.com/parca-dev/parca-agent/pkg/target"
 	"github.com/parca-dev/parca-agent/pkg/template"
 )
@@ -83,7 +81,7 @@ type flags struct {
 	SystemdCgroupPath  string            `kong:"help='The cgroupfs path to a systemd slice.'"`
 }
 
-func getExternalLabels(flagExternalLabels map[string]string, flagNode string) model.LabelSet {
+func externalLabels(flagExternalLabels map[string]string, flagNode string) model.LabelSet {
 	if flagExternalLabels == nil {
 		flagExternalLabels = map[string]string{}
 	}
@@ -135,9 +133,6 @@ func main() {
 		debugInfoClient = parcadebuginfo.NewDebugInfoClient(conn)
 	}
 
-	ksymCache := ksym.NewKsymCache(logger)
-	objCache := objectfile.NewCache()
-
 	var (
 		configs discovery.Configs
 		// TODO(Sylfrena): Make ticker duration configurable
@@ -160,8 +155,13 @@ func main() {
 		))
 	}
 
-	externalLabels := getExternalLabels(flags.ExternalLabel, flags.Node)
-	tm := target.NewManager(logger, reg, ksymCache, objCache, profileListener, debugInfoClient, flags.ProfilingDuration, externalLabels, flags.TempDir)
+	tm := target.NewManager(
+		logger, reg,
+		profileListener, debugInfoClient,
+		flags.ProfilingDuration,
+		externalLabels(flags.ExternalLabel, flags.Node),
+		flags.TempDir,
+	)
 
 	mux.Handle("/metrics", promhttp.HandlerFor(reg, promhttp.HandlerOpts{}))
 	mux.HandleFunc("/debug/pprof/", pprof.Index)
@@ -233,7 +233,10 @@ func main() {
 
 			err := template.StatusPageTemplate.Execute(w, statusPage)
 			if err != nil {
-				http.Error(w, "Unexpected error occurred while rendering status page: "+err.Error(), http.StatusInternalServerError)
+				http.Error(w,
+					"Unexpected error occurred while rendering status page: "+err.Error(),
+					http.StatusInternalServerError,
+				)
 			}
 
 			return
@@ -244,7 +247,10 @@ func main() {
 			query := r.URL.Query().Get("query")
 			matchers, err := parser.ParseMetricSelector(query)
 			if err != nil {
-				http.Error(w, `query incorrectly formatted, expecting selector in form of: {name1="value1",name2="value2"}`, http.StatusBadRequest)
+				http.Error(w,
+					`query incorrectly formatted, expecting selector in form of: {name1="value1",name2="value2"}`,
+					http.StatusBadRequest,
+				)
 				return
 			}
 
@@ -256,7 +262,12 @@ func main() {
 
 			profile, err := profileListener.NextMatchingProfile(ctx, matchers)
 			if profile == nil || err == context.Canceled {
-				http.Error(w, "No profile taken in the last 11 seconds that matches the requested label-matchers query. Profiles are taken every 10 seconds so either the profiler matching the label-set has stopped profiling, or the label-set was incorrect.", http.StatusNotFound)
+				http.Error(w,
+					"No profile taken in the last 11 seconds that matches the requested label-matchers query. "+
+						"Profiles are taken every 10 seconds so either the profiler matching the label-set has stopped profiling, "+
+						"or the label-set was incorrect.",
+					http.StatusNotFound,
+				)
 				return
 			}
 			if err != nil {

go.mod

@@ -81,6 +81,7 @@ require (
 	github.com/googleapis/gnostic v0.5.5 // indirect
 	github.com/grpc-ecosystem/go-grpc-middleware/v2 v2.0.0-rc.2.0.20201207153454-9f6bf00c00a7 // indirect
 	github.com/grpc-ecosystem/grpc-gateway/v2 v2.7.3 // indirect
+	github.com/hashicorp/golang-lru v0.5.4 // indirect
 	github.com/imdario/mergo v0.3.12 // indirect
 	github.com/json-iterator/go v1.1.12 // indirect
 	github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 // indirect

pkg/debuginfo/debuginfo.go

@@ -32,6 +32,7 @@ import (
 	"github.com/containerd/containerd/sys/reaper"
 	"github.com/go-kit/log"
 	"github.com/go-kit/log/level"
+	lru "github.com/hashicorp/golang-lru"
 	"github.com/parca-dev/parca/pkg/symbol/elfutils"
 
 	"github.com/parca-dev/parca-agent/pkg/objectfile"
@@ -59,20 +60,28 @@ func NewNoopClient() Client {
 }
 
 type Extractor struct {
-	logger   log.Logger
-	client   Client
-	dbgCache *cache
-	tmpDir   string
+	logger log.Logger
+
+	client       Client
+	dbgFileCache *lru.ARCCache
+
+	tmpDir string
 
 	pool sync.Pool
 }
 
+// TODO(kakkoyun): Split extract and upload into separate layers.
+// - Use debuginfo_file for extraction related operations.
 func NewExtractor(logger log.Logger, client Client, tmpDir string) *Extractor {
+	cache, err := lru.NewARC(128) // Arbitrary cache size.
+	if err != nil {
+		level.Warn(logger).Log("msg", "failed to initialize debug file cache", "err", err)
+	}
 	return &Extractor{
-		logger:   logger,
-		client:   client,
-		tmpDir:   tmpDir,
-		dbgCache: newCache(),
+		logger:       logger,
+		client:       client,
+		tmpDir:       tmpDir,
+		dbgFileCache: cache,
 		pool: sync.Pool{
 			New: func() interface{} {
 				return bytes.NewBuffer(nil)
@@ -88,7 +97,7 @@ func (di *Extractor) Upload(ctx context.Context, objFilePaths map[string]string)
 	default:
 	}
 
-	for buildID, path := range objFilePaths {
+	for buildID, pth := range objFilePaths {
 		exists, err := di.client.Exists(ctx, buildID)
 		if err != nil {
 			level.Error(di.logger).Log("msg", "failed to check whether build ID symbol exists", "err", err)
@@ -98,31 +107,31 @@ func (di *Extractor) Upload(ctx context.Context, objFilePaths map[string]string)
 		if !exists {
 			level.Debug(di.logger).Log("msg", "could not find symbols in server", "buildid", buildID)
 
-			hasDebugInfo, err := checkDebugInfo(path)
+			hasDebugInfo, err := checkIfFileHasDebugInfo(pth)
 			if err != nil {
-				level.Debug(di.logger).Log("msg", "failed to determine whether file has debug symbols", "file", path, "err", err)
+				level.Debug(di.logger).Log("msg", "failed to determine whether file has debug symbols", "file", pth, "err", err)
 				continue
 			}
 
 			if !hasDebugInfo {
-				level.Debug(di.logger).Log("msg", "file does not have debug information, skipping", "file", path, "err", err)
+				level.Debug(di.logger).Log("msg", "file does not have debug information, skipping", "file", pth, "err", err)
 				continue
 			}
 
-			debugInfoFile, err := di.extract(ctx, buildID, path)
+			debugInfoFile, err := di.extract(ctx, buildID, pth)
 			if err != nil {
-				level.Debug(di.logger).Log("msg", "failed to extract debug information", "buildid", buildID, "file", path, "err", err)
+				level.Debug(di.logger).Log("msg", "failed to extract debug information", "buildid", buildID, "file", pth, "err", err)
 				continue
 			}
 
 			if err := di.uploadDebugInfo(ctx, buildID, debugInfoFile); err != nil {
 				os.Remove(debugInfoFile)
-				level.Error(di.logger).Log("msg", "failed to upload debug information", "buildid", buildID, "file", path, "err", err)
+				level.Error(di.logger).Log("msg", "failed to upload debug information", "buildid", buildID, "file", pth, "err", err)
 				continue
 			}
 
 			os.Remove(debugInfoFile)
-			level.Info(di.logger).Log("msg", "debug information uploaded successfully", "buildid", buildID, "file", path)
+			level.Info(di.logger).Log("msg", "debug information uploaded successfully", "buildid", buildID, "file", pth)
 			continue
 		}
 
@@ -152,40 +161,46 @@ func (di *Extractor) Extract(ctx context.Context, objFilePaths map[string]string
 	return files, nil
 }
 
-func (di *Extractor) EnsureUploaded(ctx context.Context, objFiles []*objectfile.ObjectFile) {
+func (di *Extractor) EnsureUploaded(ctx context.Context, objFiles []*objectfile.MappedObjectFile) {
 	for _, objFile := range objFiles {
 		buildID := objFile.BuildID
-		exists, err := di.client.Exists(ctx, objFile.BuildID)
+		exists, err := di.client.Exists(ctx, buildID)
 		if err != nil {
 			level.Warn(di.logger).Log("msg", "failed to check whether build ID symbol exists", "err", err)
 			continue
 		}
 
 		if !exists {
-			level.Debug(di.logger).Log("msg", "could not find symbols in server", "buildid", objFile.BuildID)
-			dbgInfoFile := di.dbgCache.debugInfoFile(objFile)
-			objFilePath := objFile.FullPath()
-			hasDebugInfo, err := dbgInfoFile.HasDebugInfo()
-			if err != nil {
-				level.Debug(di.logger).Log("msg", "failed to determine whether file has debug symbols", "file", objFilePath, "err", err)
-				continue
+			level.Debug(di.logger).Log("msg", "could not find symbols in server", "buildid", buildID)
+			var dbgInfoFile *debugInfoFile
+			if di.dbgFileCache != nil {
+				if val, ok := di.dbgFileCache.Get(buildID); ok {
+					dbgInfoFile = val.(*debugInfoFile)
+				} else {
+					f, err := newDebugInfoFile(objFile)
+					if err != nil {
+						level.Debug(di.logger).Log("msg", "failed to create debug information file", "buildid", buildID, "err", err)
+						continue
+					}
+					di.dbgFileCache.Add(buildID, f)
+					dbgInfoFile = f
+				}
 			}
-
-			if !hasDebugInfo {
+			objFilePath := objFile.Path
+			if !dbgInfoFile.hasDebugInfo {
 				// The object does not have debug symbols, but maybe debuginfos
 				// have been installed separately, typically in /usr/lib/debug, so
 				// we try to discover if there is a debuginfo file, that has the
 				// same build ID as the object.
-				level.Debug(di.logger).Log("msg", "could not find symbols in binary, checking for additional debuginfo file", "buildid", objFile.BuildID, "file", objFilePath)
-				dbgInfo, err := dbgInfoFile.LocalHostDebugInfoPath()
-				if err != nil {
-					if !errors.Is(err, errNotFound) {
-						level.Debug(di.logger).Log("msg", "failed to find additional debug information", "root", objFile.Root(), "err", err)
-					}
+				level.Debug(di.logger).Log(
+					"msg", "could not find symbols in binary, checking for additional debug info files on the system",
+					"buildid", objFile.BuildID, "file", objFilePath,
+				)
+				if dbgInfoFile.localDebugInfoPath == "" {
+					// Binary does not have debug symbols, and we could not find any on the system. Nothing to do here.
 					continue
 				}
-
-				objFilePath = dbgInfo
+				objFilePath = dbgInfoFile.localDebugInfoPath
 			}
 
 			extractedDbgInfo, err := di.extract(ctx, buildID, objFilePath)

pkg/debuginfo/debuginfo_file.go

@@ -15,12 +15,12 @@ package debuginfo
 
 import (
 	"debug/elf"
+	"errors"
 	"fmt"
 	"os"
 	"path"
 	"path/filepath"
 	"strings"
-	"sync"
 
 	"github.com/parca-dev/parca-agent/pkg/buildid"
 	"github.com/parca-dev/parca-agent/pkg/objectfile"
@@ -39,26 +39,34 @@ var dwarfSuffix = func(s *elf.Section) string {
 	}
 }
 
+// TODO(kakkoyun): Use to keep track of state of uploaded files.
+// - https://github.com/parca-dev/parca-agent/issues/256
 type debugInfoFile struct {
 	*objectfile.ObjectFile
 
-	dbgOnce      sync.Once
-	dbgErr       error
-	hasDebugInfo bool
-
-	localDbgOnce       sync.Once
-	localDbgErr        error
+	hasDebugInfo       bool
 	localDebugInfoPath string
 }
 
-func (f *debugInfoFile) LocalHostDebugInfoPath() (string, error) {
-	f.localDbgOnce.Do(func() {
-		f.localDebugInfoPath, f.localDbgErr = f.checkLocalHostDebugInfo()
-	})
-	return f.localDebugInfoPath, f.localDbgErr
+func newDebugInfoFile(file *objectfile.MappedObjectFile) (*debugInfoFile, error) {
+	ldbg, err := checkIfHostHasLocalDebugInfo(file)
+	if err != nil {
+		if !errors.Is(err, errNotFound) {
+			return nil, fmt.Errorf("failed to check if host has local debug info: %w", err)
+		}
+		// Failed to find local debug info, so make sure it's empty path.
+		ldbg = ""
+	}
+
+	hdbg, err := checkIfFileHasDebugInfo(file.Path)
+	if err != nil {
+		return nil, fmt.Errorf("failed to check if file has debug info: %w", err)
+	}
+
+	return &debugInfoFile{ObjectFile: file.ObjectFile, localDebugInfoPath: ldbg, hasDebugInfo: hdbg}, nil
 }
 
-func (f *debugInfoFile) checkLocalHostDebugInfo() (string, error) {
+func checkIfHostHasLocalDebugInfo(f *objectfile.MappedObjectFile) (string, error) {
 	var (
 		found = false
 		file  string
@@ -95,27 +103,25 @@ func (f *debugInfoFile) checkLocalHostDebugInfo() (string, error) {
 	return file, nil
 }
 
-func (f *debugInfoFile) HasDebugInfo() (bool, error) {
-	f.dbgOnce.Do(func() { f.hasDebugInfo, f.dbgErr = checkDebugInfo(f.FullPath()) })
-
-	return f.hasDebugInfo, f.dbgErr
-}
-
-func checkDebugInfo(path string) (bool, error) {
-	ef, err := elf.Open(path)
+func checkIfFileHasDebugInfo(filePath string) (bool, error) {
+	ef, err := elf.Open(filePath)
 	if err != nil {
 		return false, fmt.Errorf("failed to open elf: %w", err)
 	}
 	defer ef.Close()
 
 	for _, section := range ef.Sections {
-		if section.Type == elf.SHT_SYMTAB || // TODO: Consider moving this to a specific func.
-			strings.HasPrefix(section.Name, ".debug_") ||
-			strings.HasPrefix(section.Name, ".zdebug_") ||
-			strings.HasPrefix(section.Name, "__debug_") || // macos
-			section.Name == ".gopclntab" { // go
+		if checkIfSectionHasSymbols(section) {
 			return true, nil
 		}
 	}
 	return false, nil
 }
+
+func checkIfSectionHasSymbols(section *elf.Section) bool {
+	return section.Type == elf.SHT_SYMTAB ||
+		strings.HasPrefix(section.Name, ".debug_") ||
+		strings.HasPrefix(section.Name, ".zdebug_") ||
+		strings.HasPrefix(section.Name, "__debug_") || // macos
+		section.Name == ".gopclntab" // go
+}