Support for non-deterministic resource configuration

[pjambet]

Supersedes #479 as a solution for #480

This PR introduces handling for a new resource option, deterministic, which defaults to true.

When true, the behavior is identical to the current behavior on master. The logic in Adapter will memoize the result of raw_semian_options, as well as the result of semian_resource, so that a single instance of the host class (the thing that includes Semian::Adapter, so an instance of Net::HTTP, a Redis connection object or a MySQL connection object) will reuse previously fetched options and previously instantiated Semian::ProtectedResource objects.

When the options includes deterministic: false, that memoization is turned off.

In practice, it means that calling Semian::Adapter#acquire_semian_resource might see different options, and in turn returns a different instance of Semian::ProtectedResource.

The only use case for this is for the Net::HTTP adapter, where the options hash is returned from the block given to Semian::NetHTTP.semian_configuration. The use case described in #480 can now be achieved with a configuration such as:

class CurrentSemianConfig < ActiveSupport::Attributes
  attribute :sub_resource_name
end

SEMIAN_HTTP_DEFAULTS = {
  success_threshold: 1,
  error_threshold: 3,
  error_timeout: 60,
  error_threshold_timeout: 100, 
  half_open_resource_timeout: 15, 
  bulkhead: false,
}

Semian::NetHTTP.semian_configuration = proc do |host, port|
  name = [host, port, CurrentSemianConfig.sub_resource_name].compact.join("_")
  SEMIAN_HTTP_DEFAULTS.merge(name: name, deterministic: false)
end

Because opting out of the memoization behavior would lead to multiple calls to raw_semian_options (and the execution of the config block) during a single "acquisition flow", an optimization was made in the net http adapter as to limit the number of executions of the config block to one per flow, with the with_cleared_non_deterministic_options method. That way, even non-deterministic options are cached during the execution of a request (or a connection).

---

Benchmark notes

I wasn't sure what the best approach was to benchmark this, but here is a summary of what I did. Note that my goal with this feature was to have no impact on deterministic flows.

I ran the following benchmark script on a host application that uses Semian for Redis, MySQL & Net::HTTP. The Redis & MySQL config have nothing special, the values are configured in the YML files as documented in Semian's README, and the Net::HTTP block was similar to the one above, a default hash merging #{host}_#{port} as the value for the name key.

# typed: false
# frozen_string_literal: true

require 'benchmark/ips'
require_relative './config/environment'

uri = URI("http://example.com")
http = Net::HTTP.start(uri.hostname)

redis_connection = nil
REDIS.with { |r| redis_connection = r } # the host app uses connection pool, where the REDIS constant was defined as an instance of ConnectionPool

mysql_connection = ActiveRecord::Base.connection

Benchmark.ips do |x|
  x.report('http') do
    http.semian_resource.acquire {}
  end

  x.report('redis') do
    redis_connection.semian_resource.acquire {}
  end

  x.report('mysql') do
    mysql_connection.semian_resource.acquire {}
  end

  x.compare!
end

On a 2021 14" Macbook Pro, I got the following results:

Warming up --------------------------------------
                http     7.442k i/100ms
               redis     7.883k i/100ms
               mysql     7.930k i/100ms
Calculating -------------------------------------
                http     75.206k (± 6.0%) i/s -    379.542k in   5.065632s
               redis     74.096k (± 4.9%) i/s -    370.501k in   5.012493s
               mysql     71.935k (± 7.2%) i/s -    364.780k in   5.098432s

Comparison:
                http:    75206.4 i/s
               redis:    74095.7 i/s - same-ish: difference falls within error
               mysql:    71934.6 i/s - same-ish: difference falls within error

With the changes from this branch, I got the following:

Warming up --------------------------------------
                http     5.394k i/100ms
               redis     7.652k i/100ms
               mysql     7.212k i/100ms
Calculating -------------------------------------
                http     48.084k (± 7.6%) i/s -    242.730k in   5.087050s
               redis     72.137k (± 5.6%) i/s -    359.644k in   5.001153s
               mysql     72.686k (± 5.7%) i/s -    367.812k in   5.076525s

Comparison:
               mysql:    72686.1 i/s
               redis:    72137.0 i/s - same-ish: difference falls within error
                http:    48083.8 i/s - 1.51x  slower

This matches my expectations, MySQL & Redis are not impacted by the changes in this branch, but Net::HTTP is. As much as the difference looks significant, ~48k i/s vs 75.2k i/s, I think this is acceptable for this use case. This amounts to ~0.014ms vs 0.021ms, which in practice is likely to be an acceptable performance hit in the context of HTTP requests that take between 10ms and 5,000+ms

[pjambet]

I honestly was surprised that I couldn't find a simpler implementation. The goal here was to make sure that @raw_semian_options is cleared at the end of an acquire flow, i.e. after establishing a connection, or after issuing a request

Things started getting a bit messy when I noticed that calls could sometimes be nested, IIRC when executing a request with response = http.request(request) where http is the result of Net::HTTP.start and request is an instance of Net::HTTP::Get then transport_request was called, and then connect to establish the connection when the keep alive expired

This is why I ended up with this convoluted implementation. resource_acquisition_started is used as the outermost flag, in that case, the one set from transport_request, and not the inner call to connect, so that we only clear @raw_semian_options once.

I'm also realizing I should probably add tests for this

EDIT: If anyone has a suggestion for a better implementation, because I really don't love this

[miry]

It looks a bit complex line for me.

I think symbolized_options could be nil, only when options is nil.
In other cases symbolized_options is a Hash.

Also I have doubts of using tap in this section of changes.
Can you help me to understand, why tap is required?

Do you know if it is possible to avoid this block

[pjambet]

Agreed, it does look overly complex.

The reasoning behind tap was to get an easy way to always return symbolized_options (regardless of the value for :deterministic). The alternative version I started with (and ended up refactoring to the tap version) looked like this:

symbolized_options = options && options.transform_keys(&:to_sym)
@semian_options = symbolized_options if (symbolized_options || {}).fetch(:deterministic, true)
symbolized_options

The symbolized_options || {} piece was for cases where raw_semian_options returns false or nil.

With all that said, would you prefer a tap-less, and without the || {} thingy, like this:

symbolized_options = options && options.transform_keys(&:to_sym)
@semian_options = symbolized_options if !symbolized_options || symbolized_options.fetch(:deterministic, true)
symbolized_options

[miry]

Would it better to symbolize Hash keys on initialization part for all adapters or HTTP one?
My goal here to have less operations.

Probably it is a good task for a future separate PR.

[casperisfine]

Agreed. Also for a quick symbolize keys:

Suggested change

	symbolized_options = options && options.map { |k, v| [k.to_sym, v] }.to_h
	symbolized_options = options && options.transform_keys(&:to_sym)

[miry]

There are no blockers from my side.
I would wait for @byroot review as well.

[miry]

Actually I think there is missing documentation part of the new attribute and Changelog.
But it could be added after review finished.

@pjambet FYI: I keep some small examples how to use semian for net/http adapter - https://github.com/Shopify/semian/tree/c8a1ee0199ef4e983d85da447be5ec3eee86c7fb/examples/net_http . It is possible to update them with new configuration option as part of documentation.

[casperisfine]

LGTM.

Not sure I'm a bit fine of the deterministic: false name though. Maybe dynamic: true or something? No strong opinion here.

[yash]

Agree with @casperisfine. Not sure about the deterministic name, but it's also not a blocker on my end.

[pjambet]

@casperisfine I do like :dynamic, with a default to false. Will make that change.

@miry I will update the changelog and the examples as you suggested

[pjambet]

Rubocop was incorrectly flagging this line with:

Style/SafeNavigation: Use safe navigation (&.) instead of checking if an object exists before calling the method.

But that wouldn't work when options is false.

[pjambet]

Just rebased because of the conflicts in CHANGELOG.md which was recently updated on master

[miry]

👍🏼