Move logging configuration to be per-domain

[kobim]

Type of Change

• Build Scripts: -
• Tool Source: Vue: configuration, migration, JS: output script.
• Something else: -

What issue does this relate to?

fixes #354 by giving more control of the access_log and error_log directives.

What should this PR do?

• Change the defaults around logging - enable per-domain logging instead of global logging
• Add configurable arguments parameter (e.g., buffer=512k flush=1m) to the access_log directive.
• Move the log level inside the error_log path to a new radio buttons group with valid options.

What are the acceptance criteria?

Global Configuration -> Logging

BEFORE

AFTER

Domain Configuration -> Logging

BEFORE

AFTER

Values

Case	http/domain block level	Log	Value	Notes
Default	http	access_log	off
		error_log	/dev/null	has no off option
	domain	access_log	/var/log/nginx/access.log buffer=512k flush=1m
		error_log	/var/log/nginx/error.log warn
	domain redirects	access_log	turned off
		error_log	turned off
Compatability (SOME existing configuration exists)	http	access_log	/var/log/nginx/access.log
		error_log	/var/log/nginx/error.log
	domain	access_log	if per-domain logging was enabled - /var/log/nginx/<domain>.access.log else - /var/log/nginx/access.log (or whatever was the global accessLog value)	In any case there will be a value for access_log
		error_log	if per-domain logging was enabled - /var/log/nginx/<domain>.error.log warn else - /var/log/nginx/error.log warn (or whatever was the global errorLog value)	In any case there will be a value for error_log
	domain redirects	access_log	turned off
		error_log	turned off

Outputs

• Default access_log and error_log should be disabled in the global (http) scope.

[kobim]

You're right @MattIPv4, I think I made some mess with the chosen implementation.

550aec4 introduces more refined controls in each section (global/domain-specific) together with sane defaults and options. I have also updated the PR description to reflect those changes, and I hope these make more sense to users. Could you please take a look and let me know what you think?

[kobim]

By default, per-domain access logging should now be enabled (as this replaces the old global access logging that was default), and by default it should use the /var/log/nginx/access.log path.

are you sure the default should be a single file? I would think that if someone brings an existing configuration (does existing mean "some attribute, even if not logging" in the URL?), we would want to retain the single file. But for any new configuration have a single file per domain?

It sounds confusing that the only way to get a "per-domain file" will be to have a previous configuration with explicit domains.<id>.logging.accessLog=true.

[MattIPv4]

are you sure the default should be a single file? I would think that if someone brings an existing configuration (does existing mean "some attribute, even if not logging" in the URL?), we would want to retain the single file. But for any new configuration have a single file per domain?

I was suggesting this with the aim of having the backwards compatibility logic be simpler, but if you'd rather only switch to a single file when an existing configuration is detected (which yeah, would be any query param essentially [though you'd want to ignore things like language]), you're welcome to do that.

It sounds confusing that the only way to get a "per-domain file" will be to have a previous configuration with explicit domains.<id>.logging.accessLog=true.

That wouldn't be the case? You could still set the file names yourself to be whatever you want, including per-domain?

[kobim]

to summarize the defaults and backward compatibility (making sure I understand everything):

Case	http/domain block level	Log	Value	Notes
Default	http	access_log	off
		error_log	/dev/null	has no off option
	domain	access_log	/var/log/nginx/<domain>.access.log buffer=512k flush=1m
		error_log	/var/log/nginx/<domain>.error.log warn
	domain redirects	access_log	turned off
		error_log	turned off
Compatability (SOME existing configuration exists - see question (1))	http	access_log	/var/log/nginx/access.log	See question (2)
		error_log	/var/log/nginx/error.log
	domain	access_log	if per-domain logging was enabled - /var/log/nginx/<domain>.access.log else - /var/log/nginx/access.log (or whatever was the global accessLog value)	In any case there will be a value for access_log
		error_log	if per-domain logging was enabled - /var/log/nginx/<domain>.error.log warn else - /var/log/nginx/error.log warn (or whatever was the global errorLog value)	In any case there will be a value for error_log
	domain redirects	access_log	turned off
		error_log	turned off

Open Questions

1. Should any existing configuration require backward compatibility fixing? because global.logging.accessLog and global.logging.errorLog defaulted to on, we can't understand when we are using an old configuration (where we should set a single logging file for all the domains) and a new configuration (where we should set per-domain file for each domain). This can also be asked - could there be a new default case where only per-domain logging is enabled and defaults to a file per domain name?
2. When using an existing configuration where the access_log is turned on. Should we add the default parameters (buffer=512k flush=1m) if no parameters are found? If access_log=/var/log/nginx/my_access.log buffer=1mb flush=6m we will consider buffer=1m flush=6m as the parameters, but if only a filename is found we will append the default parameters.

