Created attachment 34287 [details] Patch for providing detailed logging in RewriteValve The logging provided for RewriteValve is minimal. httpd provides detailed logging support with configurable log levels. Also It would be useful while debugging, if the time taken for rewrite is logged. I've added a patch replicating apache mod_rewrite behaviour.
This patch is not very good. Changing the log level from debug to info is not acceptable (debug is more than appropriate given what this is, and there's also going to be a large performance impact ...), and that's what most of the patch is doing in addition to logging statistics that don't look very useful (given the precision of currentTimeMillis).
The switch from the container's logger to a dedicated Rewrite logger is problematic too. There might be a case for recording the time taken to rewrite the URL but a) it should only be collected and logged when debug logging is enabled and b) should use nanoTime. As a general rule, please keep the coding style of any patches consistent with the code being patched.
(In reply to Mark Thomas from comment #2) > The switch from the container's logger to a dedicated Rewrite logger is > problematic too. Now so? I would think this might be the one good part of this proposed patch, because it makes it easier to adjust the log level of *only* the RewriteValve. Honestly, that should have been the only thing necessary, here. Everything else would have been configuration of the log as per usual.
If there are more than one rewrite valves used, it doesn't sound like a good idea.
That's a good point, Rémy, but it's no worse than the current situation where the rewrite logs all go to the container's logger. I think there's still some useful work that can be done, here.
(In reply to Mark Thomas from comment #2) > The switch from the container's logger to a dedicated Rewrite logger is > problematic too. I don't think writing all rewrite logs to container's logger is a good idea. In the current situation, adjusting container's logger level to DEBUG is required to write rewrite logs. (In reply to Christopher Schultz from comment #3) > Now so? I would think this might be the one good part of this proposed > patch, because it makes it easier to adjust the log level of *only* the > RewriteValve. As Christopher pointed out, this would make it easier to configure log level for RewriteValve. (In reply to Mark Thomas from comment #2) > There might be a case for recording the time taken to rewrite the URL but a) > it should only be collected and logged when debug logging is enabled and b) > should use nanoTime. I've reworked the patch with log levels DEBUG,TRACE and nanoTime. I've replicated httpd mod_rewrite logging behaviour where level 2 logs only the rewrite information and level 3 also logs all the rules that were applied before the rewrite happened.
Created attachment 34290 [details] Modified patch for Rewrite Logging
(In reply to Santhana Preethi from comment #6) > > Now so? I would think this might be the one good part of this proposed > > patch, because it makes it easier to adjust the log level of *only* the > > RewriteValve. > > As Christopher pointed out, this would make it easier to configure log level > for RewriteValve. That's his opinion and I happen to disagree with it. You can configure the log level of the container. I also don't see what the processing time is used for in a sub component, this screams like a feature from 15 years ago [use a debugger instead].
Created attachment 34293 [details] Valve logging change Since the issue is about configuring the valve logging, I have a possible patch for that. It allows creating subcategories for the container logger, and exploits the ValveBase.containerLog field to use that other logger. Not sure if it's really a good idea though.
(In reply to Remy Maucherat from comment #9) > > Since the issue is about configuring the valve logging, I have a possible > patch for that. It allows creating subcategories for the container logger, > and exploits the ValveBase.containerLog field to use that other logger. The proposed patch does allow adjusting the log level of ONLY the RewriteValve. It is definitely better than the current situation. (In reply to Remy Maucherat from comment #8) > > I also don't see what the processing time is used for in a sub component, > this screams like a feature from 15 years ago [use a debugger instead]. Often, Url Rewriting involves complex regex matching. Logging the processing time for the rewrite will be useful for debugging any slow requests. Also, httpd already logs the rules applied for every URI at a higher log level (3 and above).
Any update regarding writing RewriteValve logs based on Remy Maucherat's patch?
Although I didn't get feedback, I decided to include the change (with some fixes). So this is now "fixed".