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

Fix name generation for BPF metrics #1552

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

marctc
Copy link
Contributor

@marctc marctc commented Jan 21, 2025

  • Fixed how names are fetched, making possible to get the full name!
  • Bump cillium to 0.17
  • Simplified logic of metrics collection
  • Fetch only probes that are Kprobes (this includes all Beyla probes)
  • Fetch only maps that are LRUHash (this includes all Beyla maps)

@marctc marctc requested a review from a team as a code owner January 21, 2025 11:05
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 3.89610% with 74 lines in your changes missing coverage. Please review.

Project coverage is 34.66%. Comparing base (b535e56) to head (90c0387).

Files with missing lines Patch % Lines
pkg/export/prom/prom_bpf.go 0.00% 56 Missing ⚠️
pkg/internal/netolly/ebpf/sock_tracer.go 0.00% 8 Missing ⚠️
pkg/internal/netolly/ebpf/tracer.go 0.00% 7 Missing ⚠️
pkg/internal/ebpf/tracer_linux.go 50.00% 2 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (b535e56) and HEAD (90c0387). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (b535e56) HEAD (90c0387)
oats-test 2 1
integration-test 2 0
unittests 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1552       +/-   ##
===========================================
- Coverage   67.74%   34.66%   -33.08%     
===========================================
  Files         197      193        -4     
  Lines       19755    19598      -157     
===========================================
- Hits        13383     6794     -6589     
- Misses       5685    12214     +6529     
+ Partials      687      590       -97     
Flag Coverage Δ
integration-test ?
oats-test 34.66% <3.89%> (+0.01%) ⬆️
unittests ?

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.

Copy link
Contributor

@rafaelroquetto rafaelroquetto left a comment

Choose a reason for hiding this comment

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

LGTM! The way you encapsulated the name fetching is really elegant. I've added a few suggestions, which are mere ... suggestions (and very personal taste, so feel free to ignore).

pkg/export/prom/prom_bpf.go Outdated Show resolved Hide resolved
pkg/export/prom/prom_bpf.go Outdated Show resolved Hide resolved
pkg/export/prom/prom_bpf.go Outdated Show resolved Hide resolved
pkg/export/prom/prom_bpf.go Outdated Show resolved Hide resolved
marctc and others added 4 commits January 21, 2025 14:54
@rafaelroquetto
Copy link
Contributor

rafaelroquetto commented Jan 21, 2025

Looking closer at the ebpf code, it turns out this may require CAP_SYS_ADMIN, which could be problematic:

// FuncInfos returns the offset and function information of all (sub)programs in
// a BPF program.
//
// Available from 5.0.
//
// Requires CAP_SYS_ADMIN or equivalent for reading BTF information. Returns
// ErrNotSupported if the program was created without BTF or if the kernel
// doesn't support the field.
func (pi *ProgramInfo) FuncInfos() (btf.FuncOffsets, error) { ... }

I don't see a simple way out (without BTF and without renaming the probes) apart from generating a go map matching the 16 first chars as a key and the full probe name as a value. I hacked a quick script that does it just a a POC, so we could maybe do something similar in go and invoke with //go:generate inside prom_bpf.go.

Here's the script (place it inside the ebpf dir if you want to run it)
gen_probe_map.pl.gz

And the proposed (generated) output:
probe_map.go.txt

go.mod Outdated Show resolved Hide resolved
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.

3 participants