bpftrace icon indicating copy to clipboard operation
bpftrace copied to clipboard

Don't print out maps on exit if END probe is defined

Open ajor opened this issue 9 months ago • 11 comments

If the END probe is defined, don't print out maps automatically when bpftrace exits.

This will change the behaviour of some existing scripts, so could do with a discussion.

I often see the END probe being used solely to clear() all maps so that they're not printed by bpftrace, e.g.: https://github.com/bpftrace/bpftrace/blob/c8fd334f55e6b04a7eb3fc9a06822fc6b2a6344b/tools/biosnoop.bt#L48-L54

It's tedious to write this in most scripts and keep the list of maps to be cleared up-to-date as the script evolves.

Analysing the current state of the bpftrace tools, 18 scripts use END to clear maps and 18 rely on the implicit printing without specifying an END probe. 3 scripts define and END probe and also rely on the implicit printing. They all take the form: https://github.com/bpftrace/bpftrace/blob/c8fd334f55e6b04a7eb3fc9a06822fc6b2a6344b/tools/bitesize.bt#L26-L29 But I think this would be better if they explicitly printed out the relevant map anyway, instead of relying on the implicit print-on-exit behaviour.

ajor avatar May 01 '24 15:05 ajor

Agreed that current behavior is kinda annoying.

But could be risky. There might be automated users (eg https://github.com/iovisor/kubectl-trace and others) that rely on the exit printing. If we change the behavior they could lose stats.

danobi avatar May 01 '24 21:05 danobi

Actually, current behavior is beneficial for one liners. Which is probably why it’s like this in the first place

danobi avatar May 02 '24 03:05 danobi

I agree with @danobi here. The "feature" has been around for a long time and many automated users of bpftrace may rely on it.

What about we introduced a config option (and possibly a CLI option) that would auto-clear all the maps prior to exiting?

viktormalik avatar May 02 '24 08:05 viktormalik

One liners wouldn't typically define an END probe though - in that case I'm saying we should keep the current behaviour and print out all maps. As for why the current behaviour is like this: printing maps on exit was the only way to see the data in maps for a long time. END probes came later, and then print() later still.

For these automated users, do we know if they are they defining their own end probes or not?

I've just gone through and analysed Meta's collection of bpftrace scripts and the proportions are similar to the bpftrace tools in this repo:

  • END probe defined to clear all maps: 23
  • END probe defined, but rely on implicit map printing: 4
  • END probe not defined, rely on implicit map printing: 29

So ~7% of scripts will break with this change. Of those, I think a lot could be improved by explicitly printing the maps they care about instead of the inverse (deleting those they don't want to see). Instead of this:

kprobe:xxx
{
  @a = 1;
  @b = 2;
  @c = 3;
}
END
{
  clear(@b);
  clear(@c);
  // rely on implicit printing of @a
}

It'd be both clearer and shorter as:

kprobe:xxx
{
  @a = 1;
  @b = 2;
  @c = 3;
}
END
{
  print(@a);
}

This behaviour could be toggled with a config option, but I think it should be made the default at some point as based on the scripts I've seen, it'd help with a lot more than it'd harm. Automated users (like kubectl-trace) could still default to the legacy behaviour if they want.

ajor avatar May 02 '24 09:05 ajor

I agree that auto printing of maps at the end is blah AND that maybe too many people might rely on this default behavior to change it (at least without a lot of notice).

What about instead of implicit map clearing with END, which feels a little magical, introducing a single new function clear_all_maps.

jordalgo avatar May 02 '24 10:05 jordalgo

I think we could even do both clear_all_maps and a config option/command line switch. The config option way seems more natural to me, since it is not an actual bpftrace statement which prints the maps, but bpftrace itself, thus, it feels like it should be controlled by bpftrace. Clearing all maps so they are not displayed feels more like a hack.

However, having a clear_all_maps function can also be useful for other things, perhaps together with a print_all_maps function. Iteration over all maps is also technically possible, but the current conception of types in bpftrace does not allow it and changing would add a lot of complexity into the language, so having dedicated helpers for common operations seems as the way to go.

lenticularis39 avatar May 05 '24 13:05 lenticularis39

I like the command line switch option idea (or config/pragma). I think it should be separate from -q as sometimes you may want to manipulate those independently.

Map iteration would help. Right now I assemble bpftrace with m4 and use m4 macro expansion to add maps to the print or clear lists, but it would be much cleaner to be able to iterate - especially over a wildcarded subset of maps.

janca-ucdavis avatar May 30 '24 21:05 janca-ucdavis

Map iteration would help. Right now I assemble bpftrace with m4 and use m4 macro expansion to add maps to the print or clear lists, but it would be much cleaner to be able to iterate - especially over a wildcarded subset of maps.

@janca-ucdavis not sure if we're talking about the same thing but map iteration is a thing since #3003.

viktormalik avatar Jun 03 '24 06:06 viktormalik

@janca-ucdavis not sure if we're talking about the same thing but map iteration is a thing since https://github.com/bpftrace/bpftrace/pull/3003.

Hi, I'm talking about iterating over multiple maps, not within a map - I think that's a different case.

janet-campbell avatar Jun 03 '24 16:06 janet-campbell

@janca-ucdavis not sure if we're talking about the same thing but map iteration is a thing since https://github.com/bpftrace/bpftrace/pull/3003.

Hi, I'm talking about iterating over multiple maps, not within a map - I think that's a different case.

Ah, I see. That will probably be possible once nested maps come into play. AFAIK, there're still problems with their support in the kernel, though.

viktormalik avatar Jun 04 '24 07:06 viktormalik

Ah, I see. That will probably be possible once nested maps come into play. AFAIK, there're still problems with their support in the kernel, though.

Off topic, but after speaking with people from the kernel BPF team last week it sounds very unlikely that BPF maps-in-maps will ever provide the functionality that bpftrace needs, i.e. create maps from within probe's context and ideally multi-level nesting.

Hi, I'm talking about iterating over multiple maps, not within a map - I think that's a different case.

This sounds like it's potentially getting into the realms of metaprogramming or reflection. It's not something that we're planning to work on, but feel free to start the conversation with a GitHub issue/discussion if you have ideas.

ajor avatar Jun 05 '24 17:06 ajor