Skip to content

Comments

Make beyla-genfiles self-contained#1767

Merged
rafaelroquetto merged 6 commits intomainfrom
beyla_gen_tool
Apr 1, 2025
Merged

Make beyla-genfiles self-contained#1767
rafaelroquetto merged 6 commits intomainfrom
beyla_gen_tool

Conversation

@rafaelroquetto
Copy link
Contributor

@rafaelroquetto rafaelroquetto commented Mar 26, 2025

This PR embeds the beyla-genfile binary in the generator image, to allow downstream software vendoring Beyla to run this tool without having to explicitly fetch its source code and provide it to the image (that only works when building beyla itself, as tools are not distributed as vendored modules).

This relates to grafana/alloy#2734

@rafaelroquetto rafaelroquetto requested a review from a team as a code owner March 26, 2025 04:59
export BPF_CFLAGS="-O2 -g -Wall -Werror"
export BEYLA_GENFILES_RUN_LOCALLY=1
go run cmd/beyla-genfiles/beyla_genfiles.go
beyla_genfiles
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous approach of calling go run will not work on downstream projects pulling Beyla

@codecov
Copy link

codecov bot commented Mar 26, 2025

Codecov Report

Attention: Patch coverage is 0% with 18 lines in your changes missing coverage. Please review.

Project coverage is 66.98%. Comparing base (49c45f6) to head (62c78aa).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
cmd/beyla-genfiles/beyla_genfiles.go 0.00% 18 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1767      +/-   ##
==========================================
- Coverage   67.01%   66.98%   -0.04%     
==========================================
  Files         219      219              
  Lines       22616    22632      +16     
==========================================
+ Hits        15157    15159       +2     
- Misses       6691     6705      +14     
  Partials      768      768              
Flag Coverage Δ
integration-test 54.82% <ø> (+0.30%) ⬆️
k8s-integration-test 53.02% <ø> (+0.10%) ⬆️
oats-test 34.33% <ø> (+0.05%) ⬆️
unittests 43.22% <0.00%> (-0.04%) ⬇️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rafaelroquetto rafaelroquetto force-pushed the beyla_gen_tool branch 2 times, most recently from 0f10751 to caf9792 Compare March 26, 2025 06:11
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.

I don't get why we need removing the env package (increasing our code surface instead of relying on a stable library that is used in other parts of our code).

If you want to build beyla_genfiles, you can just do, from the project root:

go build -o beyla_genfiles cmd/beyla-genfiles/beyla_genfiles.go

Which already contains the go.mod and vendored dependencies.

COPY cmd/beyla-genfiles/beyla_genfiles.go .
RUN go build -o /go/bin/beyla_genfiles beyla_genfiles.go
RUN go clean -modcache -cache
RUN rm beyla_genfiles.go
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably can just copy the whole project, build the project with:

go build -o beyla_genfiles cmd/beyla-genfiles/beyla_genfiles.go

Then, below, instead of FROM base AS builder, do something like:

FROM alpine
COPY --from=base /path/beyla_genfiles .

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually you should also move this to the FROM alpine section:

RUN apk add clang llvm19 curl
RUN apk cache purge

And copy the bpf2go command also here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe then is simpler to just keep it as you have it now 🤣

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehee nah, that (having an intermediate step and copying the resulting artefacts as you suggest) makes sense indeed. I will look into it.

So, with regards to the dependency on the env package, it was mostly because Alloy does:

go run github.com/grafana/beyla/v2/cmd/beyla-genfiles@main

was giving me issues (the go tool was complaining it could not pull the env dependency). Let me run another tests with the dependency back in place and see how it goes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mariomac ok I've updated the PR, seems to work for Alloy - I've incorporated all of your suggestions (hopefully, let me know if I missed anything) and it looks better now IMHO.

@rafaelroquetto rafaelroquetto merged commit 6a6653a into main Apr 1, 2025
13 checks passed
@rafaelroquetto rafaelroquetto deleted the beyla_gen_tool branch April 1, 2025 16:52
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.

2 participants