Message ID | fa44bc8a46021c3d307479ea2fbdf04d68e0cd10.1679942599.git.mirai@makinata.eu |
---|---|
State | New |
Headers | show |
Series | [bug#62490] services: nginx: Make logging level configurable. | expand |
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!
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"