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

modularized all source code #4391

Merged
merged 61 commits into from Oct 15, 2018
Merged

modularized all source code #4391

merged 61 commits into from Oct 15, 2018

Conversation

@ktsaou
Copy link
Member

@ktsaou ktsaou commented Oct 13, 2018

This PR changes the structure of source tree for almost every piece of code and config file.

This was required to allow more people step into netdata source tree, understand how it works, allow netdata to integrate sub-gits into it, etc.

The new structure follows the key functions of netdata:

  • collectors
  • backends
  • daemon
  • database
  • health
  • libnetdata
  • registry
  • streaming
  • web

Every file of netdata is now under these directories.

@ktsaou
Copy link
Member Author

@ktsaou ktsaou commented Oct 13, 2018

The new directory structure can be browsed here: https://github.com/ktsaou/netdata/tree/plugins3

Copy link
Contributor

@paulfantom paulfantom left a comment

src/ should remain as a directory for C code only. I don't see any reason to create more directories to host plugins code.

@ktsaou
Copy link
Member Author

@ktsaou ktsaou commented Oct 13, 2018

src/ should remain as a directory for C code only

Why only C code should be in src/?

The key point here is that every plugin and every module should be in its own directory.

I prefer to have them under src in a well organized directory structure so that people can also have visibility of how this works:

src/
+ plugins/
    + plugins.d.plugin/      <<< this internal plugin provides the external plugins API
        + python.d.plugin/   <<< this one uses that API and provides python data collection modules
            + README.md      <<< info about writing python modules
            + nginx/         <<< every file related to nginx, including a README.md
            + web_log/

So, you don't like non-C code to be in src/. Where should it be and why?

@paulfantom
Copy link
Contributor

@paulfantom paulfantom commented Oct 13, 2018

If you put everything in src then what is the point of other directories in top level? Top level directories are the first thing you see when going to github repository, they should reflect modularity of the source code. Putting all code into src is just adding one more level of indirection and looks more like sweeping mess under the rug.

Another aspect is the fact that core part written in C is the only one which needs compilation. This also can be reflected by a directory structure. I don't think there is a need to compile whole netdata when developing only apps_plugin. This increase in developer experience could be achieved partly by changing build system and partly by better directory structure.

In my opinion it would be more beneficial if code would be structured in something similar to:

web/ (if posssible)
core/
plugins/
  python.plugin/
    modules/
      nginx.py
      web_log.py
  node.plugin/
    modules/
...

This way you have clear distinction what is what and where it resides. Also I don't see any need to have directories like nginx for modules. As it is now, modules are small single files and they should stay that way. Adding another directory can potentially lead to a change in that situation.

It would be possibly even better if plugins directory was structured in a way which shows what those plugins do. Like input, output, mangle, alert etc. So maybe even structure like this would be better:

web/ (if posssible)
core/
plugins/
  input/
    python.plugin/
      modules/
        nginx.py
        web_log.py
    node.plugin/
      modules/
...

TL;DR Group directories first by function then by code type to provide better DevEX and modularity.


As for other things:

info about writing python modules

This should be in CONTRIBUTING.md as well as in documentation. If there is a need for it in code, then it should be in docs/ top level directory

every file related to nginx, including a README.md

Almost no one reads READMEs which are far down in directory layout. This should be in documentation.


Also look at this part:

+ plugins/
    + plugins.d.plugin/      <<< this internal plugin provides the external plugins API

This is pointless, you put plugins... under plugins.

@paulfantom
Copy link
Contributor

@paulfantom paulfantom commented Oct 13, 2018

Also regarding build system, good directory layout can lead to less code generation. For example Makefiles for python plugin doesn't need to be automatically created by autoconf/cmake and they can consist of developer tooling. This means that if someone is developing only parts of python code, then he/she doesn't need things like gcc, cmake/autoconf etc. as only thing needed is to have standard tools needed for particular language (python in this case). Not even core part is needed and if it is then can be downloaded from nightly build or run with docker container.

@ktsaou
Copy link
Member Author

@ktsaou ktsaou commented Oct 13, 2018

Then I think the best way is to move src/*.c and src/*.h to a directory called daemon and move everything off src/. That is, remove the src/ directory completely.

@ktsaou
Copy link
Member Author

@ktsaou ktsaou commented Oct 13, 2018

Navigate a bit this: https://github.com/ktsaou/netdata/tree/plugins3/src/plugins
I really like how it is now.

@ktsaou
Copy link
Member Author

@ktsaou ktsaou commented Oct 13, 2018

So, what do you think? Shall I try to remove src/ completely?
I think this would provide the best visibility on what netdata does.

The list will be like that:

image

With a few additions for installer, contrib, makeself, etc.

@ktsaou
Copy link
Member Author

@ktsaou ktsaou commented Oct 13, 2018

I really like removing it... It will be a great improvement...

@ktsaou ktsaou changed the title modularized all external plugins [RFC] modularized all external plugins Oct 13, 2018
@ktsaou
Copy link
Member Author

@ktsaou ktsaou commented Oct 13, 2018

Added [RFC] at the title, to let everyone know I am open for ideas about this.

@ktsaou ktsaou requested a review from Ferroin as a code owner Oct 14, 2018
@ktsaou
Copy link
Member Author

@ktsaou ktsaou commented Oct 15, 2018

oops! sorry @paulfantom @Ferroin
I modularized the web server and dismissed your reviews.
Could you please approve this again?

@ktsaou ktsaou changed the title [RFC] modularized all external plugins modularized all source code Oct 15, 2018
CMakeLists.txt Show resolved Hide resolved
Copy link
Contributor

@paulfantom paulfantom left a comment

I ran some basic tests and it seems to be ok.

@ktsaou ktsaou merged commit 8fbf817 into netdata:master Oct 15, 2018
6 of 7 checks passed
6 of 7 checks passed
Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
LGTM analysis: C/C++ No alert changes
Details
LGTM analysis: JavaScript No alert changes
Details
LGTM analysis: Python No alert changes
Details
@travis-ci
Travis CI - Pull Request Build Passed
Details
@wip
WIP ready for review
Details
license/cla Contributor License Agreement is signed.
Details
@ktsaou ktsaou deleted the ktsaou:plugins3 branch Oct 15, 2018
@paulfantom paulfantom mentioned this pull request Oct 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment