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

Integration between eBPF and Apps #9178

Merged
merged 94 commits into from Jun 12, 2020
Merged

Conversation

thiagoftsm
Copy link
Contributor

@thiagoftsm thiagoftsm commented May 27, 2020

Summary

Fixes #8912
Fixes #8914

This PR brings the integration between the collectors eBPF and Apps.

Component Name

Collector

Test Plan

1 - Install Netdata using ./netdata-installer.sh --dont-start-it.
2 - Download the file files4.zip and extract it inside /usr/libexec/netdata/plugins.d.
3 - Run netdata and verify the charts on apps.plugin.
4 - Stop netdata after few seconds.
5 - Start netdata again.

Do the steps 4 and 5 at least 3 times. We should have consistency on all charts (general and apps) in all tests.

It is expected to see 14 charts inside apps submenu.

Additional Information

@github-actions github-actions bot added area/build Build system (autotools and cmake). area/collectors Everything related to data collection labels May 27, 2020
@thiagoftsm thiagoftsm changed the title [WIP] Start integration between eBPF and Apps Start integration between eBPF and Apps May 27, 2020
@thiagoftsm thiagoftsm changed the title Start integration between eBPF and Apps [WIP] integration between eBPF and Apps May 28, 2020
@thiagoftsm thiagoftsm force-pushed the ebpf_apps branch 2 times, most recently from 3e40e96 to 43f9957 Compare May 31, 2020 22:28
@ilyam8
Copy link
Member

ilyam8 commented Jun 1, 2020

Neither Summary or Test plan are not clear. Part? What part?

so we expect that the reviewer will compile and verify any possible erro

What kind of errors should we expect? It depends on changes? (go to summary is not clear).

@thiagoftsm
Copy link
Contributor Author

Neither Summary and Test plan are not clear. Part? What part?

so we expect that the reviewer will compile and verify any possible erro

What kind of errors should we expect? It depends on changes? (go to summary is not clear).

This is the motive it is [WIP] we are finalizing it before to give details ;)

@lgtm-com
Copy link

lgtm-com bot commented Jun 2, 2020

This pull request introduces 4 alerts when merging fdca546 into f3d7624 - view on LGTM.com

new alerts:

  • 4 for Local variable hides global variable

@lgtm-com
Copy link

lgtm-com bot commented Jun 2, 2020

This pull request introduces 4 alerts when merging 2d04d4a into f3d7624 - view on LGTM.com

new alerts:

  • 4 for Local variable hides global variable

@lgtm-com
Copy link

lgtm-com bot commented Jun 3, 2020

This pull request introduces 4 alerts when merging dc04f7f into 1aa2cd7 - view on LGTM.com

new alerts:

  • 4 for Local variable hides global variable

int collect_data_for_all_processes(ebpf_process_stat_t **out,
pid_t *index,
int tbl_pid_stats_fd)
#endif
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you doing it this way? Why don't you assign function addresses in process_functions depending on STATIC macro?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The kickstart_static64 will need a different structure to compile, the STATIC is already used on netdata/kernel-collector when we need to drop completely the shared libraries, I am keeping this here for while to keep the same pattern used on the other repository.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not against using STATIC. I just prefer to do it in one place rather than several ones.

@lgtm-com
Copy link

lgtm-com bot commented Jun 3, 2020

This pull request introduces 1 alert when merging eb9853c into b63d58f - view on LGTM.com

new alerts:

  • 1 for Local variable hides global variable

thiagoftsm added a commit to thiagoftsm/netdata that referenced this pull request Jun 3, 2020
@thiagoftsm thiagoftsm requested a review from vlvkobal June 5, 2020 04:39
@thiagoftsm thiagoftsm changed the title [WIP] integration between eBPF and Apps Integration between eBPF and Apps Jun 5, 2020
@thiagoftsm
Copy link
Contributor Author

@vlvkobal I observed that we are having a small problem with the conditional variables. I inserted debug messages inside the function ebpf_socket_send_apps_data() and good part of the time that I was testing, the variables root_pid was cleaned, but this variable is used and set on process threads and the charts are created.

