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.

Sign up for GitHub

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

Jump to bottom

Include generated object files in commit history and published packages #1233

Open
MrAlias opened this issue Oct 30, 2024 · 25 comments
Open

Include generated object files in commit history and published packages #1233

MrAlias opened this issue Oct 30, 2024 · 25 comments

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Oct 30, 2024 •
edited
Loading

This issue has been discussed before, I can't seem to find where though. The goal would be to have the released go.opentelemetry.io/auto package be a self-contained distribution.

Currently, when a project takes a dependency on go.opentelemetry.io/auto the project will not compile:

package main

import "go.opentelemetry.io/auto"

func main() {
	i := auto.NewInstrumentation()
	_ = i.Close()
}
$ go version
go version go1.23.2 linux/amd64

$ cat go.mod
module testing/autoimprt

go 1.23.2

require go.opentelemetry.io/auto v0.16.0-alpha

require (
	github.com/beorn7/perks v1.0.1 // indirect
	github.com/cenkalti/backoff/v4 v4.3.0 // indirect
	github.com/cespare/xxhash/v2 v2.3.0 // indirect
	github.com/cilium/ebpf v0.16.0 // indirect
	github.com/go-logr/logr v1.4.2 // indirect
	github.com/go-logr/stdr v1.2.2 // indirect
	github.com/google/uuid v1.6.0 // indirect
	github.com/grpc-ecosystem/grpc-gateway/v2 v2.22.0 // indirect
	github.com/hashicorp/go-version v1.7.0 // indirect
	github.com/klauspost/compress v1.17.11 // indirect
	github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
	github.com/pkg/errors v0.9.1 // indirect
	github.com/prometheus/client_golang v1.20.4 // indirect
	github.com/prometheus/client_model v0.6.1 // indirect
	github.com/prometheus/common v0.60.0 // indirect
	github.com/prometheus/procfs v0.15.1 // indirect
	go.opentelemetry.io/auto/sdk v0.1.0-alpha // indirect
	go.opentelemetry.io/contrib/bridges/prometheus v0.56.0 // indirect
	go.opentelemetry.io/contrib/exporters/autoexport v0.56.0 // indirect
	go.opentelemetry.io/otel v1.31.0 // indirect
	go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc v0.7.0 // indirect
	go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp v0.7.0 // indirect
	go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc v1.31.0 // indirect
	go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp v1.31.0 // indirect
	go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.31.0 // indirect
	go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.31.0 // indirect
	go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.31.0 // indirect
	go.opentelemetry.io/otel/exporters/prometheus v0.53.0 // indirect
	go.opentelemetry.io/otel/exporters/stdout/stdoutlog v0.7.0 // indirect
	go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v1.31.0 // indirect
	go.opentelemetry.io/otel/exporters/stdout/stdouttrace v1.31.0 // indirect
	go.opentelemetry.io/otel/log v0.7.0 // indirect
	go.opentelemetry.io/otel/metric v1.31.0 // indirect
	go.opentelemetry.io/otel/sdk v1.31.0 // indirect
	go.opentelemetry.io/otel/sdk/log v0.7.0 // indirect
	go.opentelemetry.io/otel/sdk/metric v1.31.0 // indirect
	go.opentelemetry.io/otel/trace v1.31.0 // indirect
	go.opentelemetry.io/proto/otlp v1.3.1 // indirect
	golang.org/x/arch v0.11.0 // indirect
	golang.org/x/exp v0.0.0-20230224173230-c95f2b4c22f2 // indirect
	golang.org/x/net v0.30.0 // indirect
	golang.org/x/sys v0.26.0 // indirect
	golang.org/x/text v0.19.0 // indirect
	google.golang.org/genproto/googleapis/api v0.0.0-20241007155032-5fefd90f89a9 // indirect
	google.golang.org/genproto/googleapis/rpc v0.0.0-20241007155032-5fefd90f89a9 // indirect
	google.golang.org/grpc v1.67.1 // indirect
	google.golang.org/protobuf v1.35.1 // indirect
)

$ go build
../../../pkg/mod/go.opentelemetry.io/[email protected]/internal/pkg/instrumentation/bpf/database/sql/bpf_x86_bpfel.go:165:12: pattern bpf_x86_bpfel.o: no matching files found
../../../pkg/mod/go.opentelemetry.io/[email protected]/internal/pkg/instrumentation/bpf/github.com/segmentio/kafka-go/consumer/bpf_x86_bpfel.go:172:12: pattern bpf_x86_bpfel.o: no matching files found
../../../pkg/mod/go.opentelemetry.io/[email protected]/internal/pkg/instrumentation/bpf/github.com/segmentio/kafka-go/producer/bpf_x86_bpfel.go:167:12: pattern bpf_x86_bpfel.o: no matching files found
../../../pkg/mod/go.opentelemetry.io/[email protected]/internal/pkg/instrumentation/bpf/go.opentelemetry.io/auto/sdk/bpf_x86_bpfel.go:159:12: pattern bpf_x86_bpfel.o: no matching files found
../../../pkg/mod/go.opentelemetry.io/[email protected]/internal/pkg/instrumentation/bpf/go.opentelemetry.io/otel/traceglobal/bpf_x86_bpfel.go:213:12: pattern bpf_x86_bpfel.o: no matching files found
../../../pkg/mod/go.opentelemetry.io/[email protected]/internal/pkg/instrumentation/bpf/google.golang.org/grpc/client/bpf_x86_bpfel.go:170:12: pattern bpf_x86_bpfel.o: no matching files found
../../../pkg/mod/go.opentelemetry.io/[email protected]/internal/pkg/instrumentation/bpf/google.golang.org/grpc/server/bpf_x86_bpfel.go:169:12: pattern bpf_x86_bpfel.o: no matching files found
../../../pkg/mod/go.opentelemetry.io/[email protected]/internal/pkg/instrumentation/bpf/net/http/client/bpf_no_tp_x86_bpfel.go:182:12: pattern bpf_no_tp_x86_bpfel.o: no matching files found
../../../pkg/mod/go.opentelemetry.io/[email protected]/internal/pkg/instrumentation/bpf/net/http/server/bpf_x86_bpfel.go:174:12: pattern bpf_x86_bpfel.o: no matching files found

We should ship a Go module/package that is self-contained. It should not require any user to generate code within their package cache.

cc @damemi @RonFed

Proposals

Check in object files to git

  • Pros:
    • Code, in development and in release, should always be in a buildable and importable state.
    • Doesn't require any change to release process
  • Cons:
    • Bloats the git history size
    • Adds non-human readable data to git history
    • Adds non-reviewable code to PRs (reliance on CI to check)

Run and manage our own Go module proxy

#1233 (comment)

  • Pros:
    • Release code will be importable and directly usable
  • Cons:
    • Requires significant additional release processes
    • Requires OTel to manage a Go proxy server
    • Adds complication to our vanity URL service (may require development work there)
    • Adds security attack vectors

Commit the object files into release branches

#1233 (comment)

  • Pros:
    • Released code should be importable and directly usable (verification needed)
  • Cons:
    • Might bloat git history size if branches need to remain in git history
    • Requires additional release processes
    • Adds non-reviewable code to release PRs (reliance on CI to check)

Post-process Go code to contain the object file

#1233 (comment)

  • Pros:
    • Released code will be importable and directly usable
    • Does not require additional release processes
  • Cons:
    • Extra tooling needed to manage post-processing
    • Is still adding non-human readable code to repository, it is just hiding it is a human readable file
    • Adds non-reviewable code to release PRs (reliance on CI to check)

Document dependency generation steps

#1233 (comment)

  • Pros:
    • Minimal changes to what we do now
  • Cons:
    • Inconvenient and unexpected to downstream consumers

Use Git LFS

#1233 (comment)

  • Pros:
    • Solves problems with repo bloat
    • less weird than running make generate commands to build object files
  • Cons:
    • Not standard git usage (devs need to install git lfs)

Update Cilium libraries to support runtime object storage

#1233 (comment)

  • Pros: TODO
  • Cons: TODO
@MrAlias MrAlias added this to the v0.17.0-alpha milestone Oct 30, 2024
@damemi
Copy link
Contributor

damemi commented Oct 30, 2024 via email

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 30, 2024

The thing is, you cannot execute a make task when someone downloads the package. What we ship now is a broken package.

@damemi
Copy link
Contributor

damemi commented Oct 30, 2024 via email

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 30, 2024

cilium/ebpf#988

This is talking about running a make target within the project generating the eBPF code. It is not talking about providing a release target to others downloading the Go module. That approach is going to be flawed as it will require someone who runs go get or go mod tidy in their project to then go find our distributed Makefile and run another command. This is going to be especially problematic if the auto package is not even a direct dependency.

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 30, 2024

The code that we ship already directly depends on the files we do not release:

opentelemetry-go-instrumentation/internal/pkg/instrumentation/bpf/database/sql/bpf_x86_bpfel.go

Lines 163 to 166 in 77e8de9

// Do not access this directly.
//
//go:embed bpf_x86_bpfel.o
var _BpfBytes []byte

This seems like something that should be in source code if we are already referencing it.

@damemi
Copy link
Contributor

damemi commented Oct 30, 2024 via email

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 30, 2024

Looking at the remove import paths documentation, we need to use some VCS as the protocol to download source code (i.e. commit all the code), or run our own module proxy. Looking at the documentation for module proxies, it seems like we could respond to $base/$module/@v/$version.zip requests with a zip file including all the generated code.

This module proxy will need to be added as a managed service run by OTel and require additional step in our release process for uploading the release zip files.

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 30, 2024

FWIW, here's an example of another eBPF project checking in their object files: https://github.com/k8spacket/k8spacket/blob/65e5268975cc41a4a1b5daed8a534666a8a4fab5/ebpf/tc/tc_bpfel.o

@RonFed
Copy link
Contributor

RonFed commented Oct 31, 2024

I'm very supportive of improving this part - I agree with @damemi - committing these files to git should only be done if we can't find another reasonable approach.
In Odigos, we call make generate.
From my experience, besides this being not pretty it also makes development harder since each import of our library results in a build error - which makes testing harder.

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 31, 2024

Another idea: we could make our releases in release branches. Those branches would contained checked-in *.o files. If I understand the GitHub Go releasing model correctly (a big if), we might be able to delete the branch after the release is tagged and published.

I'm not sure if this is better or just the same with regard to binary data being checked in to git.

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 31, 2024

We could also post-curate the generate steps with a file manipulator that directly defines the object file binary data in the embedded type. This would be equivalent to what protobuf does. For example: https://github.com/open-telemetry/opentelemetry-proto-go/blob/53975ec09b7a48899d5153f09425f326e642a2dc/otlp/trace/v1/trace.pb.go#L963-L1119

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 31, 2024

@damemi @RonFed: please update the description with any additional proposals or pros/cons you can think of.

@damemi
Copy link
Contributor

damemi commented Nov 1, 2024

@MrAlias the post-curation idea is really interesting. That seems like the safest compromise and the pattern already exists for protobuf. I think that's my favorite option, and we could even separate the generated files into their own sub packages if we wanted.

Just thinking, you could make a philosophical argument here that while this is broken in the traditional Go sense, it's also not a very traditional Go library. I would absolutely expect almost any other Go package to not behave this way. But anyone who's using this, even transitively, should be aware that it depends on compiled eBPF objects... and so I wonder if the extra build/generate steps (like the make command @RonFed shared) could be an acceptable pattern? At least until some better guidance or alternative emerges?

To formalize that option, I'd propose our docs include a section on "using this library" that clearly includes exactly what commands the user needs as a setup step after they run go get (ideally a copy/paste one liner).

@RonFed
Copy link
Contributor

RonFed commented Nov 1, 2024

Post-process Go code to contain the object file

This object file representation will only be updated on releases or every time we change the c files?
I did not fully understand the advantages relative to just using the object files - is this not the same thing written differently? (i.e will have the same effect on git?)

Run and manage our own Go module proxy

With this approach, if a user go get our module, the proxy server will provide the .o files as if there were in git?
If so, that sounds like a complete solution to me (which will probably require a lot of work though).

A common question to most of the propsed soultions is whether we want to support imports of non-official releases (with commit hashes)? I think some of the options rely on just updating the compiled code in each official release.

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 1, 2024

I did not fully understand the advantages relative to just using the object files - is this not the same thing written differently? (i.e will have the same effect on git?)

I don't thing there objectively is a difference. The difference is stylistic. There is precedence with protobuf generating binary data directly held in .go files that is checked into repositories.

If others are okay with this approach, I'm okay as well. It is not my first choice though.

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 1, 2024

With this approach, if a user go get our module, the proxy server will provide the .o files as if there were in git?
If so, that sounds like a complete solution to me (which will probably require a lot of work though).

Correct. I haven't built out a prototype yet, but all the documentation I have read indicates this will be the behavior. We will upload snapshots of our code (maybe already zipped(?)) to some module proxy server. Those snapshots will be served and they will contain all the *.o files. The git history would remain as it currently is in this approach.

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 1, 2024

A common question to most of the propsed soultions is whether we want to support imports of non-official releases (with commit hashes)? I think some of the options rely on just updating the compiled code in each official release.

That's a good question. We would have to investigate this further to see what kind of support we could provide.

