search

logstash-plugins / logstash-output-google_pubsub

Logstash output for sending events to the Google Pub/Sub service
Apache License 2.0
7 stars 10 forks source link

Add interface to MessageFutureCallback for performance #18

Closed ivantsepp closed 5 years ago

ivantsepp commented 5 years ago

Update (see below horizontal rule for previous hypothesis)

After some debugging, it seems that there's some jruby magic going on when you invoke com.google.api.core.ApiFutures.addCallback on the callback object. Something's going on where jruby is loading a class/interface(?) and attaching that java object to our callback object.

However, you can specify that the MessageFutureCallback implements a specific interface (see https://github.com/jruby/jruby/wiki/CallingJavaFromJRuby#implementing-java-interfaces-in-jruby). This seems to make jruby happier and there isn't contention on jruby to go from ruby to java. Again, I do not have a 100% level understanding so I'm making guesses here.

Performance also seems better with this patch. You can see this behavior when using benchmark and a stripped down version of the logic: https://gist.github.com/ivantsepp/e156e35978afbdd29e23e6a3e9ea90cb

We've been debugging Logstash for performance recently since messages seemed to be backed up in the system. We've found that by removing the callback functionality in this plugin, throughput seems to be better.

I'm still wrangling my head to understand why adding a callback degrades performance and I suspect there's some locking/contention in that one call: com.google.api.core.ApiFutures.addCallback. Since the callback is just doing debug or error messages, I'm removing it in this patch.

Here is also CPU usage of our instances before and after this patch:

Dashboard_-_Monitoring_Overview

I hope for a discussion around this change and I think there are better ways to patch this too (perhaps there is an option in the plugin config to skip adding the callback if it's not needed). But if I am able to dig deeper and understand what's going on, I will post and update this PR!

ivantsepp commented 5 years ago

Posted an update to the original PR description. The updated PR is simply adding an interface to the callback class

ivantsepp commented 5 years ago

You can also see this behavior when using benchmark and a stripped down version of the logic:

https://gist.github.com/ivantsepp/e156e35978afbdd29e23e6a3e9ea90cb

ivantsepp commented 5 years ago

bump, anyone able to review this PR?

ivantsepp commented 5 years ago

thanks for looking!

josephlewis42 commented 5 years ago

Hey @ivantsepp, I was able to publish this because it was a non breaking change, the release is live now on rubygems. I did basic testing with the latest 7.x version of Logstash.

ivantsepp commented 5 years ago

Awesome! Thanks for publishing - cheers =)