thiagoftsm added a commit to thiagoftsm/netdata that referenced this pull request Jun 5, 2020
@github-actions github-actions bot added the area/packaging Packaging and operating systems support label Jun 5, 2020
@underhood
Copy link
Contributor

apps.task_close is still stuck at 0 in CentOS 8 but it works on ubuntu.

Centos:
Screenshot from 2020-06-11 10-55-00

on bubuntu it looks more healthy:
Screenshot from 2020-06-11 10-56-21

@thiagoftsm
Copy link
Contributor Author

Why all the charts still have no titles? I keep asking it for months smile

I finally brought them, thank you to call my attention for this.

@thiagoftsm
Copy link
Contributor Author

* Why did you make it so?

Because, this was the first collector I wrote for Netdata. ;)

* Why `update_every` is hardcoded?

A big mistake, I fixed it yesterday.

* Why there is no `plugin` and `module`?

Apps plugin also does not have at the end of the line both values, I am stopping on update every due the same reason.

"CHART netdata.apps_cpu '' 'Apps Plugin CPU' 'milliseconds/s' apps.plugin netdata.apps_cpu stacked 140000 %1$d\n"

@thiagoftsm
Copy link
Contributor Author

Zero charts:
Arch - 5.6.15
* file_open

I could not reproduct this problem when I tested on kernel 5.6.17.

* vfs_write_bytes
* vfs_read_bytes
* thread_create
* task_close (it works in general charts)

We really had a problem for entry mode with these charts, the fix that we brought with the release 0.4.3 fixed them.

Ubuntu 18.04 - 4.15.0
Ubuntu 20.04 - 5.4.0
CentOS 7 - 3.10.0
CentOS 8 - 4.18.0
Debian 10 - 4.19.0
Fedora 30 - 5.0.9
OpenSUSE Leap 15.1 - 4.12.14

* vfs_write_bytes
* vfs_read_bytes
* task_close (it works in general charts, except CentOS 8 and Debian 10)

The same thing happens on these distribution, they were affected for the same bug.

@vlvkobal , please, can you test them again? I am also testing on other kernels here.

@thiagoftsm
Copy link
Contributor Author

thiagoftsm commented Jun 11, 2020

apps.task_close is still stuck at 0 in CentOS 8 but it works on ubuntu.

CentOS 8 and Ubuntu have special kernels. For CentOS 8 we have a specific script to compile for it, I will take a look on it now.

@underhood
Copy link
Contributor

@vlvkobal i am not sure we should test for Fed 30 as it is EOL soon

@underhood
Copy link
Contributor

underhood commented Jun 11, 2020

Fedora 32 AMD64 apps.file_open seems not right
Screenshot from 2020-06-11 15-22-00

Same chart from Ubuntu where it seems to be working right for comparison:
Screenshot from 2020-06-11 15-26-00

@vlvkobal
Copy link
Contributor

vlvkobal commented Jun 11, 2020

On Arch 5.6.15, Ubuntu 20.04 apps.file_deleted, apps.vfs_read_call, apps.vfs_read_bytes, apps.process_create, apps.task_close are randomly accumulating some data. There were two CMake builds of Netdata:
image

@thiagoftsm
Copy link
Contributor Author

On Arch 5.6.15, Ubuntu 20.04 apps.file_deleted, apps.vfs_read_call, apps.vfs_read_bytes, apps.process_create, apps.task_close are randomly accumulating some data. There were two CMake builds of Netdata:
image

The problem was fixed with the last commit.

@thiagoftsm thiagoftsm closed this Jun 11, 2020
@thiagoftsm thiagoftsm reopened this Jun 11, 2020
@squash-labs
Copy link

squash-labs bot commented Jun 11, 2020

Manage this branch in Squash

Test this branch here: https://thiagoftsmebpf-apps-ox6u7.squash.io

@underhood underhood self-requested a review June 12, 2020 11:48
@prologic prologic merged commit 9c52a1e into netdata:master Jun 12, 2020
@thiagoftsm thiagoftsm deleted the ebpf_apps branch June 12, 2020 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Change threads to create charts inside Apps submenu Copy functions from apps.plugin to ebpf.plugin
5 participants