[MattIPv4]

👀 access_log should never be enabled at the http level, it should always be off there.

1. Yes, any existing configuration should be backwards compatible so that the same logging occurs as it would've before. As I noted in my original message, I suspect you may just need to apply this to all configurations, as I doubt it will possible to detect old vs. new.

2. That seems like it'd be a nice extra for the compatibility :)

[kobim]

👀 access_log should never be enabled at the http level, it should always be off there.

👍

1. Yes, any existing configuration should be backwards compatible so that the same logging occurs as it would've before. As I noted in my original message, I suspect you may just need to apply this to all configurations, as I doubt it will possible to detect old vs. new.

I'm thinking out loud - will there never be a default that will allow per-domain files? Do we expect users to explicitly write the domain name in the filename if they want a per-domain file? it might be cumbersome to change the filename for each domain, only because the backward compatibility forces us to use a single file for everything as the default.

2. That seems like it'd be a nice extra for the compatibility :)

💪

[MattIPv4]

will there never be a default that will allow per-domain files?

If under the old settings the user had that enabled, then yes, we'll want to compute filenames for that.

Do we expect users to explicitly write the domain name in the filename if they want a per-domain file?

Yes. This gives them far more flexibility in customising their logging than the logic before which was just "this global file", or "this pre-templated name you can't change".

[MattIPv4]

Sorry for the delay in getting back to you -- left a few comments above, and then I think this should be good to go 💙

[SorinGFS]

LGTM, tyvm for all the work on this

Excelent work indeed. But I think is not done just yet. When user enables access_log or errorr_log for a domain the default is /var/log/nginx/access.log and /var/log/nginx/error.log respectivelly. It should be /var/log/nginx/example.com.access.log and /var/log/nginx/example.com.error.log (variable taken from main server block server_name). Of course, user has the option to update it manually, but I think you agreed with the first version.

[MattIPv4]

That cannot be the default, for it breaks backwards compatibility :)

(though the backwards compatibility logic will inject the domain if the old per-domain logging flag is detected)

[SorinGFS]

Ok. It's a big progress anyway.

Now, I think next logical step is to add custom log formats :smile:

[MattIPv4]

Feel free to open a PR 👍

[SorinGFS]

Ok, I don't know vue to open a PR... but I can open an issue when I will find some time to think about it.

[SorinGFS]

That cannot be the default, for it breaks backwards compatibility :)

Would this be bacwards compatible?

[MattIPv4]

I'm not sure I'm a fan of adding a checkbox that changes the text value of a user-editable field, that will result in very confusing UX. The user is free to add the domain to the text field already if they desire.

[SorinGFS]

The user is free to add the domain to the text field already if they desire.

You're right, but this way user doesn't need to remember the domain name is working on, the value is taken from varialble. The user editable field can be greyed out if checkbox is checked.

[MattIPv4]

Sure, interpolating it from a variable is a convenience, but I don't think that outweighs the confusing UX of having a textbox that the user can customise sometimes, but then if you enable this it locks the value and injects the domain arbitrarily (the user doesn't seem to have control over where it gets injected, what happens if they had a custom path etc.).

I think the best path is to allow the user to customise the text field as they see fit :)

[SorinGFS]

Ok.

[SorinGFS]

the user doesn't seem to have control over where it gets injected, what happens if they had a custom path etc.

I don't want tio insist, just to be clear: I don't mean to let user to customize the already customized text field, it would be like this:

• option 1 (default) = var/log/nginx/access.log
• option 2 (checkbox checked, textbox greyed out) = /var/log/nginx/example.com.access.log
• option 3 = whatever user customizes the textbox.

[SorinGFS]

what happens if they had a custom path

As for this part I think the path can have it's own value in NGINX tab right bellow NGINX config directory, so user would be able to set it globally.

[MattIPv4]

Again, I think that will create incredibly confusing UX -- sometimes the user can set the path to whatever they want at the domain level, but then if they check this box the path is a fixed value, or customised based on a completely different text box at the global level.

[SorinGFS]

Independently of what we talk I think this would be a good idea:

[MattIPv4]

Could I please suggest you open an issue if you have an idea for a feature improvement for the tool, rather than posting on a now-merged PR?

[SorinGFS]

Yes, sure, sorry.