Add Go version detection #485

myhro · 2023-12-05T12:44:22Z

Followed the discussions in the original issue and also in the related PR.

@grcevski I think I've tackled all your suggestions, but please let me know if there's still something missing. I'm interested in working on the integration test you mentioned as well. Should it be done here or in a separate PR?

Closes #436.

codecov-commenter · 2023-12-05T12:49:46Z

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (da4aaef) 77.23% compared to head (62a6663) 40.58%.

Files	Patch %	Lines
pkg/internal/discover/typer.go	0.00%	8 Missing ⚠️
pkg/internal/discover/attacher.go	0.00%	4 Missing ⚠️
pkg/internal/goexec/instructions.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #485       +/-   ##
===========================================
- Coverage   77.23%   40.58%   -36.65%     
===========================================
  Files          67       65        -2     
  Lines        5311     5196      -115     
===========================================
- Hits         4102     2109     -1993     
- Misses        988     2952     +1964     
+ Partials      221      135       -86

Flag	Coverage Δ
integration-test	`?`
unittests	`40.58% <38.46%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mariomac

Thank you @myhro!!

I think this PR will work in the case that the executable does not have DWARF debug info embedded, and then we look for the offsets in our prefetched database.

I think it would be simpler if the Go version fetching and checking is performed directly in the InspectOffsets public method, in the offsets.go file, right after the execElf == nil check.

However, when the executable has debug information, it follows a different path that

mariomac · 2023-12-05T16:00:11Z

pkg/internal/goexec/gofile.go

@@ -20,6 +40,12 @@ func findLibraryVersions(elfFile *elf.File) (map[string]string, error) {

 	goVersion = strings.ReplaceAll(goVersion, "go", "")
 	log().Debug("Go version detected", "version", goVersion)
+
+	if !supportedGoVersion(goVersion) {
+		log().Debug("Unsupported Go version detected. Skipping instrumentation", "version", goVersion)


This line is not required, as the error already describes the message and will be logged later.

No problem. I'll drop it.

mariomac · 2023-12-05T16:02:52Z

pkg/internal/goexec/structmembers.go

@@ -115,8 +115,12 @@ func structMemberOffsets(elfFile *elf.File) (FieldOffsets, error) {
 	var expected map[string]struct{}
 	dwarfData, err := elfFile.DWARF()
 	if err == nil {
-		offs, expected = structMemberOffsetsFromDwarf(dwarfData)
+		_, _, err := getGoDetails(elfFile)


Sorry, why is this needed?

I believe we need this because the library versions are only checked if we can't find the symbols, in structMemberPreFetchedOffsets. If the executable has symbols and it's compiled with older Go version we won't check otherwise.

This suggestion came from the discussion in the related PR:

Finally, I think we should also call the getGoDetails() function at this line here:

beyla/pkg/internal/goexec/structmembers.go

Line 123 in 760d3ad

return offs, nil

In case we find go symbols in the ELF we wouldn't bother checking for what's the Go version, we trust what the ELF says the offsets are. A simple call with only caring about the returned err should suffice. If we get an error there we can just propagate it upward to make the go instrumentation fail.

My understanding is that there's not much of a point in processing the binary further is this check fails. At the same time, as it's not 100% related to this PR, I can drop it if you prefer as well.

grcevski

LGTM! This looks good to me! Thanks for working on this @myhro!

I think we can make a follow-up integration test in another PR. I need to add a Docker image of older Go binary to our test repo to be able to use it. I'll tag you in a PR once I have that done.

myhro

I think we can make a follow-up integration test in another PR. I need to add a Docker image of older Go binary to our test repo to be able to use it. I'll tag you in a PR once I have that done.

Awesome! That's really great. I'll try to understand the integration tests setup in the meantime.

I think it would be simpler if the Go version fetching and checking is performed directly in the InspectOffsets public method, in the offsets.go file, right after the execElf == nil check.

You mean calling getGoDetails() here?

beyla/pkg/internal/goexec/offsets.go

Lines 25 to 28 in 289dec9

    
           func InspectOffsets(execElf *exec.FileInfo, funcs []string) (*Offsets, error) { 
        
           	if execElf == nil { 
        
           		return nil, fmt.Errorf("executable not found") 
        
           	}

I think it should work and maybe the implementation could turn out to be even cleaner, but I'm not sure if we would avoid calling getGoDetails() again down the execution path. What do you think, @grcevski ?

myhro · 2023-12-05T16:38:09Z

pkg/internal/goexec/gofile.go

@@ -20,6 +40,12 @@ func findLibraryVersions(elfFile *elf.File) (map[string]string, error) {

 	goVersion = strings.ReplaceAll(goVersion, "go", "")
 	log().Debug("Go version detected", "version", goVersion)
+
+	if !supportedGoVersion(goVersion) {
+		log().Debug("Unsupported Go version detected. Skipping instrumentation", "version", goVersion)


No problem. I'll drop it.

myhro · 2023-12-05T16:48:36Z

pkg/internal/goexec/structmembers.go

@@ -115,8 +115,12 @@ func structMemberOffsets(elfFile *elf.File) (FieldOffsets, error) {
 	var expected map[string]struct{}
 	dwarfData, err := elfFile.DWARF()
 	if err == nil {
-		offs, expected = structMemberOffsetsFromDwarf(dwarfData)
+		_, _, err := getGoDetails(elfFile)


This suggestion came from the discussion in the related PR:

Finally, I think we should also call the getGoDetails() function at this line here:

beyla/pkg/internal/goexec/structmembers.go

Line 123 in 760d3ad

return offs, nil

In case we find go symbols in the ELF we wouldn't bother checking for what's the Go version, we trust what the ELF says the offsets are. A simple call with only caring about the returned err should suffice. If we get an error there we can just propagate it upward to make the go instrumentation fail.

My understanding is that there's not much of a point in processing the binary further is this check fails. At the same time, as it's not 100% related to this PR, I can drop it if you prefer as well.

grcevski · 2023-12-06T22:36:58Z

Hey @myhro, so now that we have #490 merged, you can pull 'main' into this PR and remove the SKIP_GO_TRACERS from the 1.16 test. That should be the final proof this works :).

Thanks a bunch for all the work and reviewing my test PR.

myhro · 2023-12-07T01:30:55Z

@grcevski thanks for preparing the integration tests. I've dropped the BEYLA_SKIP_GO_SPECIFIC_TRACERS, but it didn't work as expected.

I'll ping you as soon as the CI checks turn green. Right now I'm trying to figure what's causing the error below:

=== RUN   TestSuite_UnsupportedGoVersion/RED_metrics/http://localhost:8080
    eventually.go:81: 
        	Error Trace:	/home/runner/work/beyla/beyla/test/integration/test_utils.go:108
        	            				/home/runner/work/beyla/beyla/vendor/github.com/mariomac/guara/pkg/test/eventually.go:57
        	            				/opt/hostedtoolcache/go/1.21.4/x64/src/runtime/asm_amd64.s:1650
        	Error:      	Should NOT be empty, but was []
        
--- FAIL: TestSuite_UnsupportedGoVersion (66.04s)
    --- FAIL: TestSuite_UnsupportedGoVersion/RED_metrics (60.00s)
        --- FAIL: TestSuite_UnsupportedGoVersion/RED_metrics/http://localhost:8080 (60.00s)
FAIL
FAIL	github.com/grafana/beyla/test/integration	596.992s

grcevski · 2023-12-07T01:49:31Z

Ah, I think then maybe the fix doesn't work as I expected it would. The test error says we couldn't find events in Prometheus based on the instrumentation, since we likely still instrumented as a Go application, but it's too old of a version.

I will also look more tomorrow, we might be missing something...

myhro · 2023-12-07T03:08:00Z

Yeah, I think there's a misunderstanding on how the Go version is discovered. I've recompiled the beyla binary from the main branch (to be sure I didn't mess anything up) and tried the following binaries:

testserver_1.16 from the Docker image.
testserver_1.17 compiled with Go 1.17.13.
testserver compiled with Go 1.21.5.

The getGoDetails() function, which returns the Go version, is only called from findLibraryVersions():

beyla/pkg/internal/goexec/gofile.go

Line 16 in 54209a7

goVersion, modules, err := getGoDetails(elfFile)

findLibraryVersions() is only called from structMemberPreFetchedOffsets():

beyla/pkg/internal/goexec/structmembers.go

Line 142 in 54209a7

libVersions, err := findLibraryVersions(elfFile)

structMemberPreFetchedOffsets(), in turn, would only be called in the last line of structMemberOffsets():

beyla/pkg/internal/goexec/structmembers.go

Line 133 in 54209a7

return structMemberPreFetchedOffsets(elfFile, offs)

But it is actually never called, because on all three binaries it returns earlier, on line 123, because the expected map never has a length greater than 0 in any of these cases:

beyla/pkg/internal/goexec/structmembers.go

Lines 120 to 123 in 54209a7

    
           if len(expected) > 0 { 
        
           	log().Debug("Fields not found in the DWARF file", "fields", expected) 
        
           } else { 
        
           	return offs, nil

So I guess you were right that we need a check before return offs, nil. But right now I'm not sure if checking for an error from getGoDetails() is the way to go. This function is not erroring at all on Go 1.16 and it apparently can correctly detect its version and modules, as it can be seen when recompiling the beyla binary with the changes in this PR:

> github.com/grafana/beyla/pkg/internal/goexec.getGoDetails() ./pkg/internal/goexec/gofile.go:97 (PC: 0x957728)
Warning: debugging optimized function
    92:                         readPtr = bo.Uint64
    93:                 }
    94:                 vers = readString(f, ptrSize, readPtr, readPtr(data[16:]))
    95:                 mod = readString(f, ptrSize, readPtr, readPtr(data[16+ptrSize:]))
    96:         }
=>  97:         if vers == "" {
    98:                 return "", "", errors.New("not a Go executable")
    99:         }
   100:         if len(mod) >= 33 && mod[len(mod)-17] == '\n' {
   101:                 // Strip module framing: sentinel strings delimiting the module info.
   102:                 // These are cmd/go/internal/modload.infoStart and infoEnd.
(dlv) locals
data = (unreadable could not find loclist entry at 0x734399 for address 0x957728)
err2 = (unreadable could not find loclist entry at 0x734609 for address 0x957728)
vers = "go1.16.15"
mod = "0w�\f�t\b\x02A��\a��\x18�path\tcommand-line-arguments\nmod\tgithub.com/grafa...+1774 more"

I'll also continue tomorrow, as it's getting late for me as well. Please let me know if I'm missing anything.

These debugging sessions were made by running beyla under dlv exec with the least amount of configs:

export BEYLA_LOG_LEVEL="DEBUG"
export BEYLA_OPEN_PORT="8080"
export BEYLA_PRINT_TRACES="true"

grcevski · 2023-12-07T17:00:30Z

I have some proposed changes that should make this work as expected now here: https://github.com/myhro/beyla/pull/1/files. It was a bit more complex as I expected :). Moving the check earlier worked, but since we detected the executable type as Go we still tried to instrument the program as a Go program and failed since it's not supported. This was better than before, since we now said in the Debug logs that the program is ignored, but we didn't fall back on using the generic instrumentation.

I added support to remember the error and use that to activate the generic instrumentation for the Go program. With appropriate warning, so that customers see this in the logs even with default logging.

myhro · 2023-12-07T17:35:45Z

I've just merged your PR, @grcevski. Thank you very much for going deep into the fix!

I see that there are some linter complaints, which probably weren't caught because the GitHub Actions workflows weren't executed in the fork. I'll take a look into them and push the fixes as soon as the integration tests CI check turns green.

myhro · 2023-12-07T18:03:20Z

Integrations tests are green now. I've fixed the indent-error-flow linter complaint and ignored the cyclop one. Please let me know if you think we should do it differently.

myhro · 2023-12-11T14:55:42Z

As I don't have permissions to re-run the CI checks, I've amended the last commit to trick it into re-running them.

Do you think we are good to go, @grcevski? Or do we need anything else?

mariomac · 2023-12-11T15:09:36Z

LGTM! Thank you @myhro for your contribution

myhro requested review from grcevski and mariomac as code owners December 5, 2023 12:44

mariomac requested changes Dec 5, 2023

View reviewed changes

grcevski approved these changes Dec 5, 2023

View reviewed changes

myhro commented Dec 5, 2023

View reviewed changes

myhro force-pushed the go-version-detection-436 branch from 270c18f to 20bc64b Compare December 5, 2023 17:01

myhro mentioned this pull request Dec 6, 2023

[WIP] Add Integration tests for Go 1.16 #489

Closed

myhro force-pushed the go-version-detection-436 branch from 20bc64b to 80071be Compare December 7, 2023 01:03

grcevski mentioned this pull request Dec 7, 2023

Add test for unsupported Go #490

Merged

myhro mentioned this pull request Dec 7, 2023

Move detection code earlier and report. myhro/beyla#1

Merged

myhro and others added 3 commits December 7, 2023 14:53

Add Go version detection

a806daa

Don't skip Go tracers on 1.16 integration tests

e76cafb

Move detection code earlier and report.

ad818e9

myhro force-pushed the go-version-detection-436 branch from 3a5ca0e to b964977 Compare December 7, 2023 18:03

Fix linter errors

62a6663

myhro force-pushed the go-version-detection-436 branch from b964977 to 62a6663 Compare December 11, 2023 14:19

mariomac approved these changes Dec 11, 2023

View reviewed changes

mariomac merged commit 23b60bd into grafana:main Dec 11, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Go version detection #485

Add Go version detection #485

myhro commented Dec 5, 2023

codecov-commenter commented Dec 5, 2023 •

edited

Loading

mariomac left a comment

mariomac Dec 5, 2023

myhro Dec 5, 2023

mariomac Dec 5, 2023

grcevski Dec 5, 2023

myhro Dec 5, 2023

grcevski left a comment

myhro left a comment

myhro Dec 5, 2023

myhro Dec 5, 2023

grcevski commented Dec 6, 2023

myhro commented Dec 7, 2023

grcevski commented Dec 7, 2023

myhro commented Dec 7, 2023

grcevski commented Dec 7, 2023

myhro commented Dec 7, 2023

myhro commented Dec 7, 2023

myhro commented Dec 11, 2023

mariomac commented Dec 11, 2023

	func InspectOffsets(execElf exec.FileInfo, funcs []string) (Offsets, error) {
	if execElf == nil {
	return nil, fmt.Errorf("executable not found")
	}

Add Go version detection #485

Add Go version detection #485

Conversation

myhro commented Dec 5, 2023

codecov-commenter commented Dec 5, 2023 • edited Loading

Codecov Report

mariomac left a comment

Choose a reason for hiding this comment

mariomac Dec 5, 2023

Choose a reason for hiding this comment

myhro Dec 5, 2023

Choose a reason for hiding this comment

mariomac Dec 5, 2023

Choose a reason for hiding this comment

grcevski Dec 5, 2023

Choose a reason for hiding this comment

myhro Dec 5, 2023

Choose a reason for hiding this comment

grcevski left a comment

Choose a reason for hiding this comment

myhro left a comment

Choose a reason for hiding this comment

myhro Dec 5, 2023

Choose a reason for hiding this comment

myhro Dec 5, 2023

Choose a reason for hiding this comment

grcevski commented Dec 6, 2023

myhro commented Dec 7, 2023

grcevski commented Dec 7, 2023

myhro commented Dec 7, 2023

grcevski commented Dec 7, 2023

myhro commented Dec 7, 2023

myhro commented Dec 7, 2023

myhro commented Dec 11, 2023

mariomac commented Dec 11, 2023

codecov-commenter commented Dec 5, 2023 •

edited

Loading