This feature is a good example of something where #3521 would be useful if we had it - it's a good feature to add but I don't think we're settled on a probe name yet.

To throw out another idea for probe naming that combines two cases we'd eventually want to support:

trigger:signal:SIGUSR1 {}
trigger:keypress:space {}

To throw out another idea for probe naming that combines two cases we'd eventually want to support:

trigger:signal:SIGUSR1 {}
trigger:keypress:space {}

looks more clear. if you all agree, we can use this naming.

or maybe using event instead of trigger?

event:signal:SIGUSR1 {}

What is the purpose of using the trigger: or event: prefix? To logically group probe types which fire when the bpftrace process is a target of an event (signal, keypress)? Are we expecting more such events in future?

What is the purpose of using the trigger: or event: prefix? To logically group probe types which fire when the bpftrace process is a target of an event (signal, keypress)? Are we expecting more such events in future?

The logical grouping was the reason in my mind. I can't think of any classes of event like this right now other than signals and keypresses.

I guess another reason for grouping them behind a single probe type would be that it makes it clear that it's not tracing any signal or keypress systemwide - only ones sent to the bpftrace runtime. Another idea based on this:

bpftrace:signal:SIGUSR
bpftrace:keypress:space

I guess another reason for grouping them behind a single probe type would be that it makes it clear that it's not tracing any signal or keypress systemwide - only ones sent to the bpftrace runtime. Another idea based on this:

bpftrace:signal:SIGUSR
bpftrace:keypress:space

I second this reason and this syntax makes it pretty clear that these are internal events received by bpftrace itself. I like it probably the best out of the proposed options. And it nicely opens way for more internal (pseudo-)events in future. We could even move BEGIN and END to the same group (at least internally).

Nit: The commit message could use some more detail and perhaps an example.

I don't mean to hold up this PR but I got confused when I first saw this naming bpftrace:signal:SIGUSR. I read the conversation above and understand the reasoning but naming a probe after the name of the tool feels wrong. I'm still thinking about alternatives but what about something like self:signal:SIGUSR ?

Should this also deprecate the ability to print maps when issuing a SIGUSR to bpftrace? Seems strange to have both.
https://github.com/bpftrace/bpftrace/blob/master/man/adoc/bpftrace.adoc#map-printing

Should this also deprecate the ability to print maps when issuing a SIGUSR to bpftrace? Seems strange to have both. https://github.com/bpftrace/bpftrace/blob/master/man/adoc/bpftrace.adoc#map-printing

I think the PR removes the functionality but fails to mention it in the docs. This is probably a good candidate for the "Breaking" changelog section and a migration guide entry.

hmm. while defining a signal probe changes the default behavior, still it's not a breaking change for those who don’t use it. does it really need a migration guide entry?
@viktormalik @jordalgo

@viktormalik - Making sure you saw my comment above. What do you think about self:signal:SIGUSR instead of bpftrace:signal:SIGUSR?

Apologies in advance if this sends you on bit of a side quest, but BPF_PROG_TYPE_RAW_TRACEPOINT was added in 4.17 kernel: torvalds/linux@a0fe3e5

bpftrace's policy w.r.t. old kernels is we support up to the oldest LTS kernel still in service: https://www.kernel.org/category/releases.html

The oldest LTS kernel still in service is 4.19, so we are free to assume BPF_PROG_TYPE_RAW_TRACEPOINT is always available.

So let's delete all the trigger probe stuff. It was always a bit of a hack. It should greatly simplify some of the stuff you're doing in this PR. Also means we can delete the feature probe (has_raw_tp_special())

@danobi just to make sure i got your point correctly, now we don't need to attach the special probes to anyting? calling bpf_prog_test_run_opts would be enough, right?

Also cc @andreasgerstmayr who added the original "print maps on SIGUSR1" feature.

Thanks for the ping! I agree it's a breaking change, but it'll be simple to adjust existing programs.
Nice feature! 💯

@danobi just to make sure i got your point correctly, now we don't need to attach the special probes to anyting? calling bpf_prog_test_run_opts would be enough, right?

Yes, that's right. And sorry about late reply - must've missed notification

@viktormalik - Making sure you saw my comment above. What do you think about self:signal:SIGUSR instead of bpftrace:signal:SIGUSR?

Hmm, I bpftrace prefix is ok. Maybe I write too much c++/rust. In both languages you end up having a top level namespace w/ same name as project. self is probably ok too.

Maybe I write too much c++/rust. In both languages you end up having a top level namespace w/ same name as project.

But do you find yourself writing the words "c++" or "rust" in these projects? Maybe it's just a gut reaction but I feel weird reading/writing "bpftrace" as a keyword in the tool/language itself. "self" is also shorter.

And if we decide to someday add support for embedding bpftrace scripts in other languages then the process running these scripts will not be "bpftrace" but some other process. "self" would still apply in this case.

:P sure. Sounds like self is shorter and less ambiguous for future use cases.

<joke>or we could do "this"</joke>

thanks folks. I’d like to know @ajor and @viktormalik opinion too before doing any modifications.

Yeah, self sounds good and probably expresses the purpose better (the "target" is the current bpftrace process). No strong opinion on my side.

This does make me crave the ability for bpftrace to be more interactive and be able to handle custom commands from stdin e.g. "print_maps", "lost_event_count", "print_map_value(@A, 1)", etc.

if I’m not mistaken, we can also receive data via signals. while this approach isn’t as convenient as sending data through stdin, it can still be implemented relatively easily in the future

@viktormalik @danobi - One last look before I merge?

Did a quick skim - didn't spot any major issues