You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Joe Orton <jo...@redhat.com> on 2010/06/08 10:21:02 UTC
Re: svn commit: r952201 - /httpd/httpd/trunk/server/log.c
On Mon, Jun 07, 2010 at 12:23:26PM -0000, rjung@apache.org wrote:
> Author: rjung
> Date: Mon Jun 7 12:23:26 2010
> New Revision: 952201
>
> URL: http://svn.apache.org/viewvc?rev=952201&view=rev
> Log:
> Add process id and thread id (if APR has thread support)
> to the error log.
...
> @@ -620,6 +621,18 @@ static void log_error_core(const char *f
> if ((level & APLOG_STARTUP) != APLOG_STARTUP) {
> len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
> "[%s] ", priorities[level_and_mask].t_name);
> +
> + len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
> + "[%" APR_PID_T_FMT, getpid());
> +#if APR_HAS_THREADS
> + {
> + apr_os_thread_t tid = apr_os_thread_current();
> + len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
> + ":%pT", &tid);
> + }
> +#endif
> + errstr[len++] = ']';
> + errstr[len++] = ' ';
These numbers made me double-take when reading error_log since they
don't have a descriptive prefix. Also the tid is not much use in a
non-threaded server. Could we do something like this? (Hopefully this
function won't start showing up in CPU profiles!)
Index: server/log.c
===================================================================
--- server/log.c (revision 952555)
+++ server/log.c (working copy)
@@ -628,12 +628,17 @@
"[%s] ", priorities[level_and_mask].t_name);
len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
- "[%" APR_PID_T_FMT, getpid());
+ "[pid %" APR_PID_T_FMT, getpid());
#if APR_HAS_THREADS
{
- apr_os_thread_t tid = apr_os_thread_current();
- len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
- ":%pT", &tid);
+ int result;
+
+ if (ap_mpm_query(AP_MPMQ_IS_THREADED, &result) == 0
+ && result == 1) {
+ apr_os_thread_t tid = apr_os_thread_current();
+ len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
+ ":tid %pT", &tid);
+ }
}
#endif
errstr[len++] = ']';
RE: svn commit: r952201 - /httpd/httpd/trunk/server/log.c
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Rainer Jung
> Sent: Dienstag, 8. Juni 2010 18:19
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r952201 - /httpd/httpd/trunk/server/log.c
>
> >
> > How about a local static variable in the function?
>
> I tried it. But:
>
> you can actually switch between MPMs now by apachectl graceful or
> restart when building them dynamically. The static in the
> children comes
> via fork from the parent process. The naive way of initializing it to
> UNSET and then setting it once to the result of the MPM query doesn't
> work, because it will miss any changes due to restart.
Good point. I missed that a graceful restart now can change the MPM.
> If we keep the variable local in the log function, then it
> would have to
> check whenever it logs whether a restart has happened. I
> assume querying
> the MPM isn't more expensive.
Agreed.
Regards
Rüdiger
Re: svn commit: r952201 - /httpd/httpd/trunk/server/log.c
Posted by Rainer Jung <ra...@kippdata.de>.
On 08.06.2010 12:11, "Plüm, Rüdiger, VF-Group" wrote:
>
>
>> -----Original Message-----
>> From: Rainer Jung
>> Sent: Dienstag, 8. Juni 2010 11:14
>> To: dev@httpd.apache.org
>> Subject: Re: svn commit: r952201 - /httpd/httpd/trunk/server/log.c
>>
>> On 08.06.2010 10:21, Joe Orton wrote:
>>> On Mon, Jun 07, 2010 at 12:23:26PM -0000, rjung@apache.org wrote:
>>>> Author: rjung
>>>> Date: Mon Jun 7 12:23:26 2010
>>>> New Revision: 952201
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=952201&view=rev
>>>> Log:
>>>> Add process id and thread id (if APR has thread support)
>>>> to the error log.
>>> ...
>>>> @@ -620,6 +621,18 @@ static void log_error_core(const char *f
>>>> if ((level& APLOG_STARTUP) != APLOG_STARTUP) {
>>>> len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
>>>> "[%s] ",
>> priorities[level_and_mask].t_name);
>>>> +
>>>> + len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
>>>> + "[%" APR_PID_T_FMT, getpid());
>>>> +#if APR_HAS_THREADS
>>>> + {
>>>> + apr_os_thread_t tid = apr_os_thread_current();
>>>> + len += apr_snprintf(errstr + len,
>> MAX_STRING_LEN - len,
>>>> + ":%pT",&tid);
>>>> + }
>>>> +#endif
>>>> + errstr[len++] = ']';
>>>> + errstr[len++] = ' ';
>>>
>>> These numbers made me double-take when reading error_log since they
>>> don't have a descriptive prefix. Also the tid is not much use in a
>>> non-threaded server. Could we do something like this?
>> (Hopefully this
>>> function won't start showing up in CPU profiles!)
>>
>> I'm neutral on adding the descriptive prefixes (+0). It's
>> helpful to get
>> acquainted with the new format, on the long term it might become
>> unnecessary. I guess when adding new standard tokens to the
>> error log we
>> might end up doing a callback based configurable format like in
>> mod_log_config.
>>
>> Removing the tid for non-threaded servers is fine. I thought
>> about that
>> too. Would it make sense to somehow cache the result of
>> ap_mpm_query(AP_MPMQ_IS_THREADED,&result) in one of the structs
>> available from the log function, because it won't change during the
>> runtime of the instance (at least not during the runtime of
>> the child;
>> don't know whether it would be feasible to switch the new dynamically
>> loaded MPMs during restart, but that's another topic).
>>
>
> How about a local static variable in the function?
I tried it. But:
you can actually switch between MPMs now by apachectl graceful or
restart when building them dynamically. The static in the children comes
via fork from the parent process. The naive way of initializing it to
UNSET and then setting it once to the result of the MPM query doesn't
work, because it will miss any changes due to restart.
If we keep the variable local in the log function, then it would have to
check whenever it logs whether a restart has happened. I assume querying
the MPM isn't more expensive.
Rainer
RE: svn commit: r952201 - /httpd/httpd/trunk/server/log.c
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Rainer Jung
> Sent: Dienstag, 8. Juni 2010 11:14
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r952201 - /httpd/httpd/trunk/server/log.c
>
> On 08.06.2010 10:21, Joe Orton wrote:
> > On Mon, Jun 07, 2010 at 12:23:26PM -0000, rjung@apache.org wrote:
> >> Author: rjung
> >> Date: Mon Jun 7 12:23:26 2010
> >> New Revision: 952201
> >>
> >> URL: http://svn.apache.org/viewvc?rev=952201&view=rev
> >> Log:
> >> Add process id and thread id (if APR has thread support)
> >> to the error log.
> > ...
> >> @@ -620,6 +621,18 @@ static void log_error_core(const char *f
> >> if ((level& APLOG_STARTUP) != APLOG_STARTUP) {
> >> len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
> >> "[%s] ",
> priorities[level_and_mask].t_name);
> >> +
> >> + len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
> >> + "[%" APR_PID_T_FMT, getpid());
> >> +#if APR_HAS_THREADS
> >> + {
> >> + apr_os_thread_t tid = apr_os_thread_current();
> >> + len += apr_snprintf(errstr + len,
> MAX_STRING_LEN - len,
> >> + ":%pT",&tid);
> >> + }
> >> +#endif
> >> + errstr[len++] = ']';
> >> + errstr[len++] = ' ';
> >
> > These numbers made me double-take when reading error_log since they
> > don't have a descriptive prefix. Also the tid is not much use in a
> > non-threaded server. Could we do something like this?
> (Hopefully this
> > function won't start showing up in CPU profiles!)
>
> I'm neutral on adding the descriptive prefixes (+0). It's
> helpful to get
> acquainted with the new format, on the long term it might become
> unnecessary. I guess when adding new standard tokens to the
> error log we
> might end up doing a callback based configurable format like in
> mod_log_config.
>
> Removing the tid for non-threaded servers is fine. I thought
> about that
> too. Would it make sense to somehow cache the result of
> ap_mpm_query(AP_MPMQ_IS_THREADED,&result) in one of the structs
> available from the log function, because it won't change during the
> runtime of the instance (at least not during the runtime of
> the child;
> don't know whether it would be feasible to switch the new dynamically
> loaded MPMs during restart, but that's another topic).
>
How about a local static variable in the function?
Regards
Rüdiger
Re: svn commit: r952201 - /httpd/httpd/trunk/server/log.c
Posted by Rainer Jung <ra...@kippdata.de>.
On 08.06.2010 10:21, Joe Orton wrote:
> On Mon, Jun 07, 2010 at 12:23:26PM -0000, rjung@apache.org wrote:
>> Author: rjung
>> Date: Mon Jun 7 12:23:26 2010
>> New Revision: 952201
>>
>> URL: http://svn.apache.org/viewvc?rev=952201&view=rev
>> Log:
>> Add process id and thread id (if APR has thread support)
>> to the error log.
> ...
>> @@ -620,6 +621,18 @@ static void log_error_core(const char *f
>> if ((level& APLOG_STARTUP) != APLOG_STARTUP) {
>> len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
>> "[%s] ", priorities[level_and_mask].t_name);
>> +
>> + len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
>> + "[%" APR_PID_T_FMT, getpid());
>> +#if APR_HAS_THREADS
>> + {
>> + apr_os_thread_t tid = apr_os_thread_current();
>> + len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
>> + ":%pT",&tid);
>> + }
>> +#endif
>> + errstr[len++] = ']';
>> + errstr[len++] = ' ';
>
> These numbers made me double-take when reading error_log since they
> don't have a descriptive prefix. Also the tid is not much use in a
> non-threaded server. Could we do something like this? (Hopefully this
> function won't start showing up in CPU profiles!)
I'm neutral on adding the descriptive prefixes (+0). It's helpful to get
acquainted with the new format, on the long term it might become
unnecessary. I guess when adding new standard tokens to the error log we
might end up doing a callback based configurable format like in
mod_log_config.
Removing the tid for non-threaded servers is fine. I thought about that
too. Would it make sense to somehow cache the result of
ap_mpm_query(AP_MPMQ_IS_THREADED,&result) in one of the structs
available from the log function, because it won't change during the
runtime of the instance (at least not during the runtime of the child;
don't know whether it would be feasible to switch the new dynamically
loaded MPMs during restart, but that's another topic).
> Index: server/log.c
> ===================================================================
> --- server/log.c (revision 952555)
> +++ server/log.c (working copy)
> @@ -628,12 +628,17 @@
> "[%s] ", priorities[level_and_mask].t_name);
>
> len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
> - "[%" APR_PID_T_FMT, getpid());
> + "[pid %" APR_PID_T_FMT, getpid());
> #if APR_HAS_THREADS
> {
> - apr_os_thread_t tid = apr_os_thread_current();
> - len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
> - ":%pT",&tid);
> + int result;
> +
> + if (ap_mpm_query(AP_MPMQ_IS_THREADED,&result) == 0
> +&& result == 1) {
> + apr_os_thread_t tid = apr_os_thread_current();
> + len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
> + ":tid %pT",&tid);
> + }
> }
> #endif
> errstr[len++] = ']';
Thanks for reviewing!
Rainer
Re: svn commit: r952201 - /httpd/httpd/trunk/server/log.c
Posted by Rainer Jung <ra...@kippdata.de>.
On 08.06.2010 10:21, Joe Orton wrote:
> On Mon, Jun 07, 2010 at 12:23:26PM -0000, rjung@apache.org wrote:
> These numbers made me double-take when reading error_log since they
> don't have a descriptive prefix. Also the tid is not much use in a
> non-threaded server. Could we do something like this? (Hopefully this
> function won't start showing up in CPU profiles!)
Committed as r952724 with the minmal change:
> +&& result == 1) {
1 => AP_MPMQ_STATIC
Thanks
Rainer