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

Refactor code to allow for other languages #117

Merged
merged 2 commits into from
May 22, 2023

Conversation

grcevski
Copy link
Contributor

This PR lays the ground work to allow us to add support for other languages than Go. I moved some code around and made not finding the Go runtime and offsets non-fatal.

There's no eBPF code yet that uses non-Golang support, but if I made all the changes at once it would be very hard to review :).

@grcevski grcevski added the enhancement New feature or request label May 17, 2023
@grcevski grcevski requested a review from mariomac as a code owner May 17, 2023 17:32
Copy link
Contributor

@mariomac mariomac left a comment

Choose a reason for hiding this comment

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

Amazing refactor! I have few comments about some details.

}

func attachSocketFilter(filter *ebpf.Program) (int, error) {
fd, err := unix.Socket(unix.AF_PACKET, unix.SOCK_RAW, int(htons(unix.ETH_P_ALL)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow. I cannot understand why the Go API doesn't handle this htons conversion internally.

Comment on lines 119 to 123
func htons(a uint16) uint16 {
var arr [2]byte
binary.LittleEndian.PutUint16(arr[:], a)
return binary.BigEndian.Uint16(arr[:])
}
Copy link
Contributor

@mariomac mariomac May 18, 2023

Choose a reason for hiding this comment

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

I think this will break current compatibility with ARM. Maybe you can either:

  1. Move this to respective instrumenter_arm64.go and instrumenter_amd64.go files, and write different implementations (the ARM would be just return the value).
  2. Copy this endian-independent implementation: https://github.com/songgao/ether/blob/master/afpacket.go#L14-L23

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch! I'm going to steal the implementation above, very cool trick.

Comment on lines 95 to 102
func (p *Tracer) KProbes() map[string]ebpfcommon.FunctionPrograms {
return map[string]ebpfcommon.FunctionPrograms{}
}

func (p *Tracer) SocketFilters() []*ebpf.Program {
return []*ebpf.Program{}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: you can safely return nil arrays and nil slices. Go accepts operations such as range and len over them: https://go.dev/play/p/0s6WRbXWCPK

In the case of slices, it even accepts append.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sweet! Thanks!

"github.com/grafana/ebpf-autoinstrument/pkg/goexec"

"github.com/grafana/ebpf-autoinstrument/pkg/ebpf/nethttp"
"github.com/mariomac/pipes/pkg/node"
"golang.org/x/exp/slog"
)

var log = slog.With("component", "ebpf.TracerProvider")
Copy link
Contributor

Choose a reason for hiding this comment

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

If you move this here, the logger will be created with the default level before you load the configuration and set the new log level. In this case, debug logs won't be visible.

To reuse the logger from different functions, in other files I did:

func logger() *slog.Logger {
   return slog.With("component", "ebpf.TracerProvider")
}

Then just taking care to invoke it only once at the beginning of critical functions, like the long-lived functions that iterate the channels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point, I'll revert that and make sure I can use the logger where I needed it.

@grcevski grcevski requested a review from mariomac May 18, 2023 17:05
@grcevski grcevski merged commit 2f8d365 into grafana:main May 22, 2023
3 checks passed
@grcevski
Copy link
Contributor Author

Thanks Mario!

@grcevski grcevski deleted the prep_for_no_go branch May 22, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants