t0xa / gelfj

Graylog Extended Log Format (GELF) implementation in Java and log4j appender without any dependencies.
https://github.com/t0xa/gelfj/wiki
Other
186 stars 116 forks source link

AMQP Appender does not initiate the channel correctly for confirms #72

Closed agentgt closed 10 years ago

agentgt commented 10 years ago

The current AMQP sender does not initiate the channels for confirms correctly and causes connections to be instantiated on every logging request.

The channel will be null every time because an exception will be thrown if you try to publish to the channel with out stating that you want confirms

From GelfAMQPSender:

            try {
                // establish the connection the first time
                if (channel == null) {
                    connection = factory.newConnection();
                    channel = connection.createChannel();
                    //MISSING channel.confirmSelect();
                }
                // SNIP property setting
                channel.basicPublish(exchangeName, routingKey, properties, message.toAMQPBuffer().array());
                channel.waitForConfirms();

                return true;
            } catch (Exception e) {
                channel = null;
                tries++;
            }

(I tested this with RabbitMQ 3.2.3. Perhaps other versions don't require stating confirmSelect() otherwise I'm not sure how it could have worked)

Also two features need to be implemented for this to appender to be production ready:

  1. We need a configuration to disable waitForConfirms. RabbitMQ and AMQP is already rather reliable and confirms just slow it down. In fact one fix to the above could be to just not to use confirms.
  2. We may need to add our own internal buffer queue to improve performance. The reason is that the RabbitMQ clients internal queue is not configurable. The Spring AMQP project has its own log4j appender and does what I'm talking about. Thus I think we need a AsynchGelfSender that use Java concurrent queues and could be used by the TCP and UDP appenders.
h0nIg commented 10 years ago

thats not true, i think it is related to your setup. we are running RabbitMQ 3.2.3 and we do not need to confirmSelect

h0nIg commented 10 years ago

regarding waitForConfirms, you can use e.g. an async log4j appender or write an extension to the GelfAMQPSender that does some kind of caching and therefore there is no loss of time

agentgt commented 10 years ago

@h0nIg As for the waitForConfirms I'm using RabbitMQ 3.3.0 (maybe you should upgrade :) ). The latest RabbitMQ client will throw an exception if you call waitForConfirms and the channel is not set for confirmation. You can clearly see this in the javadoc for #waitForConfirms -- Note, when called on a non-Confirm channel, waitForConfirms throws an IllegalStateException.

The log4j async appender will not work correctly MDC context (I tried) and in general the log4j async appender has serious issues and one of the reasons logback, log4j2 and Netflix's blitz4j.

Consequently I gave up and upgraded to log4j2 and use Apache Flume instead of RabbitMQ and Gelf.