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.

Sign up for GitHub

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

Jump to bottom

Add Linux page cache metrics to eBPF #10693

Merged
merged 15 commits into from Mar 3, 2021
Merged

Add Linux page cache metrics to eBPF #10693

merged 15 commits into from Mar 3, 2021

Conversation

thiagoftsm
Copy link
Contributor

Summary

When I finish the documentation I will give more details.

Component Name
Test Plan
Additional Information

@thiagoftsm thiagoftsm marked this pull request as draft February 26, 2021 04:30
@github-actions github-actions bot added area/build Build system (autotools and cmake). area/collectors Everything related to data collection area/packaging Packaging and operating systems support labels Feb 26, 2021
@knatsakis knatsakis removed their request for review February 26, 2021 12:41
@thiagoftsm thiagoftsm marked this pull request as ready for review March 1, 2021 01:11
vlvkobal
vlvkobal suggested changes Mar 1, 2021
View reviewed changes
Copy link
Contributor

@vlvkobal vlvkobal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is ebpf_modules[1] in L1676. Please use a macro instead of the number.

web/gui/dashboard_info.js Outdated Show resolved Hide resolved
vlvkobal
vlvkobal reviewed Mar 1, 2021
View reviewed changes
collectors/ebpf.plugin/ebpf.conf
@@ -19,11 +19,13 @@
#
# The eBPF collector enables and runs the following eBPF programs by default:
#
# `cachestat`: Make charts for kernel functions related to page cache.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you use the cachestat word? I think we don't need to add the stat suffix. We can simply use cache or pagecache.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using cachestat to keep compatibility with the names used by IOvisor, this was defined together our Product team (@manos-saratsis).

collectors/ebpf.plugin/ebpf_cachestat.h Outdated Show resolved Hide resolved
collectors/ebpf.plugin/ebpf_cachestat.c Outdated Show resolved Hide resolved
@thiagoftsm thiagoftsm requested a review from vlvkobal March 1, 2021 18:51
vlvkobal
vlvkobal reviewed Mar 1, 2021
View reviewed changes
collectors/ebpf.plugin/ebpf_cachestat.c Outdated Show resolved Hide resolved
collectors/ebpf.plugin/ebpf_cachestat.c Outdated Show resolved Hide resolved
vlvkobal
vlvkobal reviewed Mar 1, 2021
View reviewed changes
web/gui/dashboard_info.js Outdated Show resolved Hide resolved
@thiagoftsm thiagoftsm requested a review from vlvkobal March 1, 2021 20:07
@vlvkobal
Copy link
Contributor

vlvkobal commented Mar 2, 2021

Why are there data gaps in the apps.cachestat_ratio chart?

image

@thiagoftsm
Copy link
Contributor Author

Why are there data gaps in the apps.cachestat_ratio chart?

image

This is a problem with the original algorithm and it is more visible when we take a look direct for application. This is an expected behavior, considering that we are sending data for Netdata only when ratio is different of zero. We took this decision to force a more smooth chart.

@vlvkobal
Copy link
Contributor

vlvkobal commented Mar 2, 2021

I wouldn't say it is smooth. Especially this one:

image

@thiagoftsm
Copy link
Contributor Author

I wouldn't say it is smooth. Especially this one:

I agree, I was trying to keep the same algorithm for apps and the mem, but I changed to have a better chart, at least on my environment it is smother now, please, test on yours.

@underhood
Copy link
Contributor

underhood commented Mar 3, 2021 •
edited

@thiagoftsm sorry have been away for few days. Will start reviewing immediately.

First comment: the PR name is misleading (this is important because it goes to change log of netdata release):
Ebpf page cache makes me think you are implementing cache inside eBPF collector to avoid some allocations or sth.
This PR should be named sth. like add Linux page cache metrics to eBPF ?

@thiagoftsm thiagoftsm changed the title Ebpf page cache Add Linux page cache metrics to eBPF Mar 3, 2021
@thiagoftsm thiagoftsm merged commit 71e7114 into netdata:master Mar 3, 2021
@thiagoftsm thiagoftsm deleted the ebpf_page_cache branch March 3, 2021 16:08
Saruspete pushed a commit to Saruspete/netdata that referenced this pull request Mar 17, 2021
@thiagoftsm @Saruspete
Add Linux page cache metrics to eBPF (netdata#10693)
f23e925 
Add new eBPF thread to display page cache utilization.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@underhood underhood underhood approved these changes

@vlvkobal vlvkobal vlvkobal approved these changes

@manos-saratsis manos-saratsis Awaiting requested review from manos-saratsis

@ilyam8 ilyam8 Awaiting requested review from ilyam8

@Ferroin Ferroin Awaiting requested review from Ferroin Ferroin is a code owner

@ktsaou ktsaou Awaiting requested review from ktsaou

@vkalintiris vkalintiris Awaiting requested review from vkalintiris vkalintiris is a code owner

@jacekkolasa jacekkolasa Awaiting requested review from jacekkolasa

Assignees
No one assigned
Labels
area/build Build system (autotools and cmake). area/collectors Everything related to data collection area/packaging Packaging and operating systems support area/web
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@thiagoftsm @vlvkobal @underhood