format_log inconsistency

[pschichtel]

the ::nginx class takes the access log as "<target> <format>" via the http_access_log parameter, while ::nginx::resource::vhost has 2 parameters, one for the log target (access_log) and one for the format (format_log, defaulted to combined).

Is there a reason why it is done in this way?

c849c2cb7d471d2e294b2807fb527a90d90ea594 recently introduced the log level option for the error log, but the http_access_log still takes both settings in one string.

[pschichtel]

vhosts also don't support specifing the error log level

[wyardley]

FWIW, I added #888 which improves things in certain ways, but may actually make this worse in other ways. I agree that the interface could be more consistent. Once the next forge release comes out, it should have these changes in it.

#888 did add a new parameter, $http_format_log, which makes the behavior somewhat more consistent. It does not yet support setting the log level for vhost, and you can't set different formatting options for multiple logs of the same type. Generally, you can make things work (also by using the custom appends / prepends), but there is definitely room for improvement.

This module has a lot of cruft, and has been maintained by a few different people / organizations, and has also had a lot of changes from third party PRs, so that's probably why the interface is a bit inconsistent at times.

[kwisatz]

What about making the access_log and error_log parameters be a hash instead of an array?

So instead of having:

access_log:
      - 'syslog:server=graylog.somewhere.tld:12301'
      - '/var/log/nginx/somesite.access.log'

We could have:

access_log:
      'syslog:server=graylog.somewhere.tld:12301': graylog2_format
      '/var/log/nginx/somesite.access.log': combined

[wyardley]

@kwisatz: Yes, I think that's the cleanest way to make it work as it should (and to make all the logging statements consistent), only downsides are a) would take some work to switch everywhere, and b) not backwards compatible. This is along the lines of what I was planning to do at some point, but haven't had the time / energy. If you'd be willing to submit a PR for such a change, I'd be happy to review / merge it.

Alternately, could be an array and require that the format be included if specified, but I think hash is probably best.

[kwisatz]

I will give it a spin if I find the time. But don't let that stop someone from giving it a try ;)

[kwisatz]

While we're at it, I think we should also change how default logs are set up. I'm speaking about the elsif not @error_log part which assumes that the default log is to be a file log.

<% if @error_log.is_a?(Array) -%>
  <%- @error_log.each do |log_item| -%>
  error_log             <%= log_item %>;
  <%- end -%>
<% elsif @error_log == 'absent' -%>
<% elsif not @error_log -%>
  error_log             <%= scope['::nginx::config::log_dir'] %>/<%= @name_sanitized %>.error.log;
<% else -%>
  error_log             <%= @error_log %>;
<% end -%>
<% if @error_pages -%>

This means that each server directive which should log to something else (e.g. syslog) will require at least one explicit access_log and error_log statement.

An improvement over this would be the possibility to define a config-wide default pattern. I guess that would require replacing the above template with a lib/puppet_x function evaluating the value of such a parameter. (Or maybe I'm missing an easier approach)

[pschichtel]

#1452 seems to have done what I asked for, right?