I know we can provide the standard Go proxy as a fallback to our proxy and it may all users to download directly from git history. The issue would be that those downloads would not have the desired object files.

I expect there is some more engineering we could do to try and solve the issue though.

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 1, 2024

Go mod proxies we could run BTW:

@damemi
Copy link
Contributor

damemi commented Nov 1, 2024 •
edited
Loading

The problem I see with the Go proxy solution is it doesn't support custom Probes (outside our repo), and makes it tougher for us to do a contrib-style repo. I guess third party probes can choose to provide their object files however they'd like, but a consistent approach would be nice

@damemi
Copy link
Contributor

damemi commented Nov 1, 2024

Another suggestion from ebpf slack was to use Git LFS, which from a quick search looks like it should be compatible with Go modules. I don't know if that would help with the code reviews and diffs, but it would solve the repo bloat.

I also like the release branch idea, and I think we should start using release branches anyway for backports once our release model is more stable.

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 2, 2024

It looks like using Git LFS will mean all modules that take this one as a dependency will need to have it installed and configured in their development environments: https://stackoverflow.com/questions/58280942/import-a-golang-module-with-lfs-objects

@damemi
Copy link
Contributor

damemi commented Nov 2, 2024

It looks like using Git LFS will mean all modules that take this one as a dependency will need to have it installed and configured in their development environments: https://stackoverflow.com/questions/58280942/import-a-golang-module-with-lfs-objects

Yeah, that is the downside. But I think it's still an improvement over the weird go cache generate commands you need otherwise since "all" you have to do is install git lfs.

By the way, when we finish the custom probes API #1105 this should contain the blast radius of this issue to just the probe modules themselves, since at that point the main auto-instrumentation framework won't have an explicit dependency on the probes. The problem still exists, but I think that in conjunction with one of the other solutions is going to be enough

@damemi
Copy link
Contributor

damemi commented Nov 3, 2024

I also had an idea for a proposal we could make to the Cilium libraries to support runtime object storage:

The compiled object bytes aren’t actually a build dependency, they’re only needed at runtime when the Cilium libraries load them into the kernel. The file just needs to be there for the Go compiler to build.

So if we built a similar approach to Git LFS and had bpf2go embed a file that just held a pointer to the actual object, then there wouldn’t be any compile errors and you could support any kind of runtime object storage: local, remote, lfs, etc… just have Cilium read the bytes into memory at runtime. The pointer file format would just have to declare the protocol and location and use that if an actual local file isn’t available.

There’s details to work out in the UX, like for example when developing you would want to be able to build a local file that takes precedence over whatever the pointer file says.

The downside is that you would need a connection to wherever the remote file is. We could solve this by supporting a persistent local cache (ie, download it to /tmp and read from there) or even a runtime flag that points to a local directory where the objects live (for example, each release we just publish the objects as a GitHub artifact, users download the ones they want for the probes they’re using, and save that in their host/image/etc). The tradeoff there is file I/O, but you could cache it in memory for efficiency if it's going to be loading/unloading often.

Wdyt? I'm pretty sure this is feasible but I have to look more into the details

@MrAlias MrAlias removed this from the v0.17.0-alpha milestone Nov 4, 2024
@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 5, 2024 •
edited
Loading

SIG notes:

  • Each *.o file is ~50KB. Every change would mean another 50KB to the git history.
  • Custom probes would help alleviate "a single version for everything" issue (Custom Probes #1105)
  • Maintainer of the Cillium project suggest to just commit binaries to history (via slack)
  • If we embed the *.o files into *.go files we would never be able to purge the data from git history.
  • We could have an automated fork of this repository. The fork would only be touched by bots and it would sync with this repositry and commit the *.o files for each commit upstream.
  • Takeaways:
    • Follow up with Cillium maintainers on slack.
    • We could split into short-term and long-term solutions
      • short-term:
        • Needs a way to revert what is done
        • Maybe release-brances(AI: @MrAlias )/commiting directly
      • long-term:
        • module proxy
        • updating cillium library (AI: @damemi )

@florianl florianl mentioned this issue Nov 6, 2024
Open
@damemi
Copy link
Contributor

damemi commented Nov 6, 2024

More updates from the slack thread:

  • We should make sure we're using llvm-strip to reduce the size of our .o files
  • Cilium already supports direct loading instead of embedding by calling ebpf.LoadCollectionSpec on the file and spec.LoadAndAssign on the go structs from bpf2go. If so, we could make this change today and immediately fix the IDE/compile time issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@damemi @MrAlias @RonFed