Skip to content
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

Merged
merged 14 commits into from
Nov 19, 2021

Conversation

WUMUXIAN
Copy link
Contributor

@WUMUXIAN WUMUXIAN commented Nov 10, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Copy link
Contributor

@rittme rittme left a 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.

src/common/bpf.rs Outdated Show resolved Hide resolved
src/samplers/disk/stat.rs Outdated Show resolved Hide resolved
src/samplers/disk/mod.rs Outdated Show resolved Hide resolved
src/samplers/ext4/stat.rs Outdated Show resolved Hide resolved
src/samplers/interrupt/stat.rs Outdated Show resolved Hide resolved
src/samplers/tcp/stat.rs Outdated Show resolved Hide resolved
src/common/bpf.rs Outdated Show resolved Hide resolved
src/common/bpf.rs Outdated Show resolved Hide resolved
src/common/bpf.rs Outdated Show resolved Hide resolved
src/samplers/disk/mod.rs Show resolved Hide resolved
src/samplers/disk/mod.rs Outdated Show resolved Hide resolved
src/samplers/disk/stat.rs Outdated Show resolved Hide resolved
src/samplers/ext4/stat.rs Outdated Show resolved Hide resolved
src/samplers/ext4/stat.rs Outdated Show resolved Hide resolved
src/samplers/interrupt/mod.rs Show resolved Hide resolved
@brayniac brayniac merged commit 9bedb6c into twitter:master Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants