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

Treat go binaries without offsets as generic (1.9 branch patch) #1477

Merged
merged 3 commits into from
Dec 21, 2024

Conversation

rafaelroquetto
Copy link
Contributor

@rafaelroquetto rafaelroquetto commented Dec 20, 2024

In the case in which we have found offsets, but these are related to a go proxy, we must set the error so that the attacher will treat this as a regular binary.

There is a small corner case in which we detect no go offsets in a target executable, but if that executable happens to have a ".gosyms" section, we will end up classifying it as a Go executable, causing issues. We attach an error in this case to have the attacher treat it as a generic executable.

Also, on ELF parse error, we return the list of missing fields, assuming that we would stop the instrumentation. There is an edge case error on which the missing fields set is empty so Beyla keeps with the instrumentation, but the returned list of fields is nil.

Closes #1472

Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 81.02%. Comparing base (6d9088b) to head (2a29c19).
Report is 1 commits behind head on release-1.9.

Files with missing lines Patch % Lines
pkg/internal/discover/typer.go 72.72% 3 Missing ⚠️
pkg/internal/discover/attacher.go 33.33% 1 Missing and 1 partial ⚠️
pkg/internal/goexec/structmembers.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           release-1.9    #1477      +/-   ##
===============================================
- Coverage        81.15%   81.02%   -0.13%     
===============================================
  Files              146      146              
  Lines            14828    14839      +11     
===============================================
- Hits             12033    12024       -9     
- Misses            2206     2223      +17     
- Partials           589      592       +3     
Flag Coverage Δ
integration-test 58.32% <60.00%> (-0.01%) ⬇️
k8s-integration-test 60.01% <13.33%> (-0.10%) ⬇️
oats-test 33.85% <13.33%> (-0.03%) ⬇️
unittests 51.60% <0.00%> (-0.09%) ⬇️

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.

rafaelroquetto and others added 3 commits December 20, 2024 12:43
There is a small corner case in which we detect no go offsets in a
target executable, but if that executable happens to have a ".gosyms"
section, we will end up classifying it as a Go executable, causing
issues. We attach an error in this case to have the attacher treat it as
a generic executable.
On ELF parse error, we return the list of missing fields, assuming that
we would stop the instrumentation. There is an edge case error on which
the missing fields set is empty so Beyla keeps with the instrumentation,
but the returned list of fields is nil.
In the case in which we have found offsets, but these are related to a
go proxy, we must set the error so that the attacher will treat this as
a regular binary.
@rafaelroquetto rafaelroquetto force-pushed the fix_null_go_offsets-1.9 branch from 915de07 to 2a29c19 Compare December 20, 2024 18:43
Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@grcevski grcevski merged commit 62fb834 into release-1.9 Dec 21, 2024
13 checks passed
@grcevski grcevski deleted the fix_null_go_offsets-1.9 branch December 21, 2024 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants