Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support persistent connections #241

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

drbrain
Copy link

@drbrain drbrain commented May 30, 2020

The released versions of this gem do not support HTTP persistent connections. This causes performance issues due to connection creation overhead and makes writing to the InfluxDB database fail if DNS is down when a new connection needs to be created.

Enabling persistent connections results in a ~9x speed increase allowing points to be written synchronously to a non-localhost influxDB in ~5ms instead of ~50ms for my computer.

This PR also fixes debug logging.

See commit messages for details.

drbrain added 5 commits May 29, 2020 20:46
This influxdb client is not using persistent connections.  This causes a
significant performance loss.  Below is a benchmark script that writes a
single point to influxdb.  Running it takes 44 seconds of time:

    $ ruby -Ilib t.rb
    Writing 1000 points
                       user     system      total        real
    write_points  19.270388   1.394931  20.665319 ( 44.622170)

This works out to a write speed of about 22 points per second, or about
50ms per point.

This is:
* Pretty slow.  If you are generating points quickly maximum resolution
  of 50ms is not great.  A user may lose data because they cannot sample
  quickly enough.
* Pretty inefficient.  For each point written the client library must
  establish a TCP connection, establish a TLS session, finally write
  some data.  Any data written may be restricted by the TCP slow-start
  windowing algorithm.

  Much more ruby code must be run, almost half the time spent is in the
  "user" category, time that could be doing anything else in a ruby
  application, or time that could be used by other processes.

This commit caches HTTP connections across requests.  Running the same
benchmark takes 4.26 seconds:

    $ ruby -Ilib t.rb
    Writing 1000 points
                       user     system      total        real
    write_points   0.551663   0.084603   0.636266 (  4.261201)

This works out to a speed of 234 points per second, or about 5ms per
point.  Writing points now no longer need to recreate a TCP connection,
renegotiate a TLS session, or be held up by TCP window sizing
limitations.

This is much more efficient in terms of CPU time used per point, instead
of ~46% of time taken occurring in the "user" category, now only 13% of
time is in "user".  The balance of the time is now spent waiting for IO
to complete.

Benchmark script:

    require "benchmark"
    require "influxdb"

    n = Integer ENV["N"] rescue 1_000

    influxdb =
      InfluxDB::Client.new url: ENV["INFLUX_URL"],
                           username: ENV["INFLUX_USER"],
                           password: ENV["INFLUX_PASS"],
                           time_precision: "u"

    def write_point counter, influxdb
      points = [
        {
          series: "test",
          values: {
            counter: counter,
          },
        },
      ]

      influxdb.write_points points
    end

    puts "Writing #{n} points"

    Benchmark.bm 12 do |bm|
      bm.report "write_points" do
        n.times do |i|
          write_point i, influxdb
        end
      end
    end
Previously setting the log level to Logger::DEBUG would have no effect
as the log level of the Logger object was not changed.

If you set:

    InfluxDB::Logging.log_level = Logger::DEBUG

InfluxDB::Logging::log? would return true, allowing the log statement to
proceed, but #log would not do anything because the Logger object was
still at its default from initialization, Logger::INFO.

By setting the log level directly on the Logger object and removing
::log? we allow the Logger object to determine if a log level needs to
be logged or not.

This allows debug-level log messages to be displayed.
With the addition of persistent connections we need to make a separate
InfluxDB::Client per worker as the persistent connection is attached to
the client.

Here this is done by copying the InfluxDB::Config object and creating a
new host queue (so threads don't have to communicate to share hosts) and
creating a new client without async enabled (so it will use HTTP or UDP
write methods).
@drbrain drbrain force-pushed the drbrain/persistent-connections branch 2 times, most recently from d6ce25c to 3af192d Compare May 30, 2020 08:25
@drbrain drbrain force-pushed the drbrain/persistent-connections branch 3 times, most recently from 5861c91 to 3e6265b Compare June 1, 2020 21:38
@drbrain drbrain force-pushed the drbrain/persistent-connections branch from 3e6265b to 67840b2 Compare June 1, 2020 22:33
Copy link
Collaborator

@Esity Esity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve and CICD errors first. Will look over this hopefully later today.

@@ -18,6 +18,7 @@ module InfluxDB
auth_method: nil,
proxy_addr: nil,
proxy_port: nil,
persistent: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be false. If people are using this gem, we want to allow them to turn on new features when they are ready.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

@drbrain
Copy link
Author

drbrain commented Jun 10, 2020

I'll note that one of the rubocop failures is for cyclomatic complexity, I seem to recall that similar checks are disabled in other places in the library, would you prefer I disable the check here, or to split the method apart?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants