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

Set loglevels from config #4091

Merged
merged 8 commits into from Sep 19, 2022
Merged

Conversation

bazsi
Copy link
Collaborator

@bazsi bazsi commented Jul 29, 2022

This patch adds a feature to control the internal verbosity of syslog-ng (e.g. verbose, debug, trace) via the configuration file.

Example:

    options {
      log-level(debug);
    };

Possible values for log-level:
- default, just normal log messages
- verbose, normal + messages that get enabled via -v
- debug, verbose + messages that get enabled via -d
- trace, debug + messages that get enabled via -t

If the option is unset, we leave the settings as specified on the command line.

If we have log-level(default) it would override settings specified via the command line.

Rationale: in containerized environments, it is much easier to override log verbosity settings via the config file than via changing the command line options.

@github-actions
Copy link
Contributor

No news file has been detected. Please write one, if applicable.

@lgtm-com
Copy link

lgtm-com bot commented Jul 29, 2022

This pull request introduces 1 alert when merging 20c4f4daafa815b8a178b47e1dd85fa983388641 into d8d8d0f - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@bazsi bazsi force-pushed the set-loglevels-from-config branch from 20c4f4d to 7410142 Compare July 29, 2022 12:20
@MrAnno
Copy link
Collaborator

MrAnno commented Jul 29, 2022

Shouldn't we work the other way around? Command line options are usually stronger than configuration file options as they are easier to modify. For example, starting syslog-ng with -d is much quicker than removing/changing the option in the config.

AFAIK other tools work similarly, for example, nginx has a command line flag to override a few directives inside the config file.

@bazsi
Copy link
Collaborator Author

bazsi commented Jul 29, 2022 via email

@bazsi bazsi force-pushed the set-loglevels-from-config branch from 7410142 to a307bdb Compare August 4, 2022 18:38
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi bazsi force-pushed the set-loglevels-from-config branch from a307bdb to 4cb5d58 Compare August 7, 2022 06:33
@kira-syslogng
Copy link
Contributor

Build FAILURE

1 similar comment
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi bazsi force-pushed the set-loglevels-from-config branch 2 times, most recently from fd2ff11 to a60eed7 Compare August 24, 2022 10:58
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi
Copy link
Collaborator Author

bazsi commented Aug 24, 2022

@kira-syslogng seems to build on the response from "syslog-ng-ctl debug" which was changed by this PR. Can anyone help me with fixing that dependency?

@kira-syslogng
Copy link
Contributor

Build FAILURE

@MrAnno
Copy link
Collaborator

MrAnno commented Aug 28, 2022

@bazsi Of course. I'll take a look at this next week.

@MrAnno MrAnno self-requested a review August 28, 2022 20:43
@kira-syslogng
Copy link
Contributor

Build FAILURE

@mitzkia
Copy link
Collaborator

mitzkia commented Sep 2, 2022

@kira-syslogng test this please test branch=followup_log_level_changes; test=functions/tools/syslog-ng-ctl/;

@kira-syslogng
Copy link
Contributor

Build FAILURE

@mitzkia
Copy link
Collaborator

mitzkia commented Sep 2, 2022

@kira-syslogng test this please test branch=followup_log_level_changes; test=functions/tools/syslog-ng-ctl/;

@kira-syslogng
Copy link
Contributor

Build FAILURE

@MrAnno
Copy link
Collaborator

MrAnno commented Sep 2, 2022

@kira-syslogng test this please test branch=followup_log_level_changes;

@kira-syslogng
Copy link
Contributor

Build FAILURE

@MrAnno
Copy link
Collaborator

MrAnno commented Sep 2, 2022

I'm just thinking out loud:
This PR deliberately replaces our composable diagnostic flags -Fdevt with a more straightforward approach: log levels.

This means that the -Fdevt is not considered useful anymore as -Fet produces the same level.

In that case, we could say that the old syslog-ng-ctl commands are just for backward compatibility (deprecated), log-level should be used instead. I think the same is true for the command line flags: Since using -dvt is not intuitive anymore either, a --log-level flag would be really useful and could maintain a consistent feel.

@MrAnno
Copy link
Collaborator

MrAnno commented Sep 9, 2022

@bazsi What do you think?

@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi
Copy link
Collaborator Author

bazsi commented Sep 9, 2022

@bazsi What do you think?

done.

@bazsi
Copy link
Collaborator Author

bazsi commented Sep 12, 2022

@kira-syslogng test this please test branch=followup_log_level_changes;

@kira-syslogng
Copy link
Contributor

Build FAILURE

@mitzkia
Copy link
Collaborator

mitzkia commented Sep 15, 2022

Just for the record about the latest failing Kira job.
Somehow Jenkins clones out not the PR itself, but the master branch, and because of this the test assertion fails.
In the other Kira test job everything is ok, new testcase assertion passes on PR changes.

@mitzkia
Copy link
Collaborator

mitzkia commented Sep 16, 2022

@kira-syslogng test this please test branch=followup_log_level_changes;

@kira-syslogng
Copy link
Contributor

Build FAILURE

@mitzkia
Copy link
Collaborator

mitzkia commented Sep 16, 2022

@kira-syslogng test this please test branch=followup_log_level_changes;

@mitzkia
Copy link
Collaborator

mitzkia commented Sep 16, 2022

Latest Kira-starter job runs correctly (I have fixed the Kira job):

  • it uses this PR as a source in both cases (cmake and autotools build), and
  • the correct topic branch for testdb

@bazsi
Copy link
Collaborator Author

bazsi commented Sep 18, 2022

Thanks @mitzkia

With that I think we can merge this once it gets a thumbs up from someone.

lib/messages.c Outdated Show resolved Hide resolved
lib/messages.c Outdated Show resolved Hide resolved
lib/messages.c Show resolved Hide resolved
lib/cfg.c Show resolved Hide resolved
lib/messages.c Show resolved Hide resolved
lib/messages.c Show resolved Hide resolved
lib/mainloop-control.c Show resolved Hide resolved
@bazsi bazsi force-pushed the set-loglevels-from-config branch 2 times, most recently from 891aa9a to 380bc79 Compare September 18, 2022 14:36
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
This patch adds a feature to control the internal verbosity of syslog-ng
(e.g. verbose, debug, trace) via the configuration file.

Example:

options {
  log-level(debug);
};

Possible values for log-level:
  - default, just normal log messages
  - verbose, normal + messages that get enabled via -v
  - debug, verbose + messages that get enabled via -d
  - trace, debug + messages that get enabled via -t

The command line continues to dominate the log-level, e.g.  if the command
line contains options to control the level of logging, the configuration file would be ignored.

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
…-ctl to log levels

syslog-ng-ctl is issuing separate commands for various log levels, e.g.

LOG VERBOSE ON
LOG DEBUG ON
LOG TRACE ON

which was operating on various verbosity flags exported by the messages
module.  This patch changes the implementation so while the interface
continues to work, we are mapping the control commands to log levels.

Whevever we turn ON verbose/debug/trace, we set the log level to that
specific values.  When we turn them OFF we set the log level one less than
the specific value.

LOG VERBOSE ON => verbose
LOG VERBOSE OFF => default
LOG DEBUG ON => debug
LOG DEBUG OFF => verbose
LOG TRACE ON => trace
LOG TRACE OFF => debug

This keeps compatibility with old syslog-ng-ctl binaries with a tiny change
in functionality: the user will not be able to enable trace messages while
not enabling debug at the same time.

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
…ate function

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
…tl commands

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
This patch adds "log-level" to syslog-ng-ctl while using the old control
commands.

Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
Signed-off-by: Balazs Scheidler <bazsi77@gmail.com>
@bazsi
Copy link
Collaborator Author

bazsi commented Sep 18, 2022

added some more unit tests to cover the cmdline/config overrides.

@bazsi bazsi requested a review from MrAnno September 18, 2022 14:56
@MrAnno
Copy link
Collaborator

MrAnno commented Sep 18, 2022

@kira-syslogng test this please test branch=followup_log_level_changes;

Copy link
Collaborator

@MrAnno MrAnno left a comment

Choose a reason for hiding this comment

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

Thank you. LGTM.

We'll merge this PR on Monday together with the internal testdb modification.

{
gboolean on = g_str_equal(onoff, "ON");
if (!on)
ll = ll - 1;
Copy link
Collaborator

@MrAnno MrAnno Sep 18, 2022

Choose a reason for hiding this comment

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

One last note:

Asking our users to enable debug logging in production for a short period of time is quite usual, the support team does this frequently in the case of syslog-ng PE.

"Debug on" and then "debug off" now are not inverse functions.

I think this can be documented, and we can inform the affected parties about the change and the new intuitive log-level alternative.

@MrAnno MrAnno merged commit 79a7d9b into syslog-ng:master Sep 19, 2022
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

4 participants