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

Provide better info in the default netdata.conf, and unfiy it with the header used for generating config at runtime. #17733

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Ferroin
Copy link
Member

@Ferroin Ferroin commented May 22, 2024

Summary

The first half of this PR is simply clean up and improvements in the default config file (system/netdata.conf) that gets installed at build time. Most of that should be completely non-controversial, and should better help users who are looking to configure the agent.

The second half of the PR involves adjusting src/libnetdata/config/appconfig.c to use system/netdata.conf as the header when it generates the config that gets output by netdatacli dumpconfig or the /netdata.conf API endpoint. This ensures that the information in both places stays in sync properly, and also makes the function responsible for this generation a bit easier to read.

The majority of the changes involved in the second half of the PR revolve around a new CMake script (packaging/cmake/txt2header.cmake) that converts a text file to a C header file that defines a macro which expands to a string consisting of the contents of that text file. This was done in CMake mostly to ensure portability, but it’s also significantly simpler and faster to do this type of simple text transformation in CMake.

Test Plan

Primary testing involves verifying that CI passes on this PR.

Secondary testing involves building the agent, running it, and then inspecting the output of netdatacli dumpconfig or the /netdata.conf web API endpoint to confirm that the headers there match the contents of system/netdata.conf.

Include a link to the documentation, and also better describe the
duumpconfig and /netdata.conf stuff.
This modifies how we are handling generation of the config file exposed
via the /netdata.conf endpoint and the dumpconfig command so that the
file header is provided by system/netdata.conf.

These changes achieve this by adding a CMake script to generate a header
file with a single define containing the text of system/netdata.conf.

CMake was chosen for this instead of something like Python because it’s
implicitly portable to all of our target systems, does not add a new
build-time dependency, and surprisingly enough it’s both simpler and
faster for this specific task.

Overall, this reduces duplication in the code, and ensures that users
get the same info in the comments at the top of netdata.conf regardless
of the source of the file.
@github-actions github-actions bot added area/packaging Packaging and operating systems support area/build Build system (autotools and cmake). labels May 22, 2024
@Ferroin Ferroin marked this pull request as ready for review May 22, 2024 16:32
@Ferroin Ferroin requested review from vkalintiris, a team and thiagoftsm as code owners May 22, 2024 16:32
@ilyam8
Copy link
Member

ilyam8 commented May 22, 2024

This ensures that the information in both places stays in sync properly, and also makes the function responsible for this generation a bit easier to read.

What places? system/netdata.conf and the generated netdata.conf header? It looks like two different things that happen to have the same content now. They were different.

@Ferroin
Copy link
Member Author

Ferroin commented May 22, 2024

This ensures that the information in both places stays in sync properly, and also makes the function responsible for this generation a bit easier to read.

What places? system/netdata.conf and the generated netdata.conf header? It looks like two different things that happen to have the same content now. They were different.

They were different. They are functionally not any more, even though the content is currently different (because src/libnetdata/config/appconfig.c never got updated when we added more info to system/netdata.conf last time), because we have no actual config in our default configuration file. The info in both places should be the same though, because it serves the same purpose.

Drawing from one place for both means we only ever have to update that information that should stay in-sync in one place.

Copy link
Contributor

@vkalintiris vkalintiris left a comment

Choose a reason for hiding this comment

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

No. You can use python to do this and avoid locking us into cmake.

@vkalintiris
Copy link
Contributor

No. You can use python to do this and avoid locking us into cmake.

Or even better, install a hidden/default netdata.conf -> open/read its contents and print it when dumping the runtime netdata.conf.

@Ferroin
Copy link
Member Author

Ferroin commented May 31, 2024

@vkalintiris The intent behind using CMake here was that:

  1. It avoids adding yet another build dependency (we currently do not need Python at all at build time). This is really important from a packaging perspective, because adding new hard dependencies at build time involves a nontrivial amount of additional complexity in the update process (and like it or not, we do need to keep supporting the existing installs where people built Netdata locally).
  2. It avoids causing the issues with cross-compilation that doing it in C would have (CMake does not handle tools built to be used during the build process well when cross compiling).
  3. It’s actually significantly simpler than it is to do the same thing in Python or C because of how CMake handles string processing.

I actually did consider loading the file at runtime, but I had expected the additional complexity that would be required to correctly handle that would be a major point of contention. If that’s really the route we want to take I can rewrite it though.

@vkalintiris
Copy link
Contributor

If that’s really the route we want to take I can rewrite it though.

It is. We don't want to rely on the build to system to generate source code at configuration time.

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/packaging Packaging and operating systems support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants