interpreter: add Golang #408

florianl · 2025-03-17T17:15:36Z

Add symbolization functionality for Go executables.

fabled

Some comments added. But before just blindly starting mixing Rust, I'd like a larger discussion a comments from all maintainers on feasibility of this.

I understand using existing Rust code might be faster, and the Rust code is likely good and tested.

On the other hand this the down sides are:

core maintainers should also know Rust (possibly true?)
CGO is slower than regular Go calls. not sure of the overhead.
there is room for non-trivial errors as the calls need to use unsafe Go, potentially in non-trivial ways

Additionally go symbolization could be done directly using Go standard runtime libraries, or by extending the existing elfgopclntab code we have intree.

Could you elaborate why embedded Rust was chosen instead of the two above mentioned approaches? Do these reasons justify the overhead that mixing Go and Rust brings both in maintanenance, complexity and debugging?

interpreter/golang/golang.go

processmanager/manager.go

christos68k · 2025-03-17T19:47:42Z

Could you elaborate why embedded Rust was chosen instead of the two above mentioned approaches? Do these reasons justify the overhead that mixing Go and Rust brings both in maintanenance, complexity and debugging?

Besides the main reasons you outlined (Rust code performant/safe/well tested), using symblib allows us to bring this feature to production quickly, and solves a number of issues we have with deployed Go binaries. We do plan to also use symblib for extracting/uploading symbols from other native (non-Go) binaries, albeit this doesn't need to take place in-process in the agent, and can be a separate utility.

Regardless, I'm not opposed to doing Go symbolization in Go, assuming someone implements it we could evaluate and transition. But we don't have that right now (and at least at Elastic, have no plans to work on it) and we do have symblib.

fabled · 2025-03-17T20:54:29Z

using symblib allows us to bring this feature to production quickly, and solves a number of issues we have with deployed Go binaries

If going this way, I'd rather then use symblib as the native code dynamic symbolizer. And have it symbolize native code. We could limit to using dynamic symbols only if wanted. But getting some symbols for C-code with this would make sense. Rather than special case this for Golang only.

Regardless, I'm not opposed to doing Go symbolization in Go, assuming someone implements it we could evaluate and transition. But we don't have that right now (and at least at Elastic, have no plans to work on it) and we do have symblib.

Also looking at Rust code a bit, it seems to mmap files which is sort of good, but also previously unused. Also since Go binaries can contain C-code and Dwarf, the symblib dwarf code will get used for these executable bringing a lot of potentially new unexpected things: like automatically uncompressing the compressed dwarf to large temporary files. Edit: I missed that the Rust API used only looks at gopclntab.

IMHO the symblib code would probably need a mode the host agent can safely use for all binaries to do dynamic/elf/gopclntab symbolization but skip dwarf and compressed things. The integration as "native symbolizer" would be much cleaner than trying to special Golang.

florianl · 2025-03-18T06:59:07Z

IMHO the symblib code would probably need a mode the host agent can safely use for all binaries to do dynamic/elf/gopclntab symbolization but skip dwarf and compressed things. The integration as "native symbolizer" would be much cleaner than trying to special Golang.

That is the general idea going forward. At the moment the scope is limited and focused on Go as from this ecosystem there is the highest demand for Symbols.

florianl · 2025-03-18T08:32:47Z

interpreter/golang/golang.go

+	}
+
+	frameID := libpf.NewFrameID(libpf.NewFileID(uint64(frame.File), uint64(frame.File)),
+		libpf.AddressOrLineno(symbolsSlice[0].line_number))


I switched the AddressOrLineno part of frameID with 5bf7fd7. Previously pc was used, but this generated too much different frame IDs for the same function/source file/source line combination.

This does prevent using the data for PGO. Do you have statistics?

I didn't collect statistics. Like everywhere else for the on CPU sampling approach, leave frames are different which causes a high number of variance and differences in frame IDs.

fabled

Some additional new comments to clean up a bit.

Also, how much the profiler executable size goes up with this now that it pulls static Rust stuff?

interpreter/golang/golang.go

processmanager/manager.go

fabled · 2025-03-18T10:00:03Z

interpreter/golang/golang.go

+	frameID := libpf.NewFrameID(libpf.NewFileID(uint64(frame.File), uint64(frame.File)),
+		libpf.AddressOrLineno(symbolsSlice[0].line_number))
+
+	trace.AppendFrameID(libpf.GolangFrame, frameID)


Should this provide the mapping information similar as the non-symbolized native code is done?

At this point Go frames are treated as interpreter and no longer as native code. So I guess the answer here is no, and mapping information is not provided similar to other native code.

fabled · 2025-03-18T10:00:46Z

interpreter/golang/golang.go

+	}
+
+	frameID := libpf.NewFrameID(libpf.NewFileID(uint64(frame.File), uint64(frame.File)),
+		libpf.AddressOrLineno(symbolsSlice[0].line_number))


This does prevent using the data for PGO. Do you have statistics?

fabled · 2025-03-18T10:35:24Z

A little note that also #407 adds golang interpreter that does quite different things.

florianl · 2025-03-18T10:50:36Z

Also, how much the profiler executable size goes up with this now that it pulls static Rust stuff?

#408 @ 7616b6c

$ ls -lia ebpf-profiler 
42649440 -rwxr-xr-x 1 user user 32535648 Mar 18 11:46 ebpf-profiler

main @ 422bd6b

$ ls -lia ebpf-profiler 
42649394 -rwxr-xr-x 1 user user 30313560 Mar 18 11:48 ebpf-profiler

florianl · 2025-03-20T11:08:44Z

Force pushed to resolve merge conflict with main branch.

korniltsev · 2025-03-24T04:32:37Z

On the other hand this the down sides are:

Another potential downside is the use of malloc. Some implementations are known to not return the freed memory to the OS. Some implementations keep the freed memory in per thread arenas. This may lead to increased memory consumption, which may be amplified by the number of threads the cgo call was issued from (consider a cgo call allocates a total of 10Mib in small chunks. If we have multiple of these calls from different goroutines (and therefor threads - we do not control the thread by default)) and if we have 32 threads, eventually we may waste 320Mib.

It's possible to mitigate by locking the goroutine-thread. Nothing unsolvable, just something to keep in mind.

korniltsev · 2025-03-24T04:51:29Z

interpreter/go/go.go

+	symbolsSlice := unsafe.Slice((*C.SymblibResolvedSymbol)(unsafe.Pointer(symbols.data)),
+		symbols.len)
+	if len(symbolsSlice) != 1 {
+		return fmt.Errorf("unexpected return for point lookup: %d", len(symbolsSlice))


Why 1? Does it not resolve inlined functions?

Yes - inlined functions are not resolved atm.

Signed-off-by: Florian Lehner <[email protected]>

Co-authored-by: Timo Teräs <[email protected]>

Signed-off-by: Florian Lehner <[email protected]>

florianl · 2025-04-07T07:30:10Z

PR has been rebased on current main and conflicts got resolved.

friendly ping for feedback @fabled && @christos68k

Signed-off-by: Florian Lehner <[email protected]>

fabled

LGTM. We can revisit later if using debug/gosym is feasible when the other prerequisites for it are done first.

christos68k

LGTM, thanks!

I also tested locally and everything works as expected. I'm assuming the crashes you saw on your send were not related to pinning (otherwise they'd also manifest for me and @fabled) and I'm also assuming you can no longer trigger them on your end.

interpreter/go/go.go

processmanager/manager.go

Signed-off-by: Florian Lehner <[email protected]>

## Summary OTel Semantic Conventions [defines](open-telemetry/semantic-conventions#2003) a type for Go and OTel eBPF profiler is about to start with pushing Go frames (either with open-telemetry/opentelemetry-ebpf-profiler#409 or open-telemetry/opentelemetry-ebpf-profiler#408) FYI: @elastic/ingest-otel-data ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] ~~Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)~~ not relevant - [ ] ~~[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials~~ not relevant - [ ] ~~[Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios~~ not relevant - [ ] ~~If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)~~ not relevant - [ ] ~~This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations.~~ not relevant - [ ] ~~[Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed~~ not relevant - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - `release_note:skip` Signed-off-by: Florian Lehner <[email protected]> (cherry picked from commit 7c4af05)

# Backport This will backport the following commits from `main` to `8.19`: - [[Profiling] Add FrameType and color for Go (#215697)](#215697)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  Signed-off-by: Florian Lehner <[email protected]> Co-authored-by: Florian Lehner <[email protected]>

florianl force-pushed the interpreter-golang branch from 5ad982b to 9078329 Compare March 17, 2025 18:22

florianl marked this pull request as ready for review March 17, 2025 18:32

florianl requested review from a team as code owners March 17, 2025 18:32

fabled reviewed Mar 17, 2025

View reviewed changes

