[PATCH] services: nginx: Make logging level configurable.

  • Done
  • quality assurance status badge
Details
3 participants
  • Ludovic Courtès
  • Maxim Cournoyer
  • Bruno Victal
Owner
unassigned
Submitted by
Bruno Victal
Severity
normal
B
B
Bruno Victal wrote on 27 Mar 2023 20:51
(address . guix-patches@gnu.org)(name . Bruno Victal)(address . mirai@makinata.eu)
fa44bc8a46021c3d307479ea2fbdf04d68e0cd10.1679942599.git.mirai@makinata.eu
* gnu/services/web.scm (<nginx-configuration>)[log-level]: New field.
(assert-valid-log-level): New procedure.
(default-nginx-config): Make log-level configurable.
* doc/guix.texi (Web Services): Document it.
---

Note: Hardcoding this value to 'info is extremely bad for
flash endurance, and results in very large logs.

doc/guix.texi | 5 +++++
gnu/services/web.scm | 24 +++++++++++++++++++++++-
2 files changed, 28 insertions(+), 1 deletion(-)

Toggle diff (95 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index c49e51b72e..8df3c285ad 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -29838,6 +29838,11 @@ Web Services
@item @code{log-directory} (default: @code{"/var/log/nginx"})
The directory to which NGinx will write log files.
+@item @code{log-level} (default: @code{'error}) (type: symbol)
+Logging level, which can be any of the following values: @code{'debug},
+@code{'info}, @code{'notice}, @code{'warn}, @code{'error}, @code{'crit},
+@code{'alert}, or @code{'emerg}.
+
@item @code{run-directory} (default: @code{"/var/run/nginx"})
The directory in which NGinx will create a pid file, and write temporary
files.
diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index d56e893527..aef694e145 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -51,6 +51,9 @@ (define-module (gnu services web)
#:use-module (gnu packages logging)
#:use-module (gnu packages mail)
#:use-module (gnu packages rust-apps)
+ #:autoload (guix i18n) (G_)
+ #:use-module (guix combinators)
+ #:use-module (guix diagnostics)
#:use-module (guix packages)
#:use-module (guix records)
#:use-module (guix modules)
@@ -61,6 +64,8 @@ (define-module (gnu services web)
#:use-module ((guix packages) #:select (package-version))
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-9)
+ #:use-module (srfi srfi-34)
+ #:use-module (srfi srfi-35)
#:use-module (ice-9 match)
#:use-module (ice-9 format)
#:export (httpd-configuration
@@ -96,6 +101,7 @@ (define-module (gnu services web)
nginx-configuration-nginx
nginx-configuration-shepherd-requirement
nginx-configuration-log-directory
+ nginx-configuration-log-level
nginx-configuration-run-directory
nginx-configuration-server-blocks
nginx-configuration-upstream-blocks
@@ -562,6 +568,9 @@ (define-record-type* <nginx-configuration>
(default '())) ;list of symbols
(log-directory nginx-configuration-log-directory ;string
(default "/var/log/nginx"))
+ (log-level nginx-configuration-log-level
+ (sanitize assert-valid-log-level)
+ (default 'error))
(run-directory nginx-configuration-run-directory ;string
(default "/var/run/nginx"))
(server-blocks nginx-configuration-server-blocks
@@ -584,6 +593,18 @@ (define-record-type* <nginx-configuration>
(file nginx-configuration-file ;#f | string | file-like
(default #f)))
+(define-compile-time-procedure (assert-valid-log-level (level symbol?))
+ "Ensure @var{level} is one of @code{'debug}, @code{'info}, @code{'notice},
+@code{'warn}, @code{'error}, @code{'crit}, @code{'alert}, or @code{'emerg}."
+ (unless (memq level '(debug info notice warn error crit alert emerg))
+ (raise
+ (make-compound-condition
+ (formatted-message (G_ "unknown log level '~a'") level)
+ (condition (&error-location
+ (location
+ (source-properties->location procedure-call-location)))))))
+ level)
+
(define (config-domain-strings names)
"Return a string denoting the nginx config representation of NAMES, a list
of domain names."
@@ -692,6 +713,7 @@ (define (default-nginx-config config)
(match-record config
<nginx-configuration>
(nginx log-directory run-directory
+ log-level
server-blocks upstream-blocks
server-names-hash-bucket-size
server-names-hash-bucket-max-size
@@ -704,7 +726,7 @@ (define (default-nginx-config config)
(flatten
"user nginx nginx;\n"
"pid " run-directory "/pid;\n"
- "error_log " log-directory "/error.log info;\n"
+ "error_log " log-directory "/error.log " (symbol->string log-level) ";\n"
(map emit-load-module modules)
(map emit-global-directive global-directives)
"http {\n"
--
2.39.1
M
M
Maxim Cournoyer wrote on 28 Mar 2023 16:12
(name . Bruno Victal)(address . mirai@makinata.eu)
87r0t9newh.fsf@gmail.com
Hello,

Bruno Victal <mirai@makinata.eu> writes:

Toggle quote (5 lines)
> * gnu/services/web.scm (<nginx-configuration>)[log-level]: New field.
> (assert-valid-log-level): New procedure.
> (default-nginx-config): Make log-level configurable.
> * doc/guix.texi (Web Services): Document it.

Thanks!

Toggle quote (82 lines)
> ---
>
> Note: Hardcoding this value to 'info is extremely bad for
> flash endurance, and results in very large logs.
>
> doc/guix.texi | 5 +++++
> gnu/services/web.scm | 24 +++++++++++++++++++++++-
> 2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index c49e51b72e..8df3c285ad 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -29838,6 +29838,11 @@ Web Services
> @item @code{log-directory} (default: @code{"/var/log/nginx"})
> The directory to which NGinx will write log files.
>
> +@item @code{log-level} (default: @code{'error}) (type: symbol)
> +Logging level, which can be any of the following values: @code{'debug},
> +@code{'info}, @code{'notice}, @code{'warn}, @code{'error}, @code{'crit},
> +@code{'alert}, or @code{'emerg}.
> +
> @item @code{run-directory} (default: @code{"/var/run/nginx"})
> The directory in which NGinx will create a pid file, and write temporary
> files.
> diff --git a/gnu/services/web.scm b/gnu/services/web.scm
> index d56e893527..aef694e145 100644
> --- a/gnu/services/web.scm
> +++ b/gnu/services/web.scm
> @@ -51,6 +51,9 @@ (define-module (gnu services web)
> #:use-module (gnu packages logging)
> #:use-module (gnu packages mail)
> #:use-module (gnu packages rust-apps)
> + #:autoload (guix i18n) (G_)
> + #:use-module (guix combinators)
> + #:use-module (guix diagnostics)
> #:use-module (guix packages)
> #:use-module (guix records)
> #:use-module (guix modules)
> @@ -61,6 +64,8 @@ (define-module (gnu services web)
> #:use-module ((guix packages) #:select (package-version))
> #:use-module (srfi srfi-1)
> #:use-module (srfi srfi-9)
> + #:use-module (srfi srfi-34)
> + #:use-module (srfi srfi-35)
> #:use-module (ice-9 match)
> #:use-module (ice-9 format)
> #:export (httpd-configuration
> @@ -96,6 +101,7 @@ (define-module (gnu services web)
> nginx-configuration-nginx
> nginx-configuration-shepherd-requirement
> nginx-configuration-log-directory
> + nginx-configuration-log-level
> nginx-configuration-run-directory
> nginx-configuration-server-blocks
> nginx-configuration-upstream-blocks
> @@ -562,6 +568,9 @@ (define-record-type* <nginx-configuration>
> (default '())) ;list of symbols
> (log-directory nginx-configuration-log-directory ;string
> (default "/var/log/nginx"))
> + (log-level nginx-configuration-log-level
> + (sanitize assert-valid-log-level)
> + (default 'error))
> (run-directory nginx-configuration-run-directory ;string
> (default "/var/run/nginx"))
> (server-blocks nginx-configuration-server-blocks
> @@ -584,6 +593,18 @@ (define-record-type* <nginx-configuration>
> (file nginx-configuration-file ;#f | string | file-like
> (default #f)))
>
> +(define-compile-time-procedure (assert-valid-log-level (level symbol?))
> + "Ensure @var{level} is one of @code{'debug}, @code{'info}, @code{'notice},
> +@code{'warn}, @code{'error}, @code{'crit}, @code{'alert}, or @code{'emerg}."
> + (unless (memq level '(debug info notice warn error crit alert emerg))
> + (raise
> + (make-compound-condition
> + (formatted-message (G_ "unknown log level '~a'") level)
> + (condition (&error-location
> + (location
> + (source-properties->location procedure-call-location)))))))
> + level)

It's the first time I've seen define-compile-time-procedure in actual
use. Is it really necessary? What happens if you omit wrapping
assert-valid-log-level with it?

Toggle quote (21 lines)
> (define (config-domain-strings names)
> "Return a string denoting the nginx config representation of NAMES, a list
> of domain names."
> @@ -692,6 +713,7 @@ (define (default-nginx-config config)
> (match-record config
> <nginx-configuration>
> (nginx log-directory run-directory
> + log-level
> server-blocks upstream-blocks
> server-names-hash-bucket-size
> server-names-hash-bucket-max-size
> @@ -704,7 +726,7 @@ (define (default-nginx-config config)
> (flatten
> "user nginx nginx;\n"
> "pid " run-directory "/pid;\n"
> - "error_log " log-directory "/error.log info;\n"
> + "error_log " log-directory "/error.log " (symbol->string log-level) ";\n"
> (map emit-load-module modules)
> (map emit-global-directive global-directives)
> "http {\n"

The rest LGTM.

--
Thanks,
Maxim
B
B
Bruno Victal wrote on 28 Mar 2023 16:32
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
156c2153-cca0-2f59-6ca8-edad4fb7e919@makinata.eu
Hi Maxim,

On 2023-03-28 15:12, Maxim Cournoyer wrote:
Toggle quote (18 lines)
> Bruno Victal <mirai@makinata.eu> writes:
>>
>> +(define-compile-time-procedure (assert-valid-log-level (level symbol?))
>> + "Ensure @var{level} is one of @code{'debug}, @code{'info}, @code{'notice},
>> +@code{'warn}, @code{'error}, @code{'crit}, @code{'alert}, or @code{'emerg}."
>> + (unless (memq level '(debug info notice warn error crit alert emerg))
>> + (raise
>> + (make-compound-condition
>> + (formatted-message (G_ "unknown log level '~a'") level)
>> + (condition (&error-location
>> + (location
>> + (source-properties->location procedure-call-location)))))))
>> + level)
>
> It's the first time I've seen define-compile-time-procedure in actual
> use. Is it really necessary? What happens if you omit wrapping
> assert-valid-log-level with it?

It will still work, provided the declaration is adjusted accordingly.
As for the reasons and benefits of using define-compile-time-procedure,


Cheers,
Bruno
M
M
Maxim Cournoyer wrote on 28 Mar 2023 17:03
(name . Bruno Victal)(address . mirai@makinata.eu)
87ilekor3i.fsf@gmail.com
OK,

Bruno Victal <mirai@makinata.eu> writes:

Toggle quote (25 lines)
> Hi Maxim,
>
> On 2023-03-28 15:12, Maxim Cournoyer wrote:
>> Bruno Victal <mirai@makinata.eu> writes:
>>>
>>> +(define-compile-time-procedure (assert-valid-log-level (level symbol?))
>>> + "Ensure @var{level} is one of @code{'debug}, @code{'info}, @code{'notice},
>>> +@code{'warn}, @code{'error}, @code{'crit}, @code{'alert}, or @code{'emerg}."
>>> + (unless (memq level '(debug info notice warn error crit alert emerg))
>>> + (raise
>>> + (make-compound-condition
>>> + (formatted-message (G_ "unknown log level '~a'") level)
>>> + (condition (&error-location
>>> + (location
>>> + (source-properties->location procedure-call-location)))))))
>>> + level)
>>
>> It's the first time I've seen define-compile-time-procedure in actual
>> use. Is it really necessary? What happens if you omit wrapping
>> assert-valid-log-level with it?
>
> It will still work, provided the declaration is adjusted accordingly.
> As for the reasons and benefits of using define-compile-time-procedure,
> it's best explained at <https://logs.guix.gnu.org/guix/2023-03-20.log#131047>.

I guess it's not actually useful here, since none of the fields thunked?

As another note, the nginx-configuration record looks simple enough that
perhaps it'd best be migrated to use the define-configuration method,
which could be made as a prior commit to this change.

As a benefit it'd validate the other field types as well.

--
Thanks,
Maxim
L
L
Ludovic Courtès wrote on 2 Apr 2023 17:39
Re: bug#62490: [PATCH] services: nginx: Make logging level configurable.
(name . Bruno Victal)(address . mirai@makinata.eu)
87jzyuths2.fsf@gnu.org
Hi,

Bruno Victal <mirai@makinata.eu> skribis:

Toggle quote (4 lines)
> +(define-compile-time-procedure (assert-valid-log-level (level symbol?))
> + "Ensure @var{level} is one of @code{'debug}, @code{'info}, @code{'notice},
> +@code{'warn}, @code{'error}, @code{'crit}, @code{'alert}, or @code{'emerg}."

As it turns out, ‘define-compile-time-procedure’ cannot work with
symbols. In short, that’s because in the end the generated macro
checks:

(symbol? #'(quote debug))

which doesn’t do what we want.

Anyway, you can either make it a regular procedure instead, or use a
trick found in R6RS and used in some places in Guix, Guile-Gcrypt, etc.,
which is to define a macro that validates things:

(endianness little) ;R6RS

(operating-id valid-path?) ;(guix store), with ‘define-enumerate-type’

Making it a procedure is prolly good enough. The compiler can optimize
it out at compile time, FWIW:

Toggle snippet (4 lines)
scheme@(guile-user)> ,optimize (unless (memq 'debug '(debug info)) (throw 'x))
$13 = (if #f #f)

Ludo’.
B
B
Bruno Victal wrote on 3 Apr 2023 13:58
[PATCH v2 1/2] services: nginx: Make logging level configurable.
(address . 62490@debbugs.gnu.org)
4027e6a1d7a59e11a7f4000b821283a1985f1dcc.1680523067.git.mirai@makinata.eu
* gnu/services/web.scm (<nginx-configuration>)[log-level]: New field.
(assert-valid-log-level): New procedure.
(default-nginx-config): Make log-level configurable.
* doc/guix.texi (Web Services): Document it.
---
doc/guix.texi | 5 +++++
gnu/services/web.scm | 19 ++++++++++++++++++-
2 files changed, 23 insertions(+), 1 deletion(-)

Toggle diff (97 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index 4f72e2f34a..6a2380aa3e 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -29838,6 +29838,11 @@ Web Services
@item @code{log-directory} (default: @code{"/var/log/nginx"})
The directory to which NGinx will write log files.
+@item @code{log-level} (default: @code{'error}) (type: symbol)
+Logging level, which can be any of the following values: @code{'debug},
+@code{'info}, @code{'notice}, @code{'warn}, @code{'error}, @code{'crit},
+@code{'alert}, or @code{'emerg}.
+
@item @code{run-directory} (default: @code{"/var/run/nginx"})
The directory in which NGinx will create a pid file, and write temporary
files.
diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index d56e893527..4fe9c2d9ab 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -15,6 +15,7 @@
;;; Copyright © 2020 Oleg Pykhalov <go.wigust@gmail.com>
;;; Copyright © 2020, 2021 Alexandru-Sergiu Marton <brown121407@posteo.ro>
;;; Copyright © 2022 Simen Endsjø <simendsjo@gmail.com>
+;;; Copyright © 2023 Bruno Victal <mirai@makinata.eu>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -51,6 +52,8 @@ (define-module (gnu services web)
#:use-module (gnu packages logging)
#:use-module (gnu packages mail)
#:use-module (gnu packages rust-apps)
+ #:autoload (guix i18n) (G_)
+ #:use-module (guix diagnostics)
#:use-module (guix packages)
#:use-module (guix records)
#:use-module (guix modules)
@@ -61,6 +64,7 @@ (define-module (gnu services web)
#:use-module ((guix packages) #:select (package-version))
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-9)
+ #:use-module (srfi srfi-34)
#:use-module (ice-9 match)
#:use-module (ice-9 format)
#:export (httpd-configuration
@@ -96,6 +100,7 @@ (define-module (gnu services web)
nginx-configuration-nginx
nginx-configuration-shepherd-requirement
nginx-configuration-log-directory
+ nginx-configuration-log-level
nginx-configuration-run-directory
nginx-configuration-server-blocks
nginx-configuration-upstream-blocks
@@ -562,6 +567,9 @@ (define-record-type* <nginx-configuration>
(default '())) ;list of symbols
(log-directory nginx-configuration-log-directory ;string
(default "/var/log/nginx"))
+ (log-level nginx-configuration-log-level
+ (sanitize assert-valid-log-level)
+ (default 'error))
(run-directory nginx-configuration-run-directory ;string
(default "/var/run/nginx"))
(server-blocks nginx-configuration-server-blocks
@@ -584,6 +592,14 @@ (define-record-type* <nginx-configuration>
(file nginx-configuration-file ;#f | string | file-like
(default #f)))
+(define (assert-valid-log-level level)
+ "Ensure @var{level} is one of @code{'debug}, @code{'info}, @code{'notice},
+@code{'warn}, @code{'error}, @code{'crit}, @code{'alert}, or @code{'emerg}."
+ (unless (memq level '(debug info notice warn error crit alert emerg))
+ (raise
+ (formatted-message (G_ "unknown log level '~a'~%") level)))
+ level)
+
(define (config-domain-strings names)
"Return a string denoting the nginx config representation of NAMES, a list
of domain names."
@@ -692,6 +708,7 @@ (define (default-nginx-config config)
(match-record config
<nginx-configuration>
(nginx log-directory run-directory
+ log-level
server-blocks upstream-blocks
server-names-hash-bucket-size
server-names-hash-bucket-max-size
@@ -704,7 +721,7 @@ (define (default-nginx-config config)
(flatten
"user nginx nginx;\n"
"pid " run-directory "/pid;\n"
- "error_log " log-directory "/error.log info;\n"
+ "error_log " log-directory "/error.log " (symbol->string log-level) ";\n"
(map emit-load-module modules)
(map emit-global-directive global-directives)
"http {\n"
--
2.39.2
B
B
Bruno Victal wrote on 3 Apr 2023 13:58
[PATCH v2 2/2] services: nginx: Add reopen action.
(address . 62490@debbugs.gnu.org)
c4d06e358b17dc5fbd5b5607890d5621b59c33d7.1680523067.git.mirai@makinata.eu
This is required to allow log file rotations using rottlog, etc.

* gnu/services/web.scm (nginx-shepherd-service): Add reopen shepherd action.
---
gnu/services/web.scm | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

Toggle diff (19 lines)
diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index 4fe9c2d9ab..45897d7d6f 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -840,7 +840,11 @@ (define (nginx-shepherd-service config)
the same configuration file. It is useful for situations where the same nginx
configuration file can point to different things after a reload, such as
renewed TLS certificates, or @code{include}d files.")
- (procedure (nginx-action "-s" "reload"))))))))))
+ (procedure (nginx-action "-s" "reload")))
+ (shepherd-action
+ (name 'reopen)
+ (documentation "Re-open log files.")
+ (procedure (nginx-action "-s" "reopen"))))))))))
(define nginx-service-type
(service-type (name 'nginx)
--
2.39.2
M
M
Maxim Cournoyer wrote on 11 Apr 2023 19:12
Re: bug#62490: [PATCH] services: nginx: Make logging level configurable.
(name . Ludovic Courtès)(address . ludo@gnu.org)
87bkjumjgv.fsf_-_@gmail.com
Hi,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (30 lines)
> Hi,
>
> Bruno Victal <mirai@makinata.eu> skribis:
>
>> +(define-compile-time-procedure (assert-valid-log-level (level symbol?))
>> + "Ensure @var{level} is one of @code{'debug}, @code{'info}, @code{'notice},
>> +@code{'warn}, @code{'error}, @code{'crit}, @code{'alert}, or @code{'emerg}."
>
> As it turns out, ‘define-compile-time-procedure’ cannot work with
> symbols. In short, that’s because in the end the generated macro
> checks:
>
> (symbol? #'(quote debug))
>
> which doesn’t do what we want.
>
> Anyway, you can either make it a regular procedure instead, or use a
> trick found in R6RS and used in some places in Guix, Guile-Gcrypt, etc.,
> which is to define a macro that validates things:
>
> (endianness little) ;R6RS
>
> (operating-id valid-path?) ;(guix store), with ‘define-enumerate-type’
>
> Making it a procedure is prolly good enough. The compiler can optimize
> it out at compile time, FWIW:
>
> scheme@(guile-user)> ,optimize (unless (memq 'debug '(debug info)) (throw 'x))
> $13 = (if #f #f)

I've installed Bruno's v2, which reworked the above into a simple
procedure.

Closing!

--
Thanks,
Maxim
Closed
?