Hi Christopher.
On Tue, Mar 2, 2010 at 2:51 PM, Christopher Schultz <
ch...@christopherschultz.net> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Xie,
>
> [snip]
> > if (!isHttpServlet(request, response)) {
> > chain.doFilter(request, response);
> > return;
> > }
>
> Why does the request have to be an HTTP request in order to have the
> access log run?
That does seem to be a bug. By default, the access logger logs the common
log web server
format, but of course it doesn't have to, so it should log non-HTTP requests
as well, but maybe
only if a non-default pattern is configured?
> > long t2 = System.currentTimeMillis();
> > long time = t2 - t1;
>
> This isn't your choice, it's in the original code, but why not just do:
>
> long elapsed = System.currentTimeMillis();
> ...
> elapsed = System.currentTimeMillis() - elapsed;
>
> ??
>
> Fewer items on the stack, etc.
>
Except that then it is more difficult to debug. Right? It isn't as easy to
inspect the value of
the current time if you perform the subtraction without first assigning the
current time to a
variable.
> > Date date = getDate();
> > StringBuffer result = new StringBuffer();
>
> The original default size for the StringBuffer was 128 characters.
> Presumably, this was done to avoid the cost of re-sizing the
> StringBuffer whenever it grew too large. The default initial capacity of
> a StringBuffer is 16 characters, which is almost certainly too small for
> a reasonable access log entry. You should put the larger size back in.
>
> While you're at it, use StringBuilder instead to avoid the overhead of
> synchronization for an object in a single-threaded environment.
>
Yes, using StringBuilder would probably make people worry less. :)
> > public void log(String message) {
>
> Maybe re-write this method to avoid having to convert from
> StringBuffer/StringBuilder to String: why do the work if you don't have to?
>
> > private static String lookup(String month) {
> > int index;
> > try {
> > index = Integer.parseInt(month) - 1;
>
> Why not have a dummy "month" at index 0 and /not/ subtract? Come on...
> we're smarter than the Sun engineers, right? 0 = January? Stupid...
>
There might be a small opportunity for optimization/cleanup there.
> > private Date getDate() {
> > // Only create a new Date once per second, max.
> > long systime = System.currentTimeMillis();
> > AccessDateStruct struct = currentDateStruct.get();
> > if ((systime - struct.currentDate.getTime()) > 1000) {
> > struct.currentDate.setTime(systime);
> > struct.currentDateString = null;
> > }
> > return struct.currentDate;
> > }
>
> I don't understand why this is ThreadLocal, instead of just synchronized
> across the object. Maybe it's slightly faster to avoid the
> synchronization and just use ThreadLocals, but I'm not sure how many
> requests per second a single Thread is going to process, so I'm not
> convinced that caching this data is worth the complexity it requires in
> this class. I'd love to hear from a Tomcat dev about this.
>
Tomcat can (hopefully) answer a larger number of requests per second
every year on decently modern hardware. Benchmark it both ways on
a reasonably good/wide machine and you'll see why avoiding the sync
is helpful. I don't think it muddies the code very badly here.
> > protected static class ByteSentElement implements AccessLogElement {
> > private boolean conversion;
> >
> > /**
> > * if conversion is true, write '-' instead of 0 - %b
> > */
> > public ByteSentElement(boolean conversion) {
> > this.conversion = conversion;
> > }
> >
> > public void addElement(StringBuffer buf, Date date,
> > HttpServletRequest request, HttpServletResponse response,
> > long time) {
> > long length = 0;
> > if (response.getHeader("Content-Length") != null
> > && !"".equals(response.getHeader("Content-Length")))
> {
> > length =
> > Long.parseLong(response.getHeader("Content-Length"));
> > }
> > if (length <= 0 && response.getHeader("content-length") !=
> null
> > && !"".equals(response.getHeader("content-length")))
> {
> > length =
> > Long.parseLong(response.getHeader("content-length"));
> > }
> > if (length <= 0 && conversion) {
> > buf.append('-');
> > } else {
> > buf.append(length);
> > }
> > }
> > }
>
> Note that this class will only report what was sent with the
> Content-Length header, rather than actually counting the bytes that were
> sent. Fixing this requires an architectural change: the BytesSentElement
> must be able to observe the OutputStream/Writer used by the servlet and
> count the bytes that were sent.
>
> Also, this method can cause an error if the Content-Type exceeds 2^31-1,
> which is bad. :( Why bother parsing the Content-Length in this case?
>
> This leads to another question: if the class is "improved" to actually
> count bytes, how will you count higher than 2^31 - 1?
>
The %b portion of the Combined Log Format is documented to be the "size of
the object returned to the client, not including the response headers."
http://httpd.apache.org/docs/1.3/logs.html#common
Thanks.
--
Jason Brittain
Mulesoft <http://www.mulesoft.com>
