search

twitter / rezolus

Systems performance telemetry
Apache License 2.0
1.56k stars 116 forks source link

Do not attach a probe if none of the statistics requiring it is enabled. #254

Closed WUMUXIAN closed 2 years ago

WUMUXIAN commented 2 years ago

Problem

Currently disabling telemetries that are backed by BPF does not affect the logic of attaching probes. e.g. if BPF = true but telemetries that actually use BPF are not enabled, the kernel probes are still attached, which introduces unnecessary overhead.

We should only attach a probe when it's required by >= 1 statistic.

Solution

This PR is used to introduce the logic to determine whether or not a probe should be attached at bpf initialisation for samplers who has BPF-based telemetries.

Result

All existing samplers are now handled. In the future, implementing any BPF based telemetries needs to follow the mechanism

brayniac commented 2 years ago

It looks like the approach you took involves declaring that a given probe is used for a set of statistics? Curious why you appear to have gone that way as opposed to saying that this statistic requires this set of probes to be attached.

WUMUXIAN commented 2 years ago

It looks like the approach you took involves declaring that a given probe is used for a set of statistics? Curious why you appear to have gone that way as opposed to saying that this statistic requires this set of probes to be attached.

That's a really good question. I went with this direction because I wanted to have a central place where I can decide whether or not probe needs to be attached, which happens in 'src/common/bpf.rs' in 'try_attach_to_bpf', for the implementor, he just needs to specify probes with telemetries that are dependent on it. All logic of deciding whether to attach a probe is shared and already taken care of. I didn't really consider the alternative approach.

Now taking a closer look, I think the current approach might be too probe-centric, when it comes to introduce a new telemetry, and let's say this new telemetry requires existing probe A, B and C and non-exist probe D, then we need to create D with this telemetry as its dependent, but we also need to add this telemetry to the list of dependents of A, B, C. This works but seems to be not very intuitive.

If we do it in the alternative approach, e.g. use telemetry -> required_list_of_probes(), it will be more telemetry-centric, in this case, when we introduce a new telemetry, we need not to make any changes to existing probes A, B, C, we just need to create probe D and specify that this new telemetry requires (A,B,C,D). The downside of this though is that there will be repeated logic in each sampler checking whether a probe is needed.

Personally I think both ways work but the telemetry-centric approach might be more intuitive, what do you think?

WUMUXIAN commented 2 years ago

@brayniac Made some changes to reflect the alternative approach, please let me know what're your thoughts.