grafana / beyla

eBPF-based autoinstrumentation of web applications and network metrics
https://grafana.com/oss/beyla-ebpf/
Apache License 2.0
1.23k stars 78 forks source link

Don't cleanup prematurely #619

Closed grcevski closed 4 months ago

grcevski commented 4 months ago

Fixing number of places where we used the unsafe delete after use pattern.

codecov-commenter commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (e263f8e) 79.41% compared to head (0e52151) 44.57%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #619 +/- ## =========================================== - Coverage 79.41% 44.57% -34.84% =========================================== Files 70 68 -2 Lines 5989 5817 -172 =========================================== - Hits 4756 2593 -2163 - Misses 1005 3071 +2066 + Partials 228 153 -75 ``` | [Flag](https://app.codecov.io/gh/grafana/beyla/pull/619/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=grafana) | Coverage Δ | | |---|---|---| | [integration-test](https://app.codecov.io/gh/grafana/beyla/pull/619/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=grafana) | `?` | | | [unittests](https://app.codecov.io/gh/grafana/beyla/pull/619/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=grafana) | `44.57% <ø> (-0.04%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=grafana#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

grcevski commented 4 months ago

I'm curious about the usage of goto instead of functions, is this a common pattern in eBPF programs or it's just to simplify things? Is this fixing #618?

Yeah, it's sort of common in eBPF. Each program is always individual single function and when we use functions we'd have to put __always_inline so that the verifier likes it. The only way to call in terms of a function call is to use bpf_tail_call.

So I think you are right, we could've made the cleanup a function and "call it", but then we'd have to make individual cleanup function for each probe, I thought this was simpler, but maybe I'm not considering some approach. If you have a cleaner code in mind, please shout :).

grcevski commented 4 months ago

Is this fixing #618?

I hope it will. I'll ask on the issue if they can try with our main branch now once this is built.

myhro commented 4 months ago

Hi Nikola,

(...) when we use functions we'd have to put __always_inline so that the verifier likes it.

Is this a limitation of the way bpf2go works? I'm not sure if/how it uses libbpf under the hood, but __always_inline should only be required on older kernels (< 4.16/< 5.5). For the ones Beyla is targeting (>= 5.8), function calls should work without inlining.

grcevski commented 4 months ago

Oh maybe it's not needed anymore, I just keep using it by habbit.