Bpf logger

[vlrpl]

Add a dedicated trace pipe for better debugging eBPF progs and signaling unexpected errors.

[atenart]

I just skimmed through the code; a few suggestion though:

• This functionality looks nice for > debug messages. However including debug messages unconditionally can fill the buffer and hide higher messages. Debug messages are really only needed for development, what about hiding it under a compilation flag (-D) only set for the debug build?
• Following the above, debug messages could end up being quite verbose in some cases. It's also logically tricky to use the same application we're debugging for reporting debug messages. What about using bpf_printk for debug messages?
• It looks like all eBPF messages are reported to userspace whatever the actual log level is. I think you could benefit from the global eBPF configuration introduced in #337 to configure the probes log level. This could be done using the same logic as in #337: once probes are enabled, we can set a global variable and use it afterwards).

[vlrpl]

I just skimmed through the code; a few suggestion though:

* This functionality looks nice for `> debug` messages. However including debug messages unconditionally can fill the buffer and hide higher messages. Debug messages are really only needed for development, what about hiding it under a compilation flag (`-D`) only set for the debug build?

debug messages must not be included unconditionally. This is for error path or dev usage (see the comment before the log macros). We could strip them away from the compilation, or, considering this series is not in its final version (it lacks some usage and the "hiding" part) , the plan was to carry the loglevel down to the bpf progs and branch them behind a var in .rodata.

* Following the above, debug messages could end up being quite verbose in some cases. It's also logically tricky to use the same application we're debugging for reporting debug messages. What about using `bpf_printk` for debug messages?

not a fan of bpf_printk. It's mostly the same thing except it's a global trace pipe (also I'm not sure it's great from ux point of view).

* It looks like all eBPF messages are reported to userspace whatever the actual log level is. I think you could benefit from the global eBPF configuration introduced in [Wait for all probes to be installed before starting the collection #337](https://github.com/retis-org/retis/pull/337) to configure the probes log level. This could be done using the same logic as in [Wait for all probes to be installed before starting the collection #337](https://github.com/retis-org/retis/pull/337): once probes are enabled, we can set a global variable and use it afterwards).

this is similar to what I had in mind.

[atenart]

I just skimmed through the code; a few suggestion though:

* This functionality looks nice for `> debug` messages. However including debug messages unconditionally can fill the buffer and hide higher messages. Debug messages are really only needed for development, what about hiding it under a compilation flag (`-D`) only set for the debug build?

debug messages must not be included unconditionally. This is for error path or dev usage (see the comment before the log macros). We could strip them away from the compilation, or, considering this series is not in its final version (it lacks some usage and the "hiding" part) , the plan was to carry the loglevel down to the bpf progs and branch them behind a var in .rodata.

Makes sense. One way could be to set the rodata option before loading the BPF programs, this way debug messages are always compiled and available but are stripped when loading the program if disabled.

* Following the above, debug messages could end up being quite verbose in some cases. It's also logically tricky to use the same application we're debugging for reporting debug messages. What about using `bpf_printk` for debug messages?

not a fan of bpf_printk. It's mostly the same thing except it's a global trace pipe (also I'm not sure it's great from ux point of view).

I agree, handling debug messages in the same way we would handle other levels is good for UX and to keep things aligned. But that also makes the debug messages to rely on at least some parts of Retis to work properly. It's not a blocker, but wanted to have this discussion because it's a tradeoff to be aware of.

Maybe one way to still be able to debug Retis from an outside point of view would be to design this so that adding the following works:

#define debug bpf_printk

[vlrpl]

I just skimmed through the code; a few suggestion though:

* This functionality looks nice for `> debug` messages. However including debug messages unconditionally can fill the buffer and hide higher messages. Debug messages are really only needed for development, what about hiding it under a compilation flag (`-D`) only set for the debug build?

debug messages must not be included unconditionally. This is for error path or dev usage (see the comment before the log macros). We could strip them away from the compilation, or, considering this series is not in its final version (it lacks some usage and the "hiding" part) , the plan was to carry the loglevel down to the bpf progs and branch them behind a var in .rodata.

Makes sense. One way could be to set the rodata option before loading the BPF programs, this way debug messages are always compiled and available but are stripped when loading the program if disabled.

yes, that's what I was thinking about. I agree with all the points in the feedback and the idea was mostly to converge in that direction. This is useful especially since I wanted to discuss the .rodata (of course, we have some of those already) approach as it kind of overlaps with other global configs (requiring a lookup).

* Following the above, debug messages could end up being quite verbose in some cases. It's also logically tricky to use the same application we're debugging for reporting debug messages. What about using `bpf_printk` for debug messages?

not a fan of bpf_printk. It's mostly the same thing except it's a global trace pipe (also I'm not sure it's great from ux point of view).

I agree, handling debug messages in the same way we would handle other levels is good for UX and to keep things aligned. But that also makes the debug messages to rely on at least some parts of Retis to work properly. It's not a blocker, but wanted to have this discussion because it's a tradeoff to be aware of.

makes sense

Maybe one way to still be able to debug Retis from an outside point of view would be to design this so that adding the following works:

#define debug bpf_printk

I guess it would work, or at least can be done

[vlrpl]

there still are some open questions (minor things). Sent a new version in the meantime.

[vlrpl]

should be ready now