[bug#62490] services: nginx: Make logging level configurable.

* 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(-)

Hello,

Bruno Victal <mirai@makinata.eu> writes:

> * 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!

> ---
>
> 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?

>  (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.

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>.


Cheers,
Bruno

OK,

Bruno Victal <mirai@makinata.eu> writes:

> 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.

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:

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> ,optimize (unless (memq 'debug '(debug info)) (throw 'x))
$13 = (if #f #f)
--8<---------------cut here---------------end--------------->8---

Ludo’.

Hi,

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

> 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!