You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by rj...@apache.org on 2010/06/08 18:26:40 UTC
svn commit: r952724 - /httpd/httpd/trunk/server/log.c
Author: rjung
Date: Tue Jun 8 16:26:39 2010
New Revision: 952724
URL: http://svn.apache.org/viewvc?rev=952724&view=rev
Log:
Add descriptive prefix to pid and tid in the error log.
Only log the tid, if the MPM is threaded.
Suggested by jorton.
Modified:
httpd/httpd/trunk/server/log.c
Modified: httpd/httpd/trunk/server/log.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/log.c?rev=952724&r1=952723&r2=952724&view=diff
==============================================================================
--- httpd/httpd/trunk/server/log.c (original)
+++ httpd/httpd/trunk/server/log.c Tue Jun 8 16:26:39 2010
@@ -628,12 +628,17 @@ static void log_error_core(const char *f
"[%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 == AP_MPMQ_STATIC) {
+ 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: r952724 - /httpd/httpd/trunk/server/log.c
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Tuesday 08 June 2010, Joe Orton wrote:
> I am concerned about how verbose the error_log output is now
> though. How about restricting this whole pid/tid logging block
> to
>
> if (level_and_mask >= APLOG_DEBUG) {
I am concerned about that, too, and therefore would like to make the
behaviour configurable. But I am not sure that making this depend on
the log level is a good idea. If there are several error messages in
the log at roughly the same time, the admin may want to know if they
are related or come from different requests/threads.
Let's move the discussion to the 'Enhanced error log format for trunk'
thread, where it belongs.
Re: svn commit: r952724 - /httpd/httpd/trunk/server/log.c
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Jun 08, 2010 at 02:56:46PM -0500, William Rowe wrote:
> > URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/log.c?rev=952724&r1=952723&r2=952724&view=diff
> > ==============================================================================
> > --- httpd/httpd/trunk/server/log.c (original)
> > +++ httpd/httpd/trunk/server/log.c Tue Jun 8 16:26:39 2010
...
> > + if (ap_mpm_query(AP_MPMQ_IS_THREADED, &result) == 0
> > + && result == AP_MPMQ_STATIC) {
> > + apr_os_thread_t tid = apr_os_thread_current();
> > + len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
> > + ":tid %pT", &tid);
> > + }
> > }
> > #endif
> > errstr[len++] = ']';
>
> Question; where is the best place to perform this query once; and then to set
> aside the ap_mpm_query result as a static? Hitting an mpm_query on each and
> every log entry isn't exactly efficient.
Looking at this again, I'm not sure why using ap_mpm_query() is going to
a noticeable overhead. A hook invocation is only a couple of function
calls and pointer deferences; I would strongly suspect this is lost in
the noise relative to the huge amount of printf string parsing goop
going on here.
I am concerned about how verbose the error_log output is now though.
How about restricting this whole pid/tid logging block to
if (level_and_mask >= APLOG_DEBUG) {
?
Regards, Joe
Re: svn commit: r952724 - /httpd/httpd/trunk/server/log.c
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/log.c?rev=952724&r1=952723&r2=952724&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/log.c (original)
> +++ httpd/httpd/trunk/server/log.c Tue Jun 8 16:26:39 2010
> @@ -628,12 +628,17 @@ static void log_error_core(const char *f
> "[%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 == AP_MPMQ_STATIC) {
> + apr_os_thread_t tid = apr_os_thread_current();
> + len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
> + ":tid %pT", &tid);
> + }
> }
> #endif
> errstr[len++] = ']';
Question; where is the best place to perform this query once; and then to set
aside the ap_mpm_query result as a static? Hitting an mpm_query on each and
every log entry isn't exactly efficient.
Ideas?
Re: svn commit: r952724 - /httpd/httpd/trunk/server/log.c
Posted by Rainer Jung <ra...@kippdata.de>.
Sorry for the noise, wrong list and language ...
On 08.06.2010 22:20, Rainer Jung wrote:
> On 08.06.2010 22:08, William A. Rowe Jr. wrote:
>> On 6/8/2010 1:19 PM, William A. Rowe Jr. wrote:
>>> -1 veto. This is not the appropriate test for a threaded MPM.
>>>
>>> We have ap_mpm_query for this very purpose, please don't continue
>>> to propagate this broken meme!!!
>>
>> I'm sorry - I didn't read this patch sufficiently (zeroed straight
>> into the APR_HAVE_THREADS test). However, there seems to be no reason
>> to exclude logging the threads with static builds(!?!)
>>
>> I'm not sure there's any reason to test AP_MPMQ_NOT_SUPPORTED either;
>> this should be a true/false since this query was introduced day one.
>
> Actually the OS2 and Netware MPMs return AP_MPMQ_DYNAMIC for that query.
> Although I never worked with those MPMs, the code indicates they are
> actually threaded, but the number of threads per process can vary.
>
> So checking against AP_MPMQ_NOT_SUPPORTED is safer.
>
> Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r952724 - /httpd/httpd/trunk/server/log.c
Posted by Rainer Jung <ra...@kippdata.de>.
On 08.06.2010 22:08, William A. Rowe Jr. wrote:
> On 6/8/2010 1:19 PM, William A. Rowe Jr. wrote:
>> -1 veto. This is not the appropriate test for a threaded MPM.
>>
>> We have ap_mpm_query for this very purpose, please don't continue
>> to propagate this broken meme!!!
>
> I'm sorry - I didn't read this patch sufficiently (zeroed straight
> into the APR_HAVE_THREADS test). However, there seems to be no reason
> to exclude logging the threads with static builds(!?!)
>
> I'm not sure there's any reason to test AP_MPMQ_NOT_SUPPORTED either;
> this should be a true/false since this query was introduced day one.
Actually the OS2 and Netware MPMs return AP_MPMQ_DYNAMIC for that query.
Although I never worked with those MPMs, the code indicates they are
actually threaded, but the number of threads per process can vary.
So checking against AP_MPMQ_NOT_SUPPORTED is safer.
Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r952724 - /httpd/httpd/trunk/server/log.c
Posted by "William A. Rowe Jr." <wr...@apache.org>.
On 6/8/2010 1:19 PM, William A. Rowe Jr. wrote:
> -1 veto. This is not the appropriate test for a threaded MPM.
>
> We have ap_mpm_query for this very purpose, please don't continue
> to propagate this broken meme!!!
I'm sorry - I didn't read this patch sufficiently (zeroed straight
into the APR_HAVE_THREADS test). However, there seems to be no reason
to exclude logging the threads with static builds(!?!)
I'm not sure there's any reason to test AP_MPMQ_NOT_SUPPORTED either;
this should be a true/false since this query was introduced day one.
Reviewing it now.
In any case, the presence or absence of a tid in the log, or of one
platform or the other shouldn't be blocking an alpha candidate :)
If something is broke, we fix in the next alpha.
Re: svn commit: r952724 - /httpd/httpd/trunk/server/log.c
Posted by "William A. Rowe Jr." <wr...@apache.org>.
On 6/8/2010 1:19 PM, William A. Rowe Jr. wrote:
> -1 veto. This is not the appropriate test for a threaded MPM.
>
> We have ap_mpm_query for this very purpose, please don't continue
> to propagate this broken meme!!!
I'm sorry - I didn't read this patch sufficiently (zeroed straight
into the APR_HAVE_THREADS test). However, there seems to be no reason
to exclude logging the threads with static builds(!?!)
I'm not sure there's any reason to test AP_MPMQ_NOT_SUPPORTED either;
this should be a true/false since this query was introduced day one.
Reviewing it now.
In any case, the presence or absence of a tid in the log, or of one
platform or the other shouldn't be blocking an alpha candidate :)
If something is broke, we fix in the next alpha.
Re: svn commit: r952724 - /httpd/httpd/trunk/server/log.c
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Tue, 8 Jun 2010, William A. Rowe Jr. wrote:
> -1 veto. This is not the appropriate test for a threaded MPM.
>
> We have ap_mpm_query for this very purpose, please don't continue
> to propagate this broken meme!!!
Can you please be more verbose about what exactly you are objecting to? I
think the check should not be
result == AP_MPMQ_STATIC
but
result != AP_MPMQ_NOT_SUPPORTED
Is that what you meant?
> On 6/8/2010 11:26 AM, rjung@apache.org wrote:
>> Author: rjung
>> Date: Tue Jun 8 16:26:39 2010
>> New Revision: 952724
>>
>> URL: http://svn.apache.org/viewvc?rev=952724&view=rev
>> #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 == AP_MPMQ_STATIC) {
>> + apr_os_thread_t tid = apr_os_thread_current();
>> + len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
>> + ":tid %pT", &tid);
>> + }
>> }
Re: svn commit: r952724 - /httpd/httpd/trunk/server/log.c
Posted by "William A. Rowe Jr." <wr...@rowe-clan.net>.
-1 veto. This is not the appropriate test for a threaded MPM.
We have ap_mpm_query for this very purpose, please don't continue
to propagate this broken meme!!!
On 6/8/2010 11:26 AM, rjung@apache.org wrote:
> Author: rjung
> Date: Tue Jun 8 16:26:39 2010
> New Revision: 952724
>
> URL: http://svn.apache.org/viewvc?rev=952724&view=rev
> Log:
> Add descriptive prefix to pid and tid in the error log.
> Only log the tid, if the MPM is threaded.
>
> Suggested by jorton.
>
> Modified:
> httpd/httpd/trunk/server/log.c
>
> Modified: httpd/httpd/trunk/server/log.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/log.c?rev=952724&r1=952723&r2=952724&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/log.c (original)
> +++ httpd/httpd/trunk/server/log.c Tue Jun 8 16:26:39 2010
> @@ -628,12 +628,17 @@ static void log_error_core(const char *f
> "[%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 == AP_MPMQ_STATIC) {
> + apr_os_thread_t tid = apr_os_thread_current();
> + len += apr_snprintf(errstr + len, MAX_STRING_LEN - len,
> + ":tid %pT", &tid);
> + }
> }
> #endif
> errstr[len++] = ']';
>
>
>