Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add minimum Go version detection #458

Closed

Conversation

Dean-Coakley
Copy link

Fixes: #436

TODO:

  • Test

* If minimum go version is not detected. Exit with error
@CLAassistant
Copy link

CLAassistant commented Nov 22, 2023

CLA assistant check
All committers have signed the CLA.

@grcevski
Copy link
Contributor

grcevski commented Nov 22, 2023

Hi @Dean-Coakley, thanks for working on this!

I apologize, I should've been more specific. We want to detect that the instrumentable is not of an older Go version. Beyla won't compile without 1.20 I think, so I think we are safe if Beyla's go version is older, the dependencies we use won't allow it to compile.

To check the version of the program we are instrumenting we just need a check here:

https://github.com/grafana/beyla/blob/760d3ad8b21dc880606b0d5d0d60f93c1ed02ba7/pkg/internal/goexec/gofile.go#L15C9-L15C9

	goVersion, modules, err := getGoDetails(elfFile)
	if err != nil {
		return nil, fmt.Errorf("getting Go details: %w", err)
	}

	goVersion = strings.ReplaceAll(goVersion, "go", "")
	log().Debug("Go version detected", "version", goVersion)

At the last line above, we log the Go version we've discovered of the instrumentable. All it's needed is to check if what we found there is older than 1.17 and return the error just like above, where we can't find any go details. This should trickle back all the way and avoid instrumenting the application as a go program. We should also log a debug message saying we found an old version and we won't be instrumenting it using the go uprobes.

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

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. We may need to refactor things a bit in findLibraryVersions() to ensure we have proper code reuse.

On the tests topic, I think it will be sufficient to add a unit test for the version check, essentially test that we can recognize version older than 1.17, and newer or equal, with the goVersion variable above.

We can also add an integration test with a docker compose of an old Go application, but I can help there once we get to that point.

Again we really appreciate you picking this up, let me know if you'd like to continue with the suggestion I made above.

Cheers.

@Dean-Coakley
Copy link
Author

Hi @grcevski

Apologies for misinterpreting the issue.

Still interested in giving this a shot. However it is a lot more work than I initially expected, may take a few weeks for me to get a chance to take a look at this. 😅

@grcevski
Copy link
Contributor

Hi @Dean-Coakley,

Still interested in giving this a shot. However it is a lot more work than I initially expected, may take a few weeks for me to get a chance to take a look at this. 😅

100%, take your time!

@myhro myhro mentioned this pull request Dec 5, 2023
@Dean-Coakley
Copy link
Author

Seems this is going to be handled by 485.

Closing

@Dean-Coakley Dean-Coakley deleted the add-go-version-detection branch December 8, 2023 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Go version detection
3 participants