[PROF-9983] Leverage local symbols for otel-profiling-agent #23

Gandem · 2024-06-19T17:41:01Z

Upload symbols for executables when symbols are available locally.

By default, this is a no-op. When DD_EXPERIMENTAL_LOCAL_SYMBOL_UPLOAD environment variable is set to true, this will kick off the symbol upload logic for each new executable found in a mapping.

The datadog uploader requires setting up DD_API_KEY and DD_SITE. For now, symbols are only uploaded for executables which contain DWARF data.

I tried keeping changes to otel-profiling-agent to a minimum, the main changes requires are:

Wiring up the uploader in execinfomanager, and
Passing the global file ID to execinfomanager (in addition to the host-local ID), to be able to pass it to the uploader, and
Including the ELF file path in pfelf.File, as passed to os.Open (so that we're able to get a file path to feed to objcopy, that will always work for containers - for e.g. mapping files path)

A few examples of a profiles remotely symbolicated thanks to the local upload:

when we only filter out fs.ErrNotExist, we get frequent failures in the local development environment with the following error: /proc/<pid>/root// is a directory we seem to think that /proc/<pid>/root// is a executable path, this happens for short-lived runc processes where /proc/<pid>/maps has "/" as the pathname for the runc executable itself. so when we try to open the elf file, we first try to read it from /proc/<pid>/map_files and (as the process is short lived) when this fails, we try to open /proc/<pid>/root// which fails with a "is a directory" error instead of fs.ErrNotExist to fix this, we just ignore all errors returned by GetElf() and only log them for debug purposes. https://github.com/elastic/otel-profiling-agent/blob/0945fe6/libpf/process/process.go#L221-L239

r1viollet · 2024-06-21T15:30:21Z

libpf/pfelf/file.go

+		// The simplest binary gcc 10 can generate ("return 0") has >= 48 bytes for each section.
+		// Let's not worry about executables that may not verify this, as they would not be of
+		// interest to us.
+		if section.Size < 32 {


good thinking

agreed it's neat - to be fair all the credit goes to the original authors of the function

otel-profiling-agent/libpf/pfelf/pfelf.go

Line 195 in 967d1d5

func HasDWARFData(elfFile *elf.File) bool {

👌

peterg17 · 2024-06-21T15:40:08Z

libpf/pfelf/file.go

@@ -49,8 +50,12 @@ const (
 	// parsed sections (e.g. symbol tables and string tables; libxul
 	// has about 4MB .dynstr)
 	maxBytesLargeSection = 16 * 1024 * 1024
+	buildIDSectionName   = ".note.gnu.build-id"


how do we treat go build id, which might be in a differently named note? I think it's maybe .note.go.build-id or something?

This is handled in:

otel-profiling-agent/libpf/pfelf/file.go

Lines 485 to 499 in 967d1d5

func (f *File) GetGoBuildID() (string, error) {

s := f.Section(".note.go.buildid")

if s == nil {

s = f.Section(".notes")

}

if s == nil {

return "", ErrNoBuildID

}

data, err := s.Data(maxBytesSmallSection)

if err != nil {

return "", err

}

return getGoBuildIDFromNotes(data)

}

I had to extract the string for the GNU build id section to a constant since the new HasDwarfData() function uses it, and the linter was yelling at me for string constants duplication 🤦

minor: the absence of build id being an error is slightly too high in log level

👍 Right now the log error is only emitted if the ELF has dwarf data but no build id, so it should be pretty rare. I'm tempted to leave it as is, and happy to follow-up on this in an ulterior PR

r1viollet · 2024-06-21T15:45:10Z

README.md

+
+### Local symbol upload (Experimental)
+
+For compiled languages (C/C++/Rust/Go), the profiling-agent can upload local symbols (when available) to Datadog for symbolication. Symbols need to be available locally (unstripped binaries).


Minor: a nice thing to support in the future could debug links and other standard locations that are usually searched.

r1viollet

LGTM
Nice feature! :-)

when we fail to initialize the datadog symbol uploader, we set the uploader to nil, which will cause a panic later on when handling a new executable for symbol upload. this sets the uploader to symbolication.NewNoopUploader() when the datadog uploader is set up, but fails to be initialized.

r1viollet

LGTM

* symbolication: support uploading local symbols remotely * readme: document experimental local symbol upload * symbolication: remove noisy error log when we only filter out fs.ErrNotExist, we get frequent failures in the local development environment with the following error: /proc/<pid>/root// is a directory we seem to think that /proc/<pid>/root// is a executable path, this happens for short-lived runc processes where /proc/<pid>/maps has "/" as the pathname for the runc executable itself. so when we try to open the elf file, we first try to read it from /proc/<pid>/map_files and (as the process is short lived) when this fails, we try to open /proc/<pid>/root// which fails with a "is a directory" error instead of fs.ErrNotExist to fix this, we just ignore all errors returned by GetElf() and only log them for debug purposes. https://github.com/elastic/otel-profiling-agent/blob/0945fe6/libpf/process/process.go#L221-L239 * uploader: fix segfault on failure to init datadog uploader (#26) when we fail to initialize the datadog symbol uploader, we set the uploader to nil, which will cause a panic later on when handling a new executable for symbol upload. this sets the uploader to symbolication.NewNoopUploader() when the datadog uploader is set up, but fails to be initialized.

Gandem changed the title ~~symbolication: support uploading local symbols remotely~~ [PROF-9983] Leverage local symbols for otel-profiling-agent Jun 19, 2024

Gandem force-pushed the nayef/local-symbols branch 5 times, most recently from c839c3e to 941648c Compare June 20, 2024 11:08

symbolication: support uploading local symbols remotely

9efa85c

Gandem force-pushed the nayef/local-symbols branch from 941648c to 9efa85c Compare June 20, 2024 11:18

readme: document experimental local symbol upload

036a0ba

Gandem marked this pull request as ready for review June 20, 2024 12:18

Gandem requested review from nsavoire and r1viollet June 20, 2024 12:19

r1viollet reviewed Jun 21, 2024

View reviewed changes

peterg17 reviewed Jun 21, 2024

View reviewed changes

r1viollet reviewed Jun 21, 2024

View reviewed changes

r1viollet previously approved these changes Jun 21, 2024

View reviewed changes

nsavoire previously approved these changes Jun 24, 2024

View reviewed changes

Gandem mentioned this pull request Jun 24, 2024

[PROF-10028] Avoid re-uploading already successfully uploaded file #24

Merged

Gandem dismissed stale reviews from nsavoire and r1viollet via d394e12 June 25, 2024 08:40

r1viollet approved these changes Jun 25, 2024

View reviewed changes

Gandem merged commit 1101921 into main Jun 25, 2024
5 checks passed

Gandem deleted the nayef/local-symbols branch June 25, 2024 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PROF-9983] Leverage local symbols for otel-profiling-agent #23

[PROF-9983] Leverage local symbols for otel-profiling-agent #23

Gandem commented Jun 19, 2024 •

edited

Loading

r1viollet Jun 21, 2024

Gandem Jun 24, 2024 •

edited

Loading

peterg17 Jun 21, 2024

Gandem Jun 24, 2024

r1viollet Jun 25, 2024

Gandem Jun 25, 2024

r1viollet Jun 21, 2024

r1viollet left a comment

r1viollet left a comment

	func (f *File) GetGoBuildID() (string, error) {
	s := f.Section(".note.go.buildid")
	if s == nil {
	s = f.Section(".notes")
	}
	if s == nil {
	return "", ErrNoBuildID
	}
	data, err := s.Data(maxBytesSmallSection)
	if err != nil {
	return "", err
	}

	return getGoBuildIDFromNotes(data)
	}


		### Local symbol upload (Experimental)

		For compiled languages (C/C++/Rust/Go), the profiling-agent can upload local symbols (when available) to Datadog for symbolication. Symbols need to be available locally (unstripped binaries).

[PROF-9983] Leverage local symbols for otel-profiling-agent #23

[PROF-9983] Leverage local symbols for otel-profiling-agent #23

Conversation

Gandem commented Jun 19, 2024 • edited Loading

r1viollet Jun 21, 2024

Choose a reason for hiding this comment

Gandem Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

peterg17 Jun 21, 2024

Choose a reason for hiding this comment

Gandem Jun 24, 2024

Choose a reason for hiding this comment

r1viollet Jun 25, 2024

Choose a reason for hiding this comment

Gandem Jun 25, 2024

Choose a reason for hiding this comment

r1viollet Jun 21, 2024

Choose a reason for hiding this comment

r1viollet left a comment

Choose a reason for hiding this comment

r1viollet left a comment

Choose a reason for hiding this comment

Gandem commented Jun 19, 2024 •

edited

Loading

Gandem Jun 24, 2024 •

edited

Loading