Re: [PATCH] Make error logging modular
On 09/18/2013 02:12 PM, Jim Jagielski wrote: +1! On Sep 18, 2013, at 8:09 AM, Eric Covener cove...@gmail.com wrote: On Wed, Sep 18, 2013 at 8:01 AM, Jan Kaluža jkal...@redhat.com wrote: On 07/22/2013 08:02 AM, Jan Kaluza wrote: - Original Message - Hello Jan, Is there any reason we shouldn't do this in trunk? I don't see any reason. This patch was intended for trunk, but I don't have svn commit access, so I'm sending patches to this list :). It's also better that someone reviews my code, because I don't have so long experience with httpd development. If there's nobody against this change, I will commit the first two patches (+ documentation) to trunk in the end of the week. I think we should wait with mod_journald a bit until journald's performance gets better, but if you think it would be useful to have mod_journald in trunk too, let me know. +1, and I don't think mod_journald would hurt. Hi, I've updated mod_journald to latest trunk and added documentation. You can check the patch against trunk at http://people.apache.org/~jkaluza/patches/mod_journald/0001-mod_journald.patch. I'm going to commit it to trunk next week. Regards, Jan Kaluza
Re: [PATCH] Make error logging modular
Hi Jan,
On Fri, Jul 11, 2014 at 11:17 AM, Jan Kaluža jkal...@redhat.com wrote:
I've updated mod_journald to latest trunk and added documentation. You can
check the patch against trunk at
http://people.apache.org/~jkaluza/patches/mod_journald/0001-mod_journald.patch.
+static apr_pool_t *journald_info_get_pool(const ap_errorlog_info *info)
+{
+if (info-s info-s-process info-s-process-pool)
+return info-s-process-pool;
+if (info-pool)
+return info-pool;
+if (info-c info-c-pool)
+return info-c-pool;
+if (info-r info-r-pool)
+return info-r-pool;
+return 0;
+}
Shouldn't this be in the reverse order?
Re: [PATCH] Make error logging modular
On 07/11/2014 12:53 PM, Yann Ylavic wrote:
Hi Jan,
On Fri, Jul 11, 2014 at 11:17 AM, Jan Kaluža jkal...@redhat.com wrote:
I've updated mod_journald to latest trunk and added documentation. You can
check the patch against trunk at
http://people.apache.org/~jkaluza/patches/mod_journald/0001-mod_journald.patch.
+static apr_pool_t *journald_info_get_pool(const ap_errorlog_info *info)
+{
+if (info-s info-s-process info-s-process-pool)
+return info-s-process-pool;
+if (info-pool)
+return info-pool;
+if (info-c info-c-pool)
+return info-c-pool;
+if (info-r info-r-pool)
+return info-r-pool;
+return 0;
+}
Shouldn't this be in the reverse order?
Hm, I think my original intention here was to try as first the pools
which have the biggest chance to be available. Since I'm creating
subpool of the pool returned by this method, it should be OK to use even
s-process-pool. Maybe it's useless try to optimize things...
Jan Kaluza
Re: [PATCH] Make error logging modular
On Fri, Jul 11, 2014 at 1:07 PM, Jan Kaluža jkal...@redhat.com wrote:
On 07/11/2014 12:53 PM, Yann Ylavic wrote:
Hi Jan,
On Fri, Jul 11, 2014 at 11:17 AM, Jan Kaluža jkal...@redhat.com wrote:
I've updated mod_journald to latest trunk and added documentation. You
can
check the patch against trunk at
http://people.apache.org/~jkaluza/patches/mod_journald/0001-mod_journald.patch.
+static apr_pool_t *journald_info_get_pool(const ap_errorlog_info *info)
+{
+if (info-s info-s-process info-s-process-pool)
+return info-s-process-pool;
+if (info-pool)
+return info-pool;
+if (info-c info-c-pool)
+return info-c-pool;
+if (info-r info-r-pool)
+return info-r-pool;
+return 0;
+}
Shouldn't this be in the reverse order?
Hm, I think my original intention here was to try as first the pools which
have the biggest chance to be available. Since I'm creating subpool of the
pool returned by this method, it should be OK to use even s-process-pool.
Maybe it's useless try to optimize things...
When a subpool is created, a lock is acquired/released on the parent
pool, so maybe for threaded MPMs here we'd better not hold it process
wide for every log?
Re: [PATCH] Make error logging modular
On 07/11/2014 01:23 PM, Yann Ylavic wrote:
On Fri, Jul 11, 2014 at 1:07 PM, Jan Kaluža jkal...@redhat.com wrote:
On 07/11/2014 12:53 PM, Yann Ylavic wrote:
Hi Jan,
On Fri, Jul 11, 2014 at 11:17 AM, Jan Kaluža jkal...@redhat.com wrote:
I've updated mod_journald to latest trunk and added documentation. You
can
check the patch against trunk at
http://people.apache.org/~jkaluza/patches/mod_journald/0001-mod_journald.patch.
+static apr_pool_t *journald_info_get_pool(const ap_errorlog_info *info)
+{
+if (info-s info-s-process info-s-process-pool)
+return info-s-process-pool;
+if (info-pool)
+return info-pool;
+if (info-c info-c-pool)
+return info-c-pool;
+if (info-r info-r-pool)
+return info-r-pool;
+return 0;
+}
Shouldn't this be in the reverse order?
Hm, I think my original intention here was to try as first the pools which
have the biggest chance to be available. Since I'm creating subpool of the
pool returned by this method, it should be OK to use even s-process-pool.
Maybe it's useless try to optimize things...
When a subpool is created, a lock is acquired/released on the parent
pool, so maybe for threaded MPMs here we'd better not hold it process
wide for every log?
Didn't know that. I've updated the patch and the order is now reversed.
I'm thinking about not creating subpool in case when r-pool is
available, but I'm not sure if more complicated code is worth doing it
(not sure if the performance penalty of r-pool subpool creation is big
enough). I will do some measurement with CustomLog.
Re: [PATCH] Make error logging modular
On Fri, Jul 11, 2014 at 2:03 PM, Jan Kaluža jkal...@redhat.com wrote:
On 07/11/2014 01:23 PM, Yann Ylavic wrote:
On Fri, Jul 11, 2014 at 1:07 PM, Jan Kaluža jkal...@redhat.com wrote:
On 07/11/2014 12:53 PM, Yann Ylavic wrote:
Hi Jan,
On Fri, Jul 11, 2014 at 11:17 AM, Jan Kaluža jkal...@redhat.com wrote:
I've updated mod_journald to latest trunk and added documentation. You
can
check the patch against trunk at
http://people.apache.org/~jkaluza/patches/mod_journald/0001-mod_journald.patch.
+static apr_pool_t *journald_info_get_pool(const ap_errorlog_info *info)
+{
+if (info-s info-s-process info-s-process-pool)
+return info-s-process-pool;
+if (info-pool)
+return info-pool;
+if (info-c info-c-pool)
+return info-c-pool;
+if (info-r info-r-pool)
+return info-r-pool;
+return 0;
+}
Shouldn't this be in the reverse order?
Hm, I think my original intention here was to try as first the pools
which
have the biggest chance to be available. Since I'm creating subpool of
the
pool returned by this method, it should be OK to use even
s-process-pool.
Maybe it's useless try to optimize things...
When a subpool is created, a lock is acquired/released on the parent
pool, so maybe for threaded MPMs here we'd better not hold it process
wide for every log?
Didn't know that. I've updated the patch and the order is now reversed. I'm
thinking about not creating subpool in case when r-pool is available, but
I'm not sure if more complicated code is worth doing it (not sure if the
performance penalty of r-pool subpool creation is big enough). I will do
some measurement with CustomLog.
I don't think it's suitable to allocate all the request logs on its
pool (that can be huge), I prefer the current subpool implementation.
Re: [PATCH] Make error logging modular
On 09/18/2013 02:19 PM, Ivan Zhakov wrote: On Wed, Sep 18, 2013 at 4:01 PM, Jan Kaluža jkal...@redhat.com wrote: On 07/22/2013 08:02 AM, Jan Kaluza wrote: - Original Message - Hello Jan, Is there any reason we shouldn't do this in trunk? I don't see any reason. This patch was intended for trunk, but I don't have svn commit access, so I'm sending patches to this list :). It's also better that someone reviews my code, because I don't have so long experience with httpd development. If there's nobody against this change, I will commit the first two patches (+ documentation) to trunk in the end of the week. I think we should wait with mod_journald a bit until journald's performance gets better, but if you think it would be useful to have mod_journald in trunk too, let me know. It would be also nice to have option for log provider to declare whether multiline log messages are supported. It will be very use full for logging complex error message to Windows Event Log, like dav_log_err() does. Currently on Subversion error it writes three log messages to event log like: [[[ Provider encountered an error while streaming a REPORT response. [500, #0] A failure occurred while driving the update report editor [500, #620018] Error writing base64 data: APR does not understand this error code [500, #620018] ]]] That should be possible now. You just have to add another AP_ERRORLOG_PROVIDER flag for that. If you are going to code Windows Event log support, I think you can do it this way. But it will much more convenient to have only one event log entry logged with these three lines. Jan Kaluza
Re: [PATCH] Make error logging modular
- Original Message - Hello Jan, Is there any reason we shouldn't do this in trunk? I don't see any reason. This patch was intended for trunk, but I don't have svn commit access, so I'm sending patches to this list :). It's also better that someone reviews my code, because I don't have so long experience with httpd development. Regards, Jan Kaluza The patches and features seem generally correct to me with a cursory review. Thanks, Paul On Mon, May 27, 2013 at 3:23 AM, Jan Kaluža jkal...@redhat.com wrote: Hi, last week I was trying to write my own module to log error_log to systemd-journal [1] and I've found out that with the current error_log code, it's not possible to do that properly. I was able to use error_log hook, but there is no way to disable creation of ErrorLog file (One can set it to /dev/null, but httpd will still write data to it without any reason). Syslog logger fixes that by hardcoding syslog methods in log.c/core.c, but I don't think that's the right thing to do with journald. Therefore, I've created following patches: http://people.apache.org/~jkaluza/patches/logging/ Their descriptions should be clear from their names, but I will describe them briefly here too. Patch 0001 declares ap_errorlog_provider which can be implemented by module providing error_log logger. Admin can later define ErrorLog provider arg to choose particular errorlog provider. Old syntax still works and the change is backward compatible. This patch also removes syslog logging from log.c (it is moved to newly created mod_syslog.c in next patch) Patch 0002 creates mod_syslog.c which uses the new API to implement syslog logging. It works the same way as the version in log.c I removed in previous patch, but it's in separate module. Patch 0003 shows how mod_journald.c can use the existing API. This module works well with systemd-journal, but unfortunately the performance of systemd-journal daemon is poor so far [2], but I presume it will be fixed and the module will be usable for general use in the future. There is probably no real benefit in accepting this last patch right now. It's here to only show why the previous two patches are useful. Note that this is my first bigger patch touching httpd core, so feel free to correct my possible mistakes... :) [1] http://0pointer.de/blog/projects/journalctl.html [2] https://bugzilla.redhat.com/show_bug.cgi?id=963620 [patches] http://people.apache.org/~jkaluza/patches/logging/ Regards, Jan Kaluza
Re: [PATCH] Make error logging modular
Hello Jan, Is there any reason we shouldn't do this in trunk? The patches and features seem generally correct to me with a cursory review. Thanks, Paul On Mon, May 27, 2013 at 3:23 AM, Jan Kaluža jkal...@redhat.com wrote: Hi, last week I was trying to write my own module to log error_log to systemd-journal [1] and I've found out that with the current error_log code, it's not possible to do that properly. I was able to use error_log hook, but there is no way to disable creation of ErrorLog file (One can set it to /dev/null, but httpd will still write data to it without any reason). Syslog logger fixes that by hardcoding syslog methods in log.c/core.c, but I don't think that's the right thing to do with journald. Therefore, I've created following patches: http://people.apache.org/~jkaluza/patches/logging/ Their descriptions should be clear from their names, but I will describe them briefly here too. Patch 0001 declares ap_errorlog_provider which can be implemented by module providing error_log logger. Admin can later define ErrorLog provider arg to choose particular errorlog provider. Old syntax still works and the change is backward compatible. This patch also removes syslog logging from log.c (it is moved to newly created mod_syslog.c in next patch) Patch 0002 creates mod_syslog.c which uses the new API to implement syslog logging. It works the same way as the version in log.c I removed in previous patch, but it's in separate module. Patch 0003 shows how mod_journald.c can use the existing API. This module works well with systemd-journal, but unfortunately the performance of systemd-journal daemon is poor so far [2], but I presume it will be fixed and the module will be usable for general use in the future. There is probably no real benefit in accepting this last patch right now. It's here to only show why the previous two patches are useful. Note that this is my first bigger patch touching httpd core, so feel free to correct my possible mistakes... :) [1] http://0pointer.de/blog/projects/journalctl.html [2] https://bugzilla.redhat.com/show_bug.cgi?id=963620 [patches] http://people.apache.org/~jkaluza/patches/logging/ Regards, Jan Kaluza
[PATCH] Make error logging modular
Hi, last week I was trying to write my own module to log error_log to systemd-journal [1] and I've found out that with the current error_log code, it's not possible to do that properly. I was able to use error_log hook, but there is no way to disable creation of ErrorLog file (One can set it to /dev/null, but httpd will still write data to it without any reason). Syslog logger fixes that by hardcoding syslog methods in log.c/core.c, but I don't think that's the right thing to do with journald. Therefore, I've created following patches: http://people.apache.org/~jkaluza/patches/logging/ Their descriptions should be clear from their names, but I will describe them briefly here too. Patch 0001 declares ap_errorlog_provider which can be implemented by module providing error_log logger. Admin can later define ErrorLog provider arg to choose particular errorlog provider. Old syntax still works and the change is backward compatible. This patch also removes syslog logging from log.c (it is moved to newly created mod_syslog.c in next patch) Patch 0002 creates mod_syslog.c which uses the new API to implement syslog logging. It works the same way as the version in log.c I removed in previous patch, but it's in separate module. Patch 0003 shows how mod_journald.c can use the existing API. This module works well with systemd-journal, but unfortunately the performance of systemd-journal daemon is poor so far [2], but I presume it will be fixed and the module will be usable for general use in the future. There is probably no real benefit in accepting this last patch right now. It's here to only show why the previous two patches are useful. Note that this is my first bigger patch touching httpd core, so feel free to correct my possible mistakes... :) [1] http://0pointer.de/blog/projects/journalctl.html [2] https://bugzilla.redhat.com/show_bug.cgi?id=963620 [patches] http://people.apache.org/~jkaluza/patches/logging/ Regards, Jan Kaluza
