Merge lp:~credativ/openobject-server/trunk-logging-config into lp:openobject-server

[mistotebe]

The server.conf file does not allow the user to fully customize server logging configuration, e.g. it was impossible to set up syslog with a different facility or even UDP to a different host.

Proposing with changes as proposed during the review and discussed on OpenDays:
- keeping the config file separate
- not interfering with the current means of logging configuration unless the user decides to

[Martin Trigaux (OpenERP) (mat-openerp)]

Oh I guess you wanted to merge into 7, not trunk (lp:openobject-server/7.0 and not lp:openobject-server).
I reject then this merge propsal so you can resubmit it in the correct branch

[mistotebe (mistotebe)]

On 03/06/13 10:18, Martin Trigaux (OpenERP) wrote:
> Oh I guess you wanted to merge into 7, not trunk (lp:openobject-server/7.0 and not lp:openobject-server).
> I reject then this merge propsal so you can resubmit it in the correct branch

Actually, I wanted this to land in both 7.0 and trunk (so that it's not
forgotten in the next version), but I must have specified the wrong one
for this (and I guess they have diverged so that there's a load of
unrelated changes.

This request has been superseded by
https://code.launchpad.net/~credativ/openobject-server/7.0-fixes-syslog/+merge/166994

The way you process merge requests, should I have requested for trunk
first, then a backport to 7.0 or what is the way to get this kind of
code merged to both stable and next?

Thanks,
Ondrej

[Martin Trigaux (OpenERP) (mat-openerp)]

Ok my bad, I hadn't seen that you had made other merge proposal afterward, please discard the previous comments.

So first of all thanks for your merge proposal.
However this is a wishlist for a new feature and we can not accept new features in the stable version (even low risk like this one). This should definitely be made in trunk version.
Could you port your code to trunk ?

Regarding the feature it self, I don't see any reason why not doing so.
Regarding the code, why put a default value to user while it's already LOG_USER in SysLogHandler call ?
Except that looks good to me.

[Martin Trigaux (OpenERP) (mat-openerp)]

And to reply to your previous comment, in stable version we only do bug fixes, everything new is for trunk.
After we have a forward porting procedure. So if you have a bug introduced in 7.0, you fix it in a 7.0 branch and we will ported to trunk afterward.
The only time it is usefull to do two merge proposal 7+trunk, is when the fix is different (eg: monkey patching in 7 to avoid needing to restarting the db, clean fix in trunk).

[mistotebe (mistotebe)]

Thanks for the review and explanation.

The code now sets the facility unconditionaly so the SysLogHandler's default does not apply. Therefore the default should be preserved by the code somehow.

So it assumes the option is present as it's the simplest thing to do and has a default value for it. It could check instead whether the option was present and use a default in the code, but that does not seem it would anything to make it cleaner nor clearer.

I've now rebased it against trunk, updated the merge request and will keep our 7.0 deployments patched locally, then.

[Martin Trigaux (OpenERP) (mat-openerp)]

Hi again.

I have been speaking with some of our experts about this addition and we have some concerns about this. The problem is that we try to avoid adding unnecessary manual parameters. In case of the logging, your request is valid but as would be any other with the logging parameters.

A better and more scalable solution would be to use a configuration file. The python logging.config (http://docs.python.org/2/library/logging.config.html) would be the right way to do that.
I believe this would cover more use-cases without adding complexity.

Is this ok for you ?

[mistotebe (mistotebe)]

I've now looked into what needs doing to use logging.config and see two ways it can be integrated:
1. Use a logging configuration file, but load it with disable_existing_loggers=False, to preserve the existing configuration.
2. same as 1. but use the serverrc as the configuration file (loaded only when the "loggers", "handlers" and "formatters" sections are present)

Unless I missed something, the latter, while appealing, might be hard to achieve. There seems to be no API in logging.config to retrieve the running configuration back. The misc dictionary in the openerp.tools.config module might hold enough information to recreate it, though.

Which one seems more acceptable? Would you be OK with an extra configuration file if maintaining the information in the serverrc proved impossible?

(There is the assumption that you wanted to keep the (then obsolete) style of configuration for a while, before completely getting rid of it)

[mistotebe (mistotebe)]

I have updated the request with code as discussed on OpenDays, do you think it's in line with what you want it to be like?

It's been posted a while ago, so I'm happy to rebase it on top of the current trunk later.

Unmerged revisions

4943. By mistotebe on 2013-09-03

[IMP] Add an option to fully customize python logging

The server.conf file does not allow the user to fully customize server logging
configuration, e.g. it was impossible to set up syslog with a different
facility or even UDP to a different host.

=== modified file 'openerp/netsvc.py'
--- openerp/netsvc.py	2013-05-28 10:27:33 +0000
+++ openerp/netsvc.py	2013-09-03 14:13:12 +0000
@@ -21,6 +21,7 @@
 
 
 import logging
+import logging.config
 import logging.handlers
 import os
 import release
@@ -156,6 +157,9 @@
     for logconfig_item in logging_configurations:
         _logger.debug('logger level set: "%s"', logconfig_item)
 
+    if tools.config['log_config']:
+        logging.config.fileConfig(tools.config['log_config'], disable_existing_loggers=tools.config['log_config_only'])
+
 DEFAULT_LOG_CONFIGURATION = [
     'openerp.workflow.workitem:WARNING',
     'openerp.netsvc.rpc.request:INFO',

=== modified file 'openerp/tools/config.py'
--- openerp/tools/config.py	2013-03-28 09:50:06 +0000
+++ openerp/tools/config.py	2013-09-03 14:13:12 +0000
@@ -169,6 +169,8 @@
 
         # Logging Group
         group = optparse.OptionGroup(parser, "Logging Configuration")
+        group.add_option("--log-config", dest="log_config", help="file with logging configuration")
+        group.add_option("--log-config-only", dest="log_config_only", my_default=False, help="file with logging configuration")
         group.add_option("--logfile", dest="logfile", help="file where the server log will be stored")
         group.add_option("--logrotate", dest="logrotate", action="store_true", my_default=False, help="enable logfile rotation")
         group.add_option("--syslog", action="store_true", dest="syslog", my_default=False, help="Send the log to the syslog server")
@@ -375,7 +377,8 @@
                 'xmlrpc', 'syslog', 'without_demo', 'timezone',
                 'xmlrpcs_interface', 'xmlrpcs_port', 'xmlrpcs',
                 'static_http_enable', 'static_http_document_root', 'static_http_url_prefix',
-                'secure_cert_file', 'secure_pkey_file', 'dbfilter', 'log_handler', 'log_level'
+                'secure_cert_file', 'secure_pkey_file', 'dbfilter', 'log_handler', 'log_level',
+                'log_config', 'log_config_only', 
                 ]
 
         for arg in keys: