logstash-plugins / logstash-output-stomp

Apache License 2.0
4 stars 13 forks source link

Not all messages get sent #8

Closed consulthys closed 8 years ago

consulthys commented 8 years ago

I've filed issue #5448 in the main Logstash repository, thinking that it might be a bug in how Logstash handles the inflight events, but the more I think about this the more it feels as if the stomp output is actually telling Logstash that it has finished its job as soon as @client.send() is called.

Since there's no ACK, I'm wondering if the problem could not be solved in this plugin simply by making sure to get an ACK before returning from the receive function.

Thoughts?

ph commented 8 years ago

I think you are right here, Logstash will call the receive method with all the events coming from the queue. We are using the send method and there is no verification by default that the messages are correctly received by the stomp endpoint.

For Logstash, he thinks he was successful at sending all the message because the output isn't blocking the shutdown.

I am not too familiar with the library itself, but I see mention of client_ack So I think we could change the behavior of this output to implement ack verification on every messages. checking ack for every messages could slow things down but at least we wouldn't lost any events.

Does stomp support acking in batch?

consulthys commented 8 years ago

Thanks for your answer, @ph!

I'll dig some more into the Stomp spec and I'll come back with my findings.

consulthys commented 8 years ago

According to the OnStomp documentation, we can check for a receipt id to be sent back by the broker when sending messages.

So instead of this:

@client.send(event.sprintf(@destination), event.to_json)

I've tried the following but the problem stayed.

@client.send(event.sprintf(@destination), event.to_json) do |r|
   puts "Got receipt: #{r[:'receipt-id']}"
end

I then turned to acking in batches. Since Stomp can do transactions, I've used them to send several events in batches and ACK them all at once. I've overridden the multi_receive method to send all events inside a single transaction, like this:

  def multi_receive(events)
    @client.transaction do |t|
      events.each { |event|
        t.send(event.sprintf(@destination), event.to_json)
      }
    end
 end

Again this didn't help and messages were dropped when Logstash decided to shut down.

Another piece of information from the OnStomp library attracted my attention, namely "What Really Goes Down when you .disconnect". This disconnect call is the key but it is never made in the plugin code, so the solution was simply to override the do_close method and call disconnect to give a chance to the stomp client to send anything in its buffer.

  public
  def do_close
    @logger.warn(["Disconnecting from stomp broker"])
    @client.disconnect if @client.connected?
  end # def do_close

I've submitted PR #9 with my proposed changes, i.e. do_close + the batch mode.

consulthys commented 8 years ago

Thanks @ph for merging this, much appreciated!

ph commented 8 years ago

closed by #9, I will release new plugins version for 2.X and 5.0

ph commented 8 years ago

I've published 3.0.1 and 2.0.5 of the plugin