FluentLogger.getLogger should NOT immediately connect

[diwakergupta]

Given that the most common pattern for FluentLogger.getLogger is to assign it as a static member, these objects are likely to get initialized early in the application lifecycle. The current implementation seems to try and open a connection to fluentd immediately, and fails miserably if fluentd isn't running:

java.net.ConnectException: Connection refused
    at java.net.PlainSocketImpl.socketConnect(Native Method)
    at java.net.AbstractPlainSocketImpl.doConnect(AbstractPlainSocketImpl.java:339)
    at java.net.AbstractPlainSocketImpl.connectToAddress(AbstractPlainSocketImpl.java:200)
    at java.net.AbstractPlainSocketImpl.connect(AbstractPlainSocketImpl.java:182)
    at java.net.SocksSocketImpl.connect(SocksSocketImpl.java:392)
    at java.net.Socket.connect(Socket.java:579)
    at org.fluentd.logger.sender.RawSocketSender.connect(RawSocketSender.java:89)
    at org.fluentd.logger.sender.RawSocketSender.open(RawSocketSender.java:77)
    at org.fluentd.logger.sender.RawSocketSender.<init>(RawSocketSender.java:72)
    at org.fluentd.logger.FluentLoggerFactory.getLogger(FluentLoggerFactory.java:61)
    at org.fluentd.logger.FluentLoggerFactory.getLogger(FluentLoggerFactory.java:44)
    at org.fluentd.logger.FluentLogger.getLogger(FluentLogger.java:31)

This is bad for many reasons, the most important being that doing this kind of error-prone I/O in constructors or factory methods is Bad Practice.

[komamitsu]

Sorry for my late response and thanks for your comments.

This is bad for many reasons, the most important being that doing this kind of error-prone I/O in constructors or factory methods is Bad Practice.

Could you tell me the concrete reasons why you thought it's bad? In the current way, we can notice if the Fluentd is running or not when creating FluentLogger instance. Also, FluentLogger re-connects to Fluentd at the next time if it failed to connect.

I think some other projects sometimes use similar way (e.g. https://github.com/facebook/presto/blob/master/presto-client/src/main/java/com/facebook/presto/client/StatementClient.java#L80)

Given that the most common pattern for FluentLogger.getLogger is to assign it as a static member, these objects are likely to get initialized early in the application lifecycle.

BTW, you mean you share a FluentLogger instance by multi-threads as a static member? FluentLogger is not thread-safe for now, so it's dangerous to share a FluentLogger by multi-threads.

Thanks

[jklap]

You may want to call out the non-thread safe FluentLogger.. the example provided is:
private static FluentLogger LOG = FluentLogger.getLogger("app");

If this is taken verbatim and dropped into a singleton bean such as a Spring app all of a sudden things are going to start barfing.

I would also agree that delaying the connection to Fluentd until the first message is wise-- or at least make it a configuration option. Your comment about determining if Fluentd is running when the logger is created doesn't really buy us much-- either it's up or it's not-- knowing that immediately doesn't change what we'd do next-- especially given it will attempt to connect/re-connect as needed. Connecting immediately may just be a wasted effort for an app that doesn't log much.

[komamitsu]

@diwakergupta @jklap I fixed this issue as #25. Thanks for the suggestions.

[diwakergupta]

Thanks @komamitsu!