search

Moocar / logback-gelf

Logback plugin to send GELF messages to graylog2 server
Apache License 2.0
147 stars 59 forks source link

NPE in GelfAppender.append kills the host app #43

Closed mike-hogan closed 9 years ago

mike-hogan commented 9 years ago

Using these dependencies:

ch.qos.logback.logback-core 1.1.2 ch.qos.logback.logback-classic 1.1.2 me.moocar.logback-gelf 0.12 com.google.code.gson.gson 2.2.4

I somehow managed to product this stacktrace:

java.lang.NullPointerException at me.moocar.logbackgelf.GelfAppender.append(GelfAppender.java:60) at me.moocar.logbackgelf.GelfAppender.append(GelfAppender.java:21) at ch.qos.logback.core.AppenderBase.doAppend(AppenderBase.java:85) at ch.qos.logback.core.spi.AppenderAttachableImpl.appendLoopOnAppenders(AppenderAttachableImpl.java:48) at ch.qos.logback.classic.Logger.appendLoopOnAppenders(Logger.java:273) at ch.qos.logback.classic.Logger.callAppenders(Logger.java:260) at ch.qos.logback.classic.Logger.buildLoggingEventAndAppend(Logger.java:442) at ch.qos.logback.classic.Logger.filterAndLog0Or3Plus(Logger.java:396) at ch.qos.logback.classic.Logger.debug(Logger.java:503)

And it killed my app.

I can see that the try/catch in GelfAppender.append re-throws the exception. Should it now swallow it?

mike-hogan commented 9 years ago

I figured out how I managed to produce this NPE. I was using an environment variable for the graylog server param, like this

    <graylog2ServerHost>${GRAYLOG_SERVER_IP}</graylog2ServerHost>

and it was not set, so the value GRAYLOG_SERVER_IP_IS_UNDEFINED comes in, which causes this exception

Caused by: java.lang.IllegalStateException: Unknown host: GRAYLOG_SERVER_IP_IS_UNDEFINED: unknown error. Make sure you have specified the 'graylog2ServerHost' property correctly in your logback.xml' at at me.moocar.logbackgelf.InternetUtils.getInetAddress(InternetUtils.java:53) at at me.moocar.logbackgelf.GelfAppender.initExecutor(GelfAppender.java:89)

Then subsequent appends throw NPE

Moocar commented 9 years ago

Perhaps a good middle ground here is to add a regex for graylog2ServerHost to ensure that it matches an IP. Then, if it doesn't, print a warning. PRs welcome :) otherwise I'll eventually get to it.

mike-hogan commented 9 years ago

I see. What if somebody wanted to use a dns name? As it happens my plan was to move my system to do that, so apps can keep logging if I cycle my graylog server and it gets a new ip address.

Moocar commented 9 years ago

yeah, good point. I guess this would he hard to protect against. Out of interest, what part of the stack is setting the value to "GRAYLOG_SERVER_IP_IS_UNDEFINED"? I've never seen that before.

mike-hogan commented 9 years ago

I think its logback.

Moocar commented 9 years ago

Closing as error message is better after merging #49