52North / WPS

**DEPRECATED** The 52°North Web Processing Service enables the deployment of geo-processes on the web in a standardized way.
GNU General Public License v2.0
63 stars 55 forks source link

Remove string concatenation from log statements #130

Open nuest opened 10 years ago

nuest commented 10 years ago

String concatenation in log statements decrease performance, they should be replaced with placeholders. Search the whole workspace with the regexes LOGGER*" + and log*" +

isuftin commented 10 years ago

:+1: What's your preferred way of tokenizing logging statements? Have you looked at http://docs.oracle.com/javase/7/docs/api/java/text/MessageFormat.html ?

nuest commented 10 years ago

I have to admit I have not considered formatting. I'd start with using the slf4 parameterized messages: http://www.slf4j.org/faq.html#logging_performance But these only call toString(() on objects afaik.

isuftin commented 10 years ago

If not using it yet, have you given consideration to using Logback over SLF4J and using the Async logging appender? http://logback.qos.ch/manual/appenders.html#AsyncAppender

nuest commented 10 years ago

We're actually using logback, but behind SLF4J as an abtraction layer.

AsyncAppender could probably be quickly configure in the WPS's logback XML config file.

isuftin commented 10 years ago

Agreed. My only hesitation in using it is it seems to drop TRACE, DEBUG and INFO levels of logging if its queue is 80% full. There's that tradeoff.

nuest commented 10 years ago

Well, I guess we could leave the old configuration in place and leave a comment there to switch back to that one if issues arise that require full TRACE log. But in a productive environment, you're probably not logging at DEBUG level anyway, right? And that is where the dropping makes sense because you're not stalling the application because of logging. But I'd have to read more about this to give a real opinion here, this is mostly guessing :smile:

isuftin commented 10 years ago

I agree. It does also drop INFO, though. :(