interpreter/golang/golang.go Outdated Show resolved Hide resolved

processmanager/manager.go Outdated Show resolved Hide resolved

florianl commented Mar 18, 2025

View reviewed changes

fabled reviewed Mar 18, 2025

View reviewed changes

fabled mentioned this pull request Mar 18, 2025

Custom labels for native and go #407

Closed

florianl force-pushed the interpreter-golang branch from 3222c8d to 9a26452 Compare March 20, 2025 11:08

florianl force-pushed the interpreter-golang branch 11 times, most recently from 8be0993 to 9a26452 Compare March 21, 2025 11:27

florianl mentioned this pull request Mar 21, 2025

Rust: add binary blobs #415

Merged

korniltsev reviewed Mar 24, 2025

View reviewed changes

florianl mentioned this pull request Mar 24, 2025

[Profiling] Add FrameType and color for Go elastic/kibana#215697

Merged

7 tasks

florianl and others added 14 commits April 7, 2025 08:12

fixup: drop LRU and use resolved line number instead of pc for frameID

3cccd82

Signed-off-by: Florian Lehner <[email protected]>

Update processmanager/manager.go

1210f6e

Co-authored-by: Timo Teräs <[email protected]>

fixup: drop NOP comment

d4fac4e

Signed-off-by: Florian Lehner <[email protected]>

fixup: reword Golang to Go

5ccf0b0

Signed-off-by: Florian Lehner <[email protected]>

fixup: s/Golang/Go/

db9f246

Signed-off-by: Florian Lehner <[email protected]>

fixup: fix frame ID

62a70b3

Signed-off-by: Florian Lehner <[email protected]>

fixup: en-/disable Go symbolization by flag

47c0680

Signed-off-by: Florian Lehner <[email protected]>

fixup: add Go symbolization test

4aca444

Signed-off-by: Florian Lehner <[email protected]>

fixup: avoid flaky tests

b823392

Signed-off-by: Florian Lehner <[email protected]>

Add and use ExtractAsFile for Process interface

e8d2582

fixup: build PointResolver in Loader()

c4c0d33

Signed-off-by: Florian Lehner <[email protected]>

fixup: add coredump test

b948522

Signed-off-by: Florian Lehner <[email protected]>

fixup: make linter happy

81c4ddd

Signed-off-by: Florian Lehner <[email protected]>

fixup: fix arm64 coredump tests

74ad1f9

Signed-off-by: Florian Lehner <[email protected]>

florianl force-pushed the interpreter-golang branch from df97089 to dbc4130 Compare April 7, 2025 07:14

fixup: implement (goData)Unload()

733570f

Signed-off-by: Florian Lehner <[email protected]>

florianl force-pushed the interpreter-golang branch from dbc4130 to 733570f Compare April 7, 2025 07:19

fixup: support inlined frames

b93d090

Signed-off-by: Florian Lehner <[email protected]>

florianl force-pushed the interpreter-golang branch from ac92755 to b93d090 Compare April 7, 2025 12:48

fabled approved these changes Apr 7, 2025

View reviewed changes

christos68k approved these changes Apr 7, 2025

View reviewed changes

interpreter/go/go.go Outdated Show resolved Hide resolved

processmanager/manager.go Outdated Show resolved Hide resolved

fixup: apply feedback

6356af7

Signed-off-by: Florian Lehner <[email protected]>

florianl merged commit a996c24 into main Apr 8, 2025
26 checks passed

florianl deleted the interpreter-golang branch April 8, 2025 08:18

florianl self-assigned this Apr 23, 2025

florianl mentioned this pull request Apr 24, 2025

Confusion surrounding the Symbolization process #449

Closed

gnurizen mentioned this pull request May 19, 2025

go labels dd parca-dev/opentelemetry-ebpf-profiler#70

Closed

interpreter: add Golang #408

interpreter: add Golang #408

Uh oh!

Conversation

florianl commented Mar 17, 2025

Uh oh!

fabled left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

christos68k commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabled commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

florianl commented Mar 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabled left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabled commented Mar 18, 2025

Uh oh!

florianl commented Mar 18, 2025

Uh oh!

florianl commented Mar 20, 2025

Uh oh!

korniltsev commented Mar 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

florianl commented Apr 7, 2025

Uh oh!

fabled left a comment

Choose a reason for hiding this comment

Uh oh!

christos68k left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

christos68k commented Mar 17, 2025 •

edited

Loading

fabled commented Mar 17, 2025 •

edited

Loading