-
Notifications
You must be signed in to change notification settings - Fork 115
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
Do not attach a probe if none of the statistics requiring it is enabled. #254
Do not attach a probe if none of the statistics requiring it is enabled. #254
Conversation
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? |
@brayniac Made some changes to reflect the alternative approach, please let me know what're your thoughts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new approach looks good to me. My comments are mostly nits.
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