From c4e490aea9938ffcd9179c2ff73ea791157f42cc Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Tue, 25 Oct 2022 14:52:00 -0400 Subject: [PATCH 01/24] Install adapter gem, bump to Rails edge --- Gemfile | 3 ++- Gemfile.lock | 24 +++++++++++++++++++++++- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/Gemfile b/Gemfile index af5fd6ef..3541ec32 100644 --- a/Gemfile +++ b/Gemfile @@ -17,7 +17,8 @@ group :test do # The last stable version for MacOS ARM darwin gem "grpc", "1.47.0" gem "mysql2", "~> 0.5" - gem "activerecord", ">= 7.0.3" + gem "activerecord-trilogy-adapter" + gem "activerecord", github: "rails/rails", branch: "main" gem "hiredis", "~> 0.6" # NOTE: v0.12.0 required for ruby 3.2.0. https://github.com/redis-rb/redis-client/issues/58 gem "hiredis-client", ">= 0.12.0" diff --git a/Gemfile.lock b/Gemfile.lock index bb3d948d..115dcdb6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,3 +1,20 @@ +GIT + remote: https://github.com/rails/rails.git + revision: 73388a2350060045328fdd409fa2282b4764b90f + branch: main + specs: + activemodel (7.1.0.alpha) + activesupport (= 7.1.0.alpha) + activerecord (7.1.0.alpha) + activemodel (= 7.1.0.alpha) + activesupport (= 7.1.0.alpha) + activesupport (7.1.0.alpha) + concurrent-ruby (~> 1.0, >= 1.0.2) + connection_pool (>= 2.2.5) + i18n (>= 1.6, < 2) + minitest (>= 5.1) + tzinfo (~> 2.0) + PATH remote: . specs: @@ -16,6 +33,9 @@ GEM i18n (>= 1.6, < 2) minitest (>= 5.1) tzinfo (~> 2.0) + activerecord-trilogy-adapter (2.1.0) + activerecord (~> 7.1.a) + trilogy (>= 2.1.1) ast (2.4.2) benchmark-memory (0.2.0) memory_profiler (~> 1) @@ -81,6 +101,7 @@ GEM ruby-progressbar (1.11.0) ruby2_keywords (0.0.5) toxiproxy (2.0.2) + trilogy (2.2.0) tzinfo (2.0.5) concurrent-ruby (~> 1.0) unicode-display_width (2.4.2) @@ -94,7 +115,8 @@ PLATFORMS x86_64-linux DEPENDENCIES - activerecord (>= 7.0.3) + activerecord! + activerecord-trilogy-adapter benchmark-memory grpc (= 1.47.0) hiredis (~> 0.6) From bbc7259211621130d628060f097537d9209b20e1 Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Wed, 26 Oct 2022 12:13:07 -0400 Subject: [PATCH 02/24] Basis for integrating Semian at Trilogy adapter layer --- lib/semian/trilogy_adapter.rb | 84 +++++++++++++++++++++++++++ test/adapters/trilogy_adapter_test.rb | 57 ++++++++++++++++++ 2 files changed, 141 insertions(+) create mode 100644 lib/semian/trilogy_adapter.rb create mode 100644 test/adapters/trilogy_adapter_test.rb diff --git a/lib/semian/trilogy_adapter.rb b/lib/semian/trilogy_adapter.rb new file mode 100644 index 00000000..b4f36b1f --- /dev/null +++ b/lib/semian/trilogy_adapter.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +require "semian/adapter" +require "activerecord-trilogy-adapter" +require "active_record/connection_adapters/trilogy_adapter" + +module ActiveRecord + module ConnectionAdapters + class TrilogyAdapter + StatementInvalid.include(::Semian::AdapterError) + + class SemianError < StatementInvalid + def initialize(semian_identifier, *args) + super(*args) + @semian_identifier = semian_identifier + end + end + + ResourceBusyError = Class.new(SemianError) + CircuitOpenError = Class.new(SemianError) + end + end +end + +module Semian + module TrilogyAdapter + include Semian::Adapter + + attr_reader :raw_semian_options, :semian_identifier + + def initialize(options) + @raw_semian_options = options.delete(:semian) + @semian_identifier = begin + name = semian_options && semian_options[:name] + unless name + host = options[:host] || "localhost" + port = options[:port] || 3306 + name = "#{host}:#{port}" + end + :"trilogy_adapter_#{name}" + end + super + end + + def execute(sql, *) + if query_allowlisted?(sql) + super + else + acquire_semian_resource(adapter: :trilogy_adapter, scope: :execute) do + super + end + end + end + + private + + def resource_exceptions + [] + end + + # TODO: share this with Mysql2 + QUERY_ALLOWLIST = Regexp.union( + %r{\A(?:/\*.*?\*/)?\s*ROLLBACK}i, + %r{\A(?:/\*.*?\*/)?\s*COMMIT}i, + %r{\A(?:/\*.*?\*/)?\s*RELEASE\s+SAVEPOINT}i, + ) + + def query_allowlisted?(sql, *) + QUERY_ALLOWLIST.match?(sql) + rescue ArgumentError + return false unless sql.valid_encoding? + + raise + end + + def connect(*args) + acquire_semian_resource(adapter: :trilogy_adapter, scope: :connection) do + super + end + end + end +end + +ActiveRecord::ConnectionAdapters::TrilogyAdapter.prepend(Semian::TrilogyAdapter) diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb new file mode 100644 index 00000000..2253ef1d --- /dev/null +++ b/test/adapters/trilogy_adapter_test.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require "test_helper" +require "semian/trilogy_adapter" + +class TrilogyAdapterTest < Minitest::Test + ERROR_TIMEOUT = 5 + ERROR_THRESHOLD = 1 + SEMIAN_OPTIONS = { + name: "testing", + tickets: 1, + timeout: 0, + error_threshold: ERROR_THRESHOLD, + success_threshold: 2, + error_timeout: ERROR_TIMEOUT, + } + + def setup + @proxy = Toxiproxy[:semian_test_mysql] + Semian.destroy(:trilogy_adapter_testing) + + @configuration = { + adapter: "trilogy", + username: "root", + host: SemianConfig["toxiproxy_upstream_host"], + port: SemianConfig["mysql_toxiproxy_port"], + } + @adapter = trilogy_adapter + end + + def test_semian_identifier + assert_equal(:"trilogy_adapter_testing", @adapter.semian_identifier) + + adapter = trilogy_adapter(host: "127.0.0.1", semian: { name: nil }) + assert_equal(:"trilogy_adapter_127.0.0.1:13306", adapter.semian_identifier) + + adapter = trilogy_adapter(host: "example.com", port: 42, semian: { name: nil }) + assert_equal(:"trilogy_adapter_example.com:42", adapter.semian_identifier) + end + + def test_semian_can_be_disabled + resource = trilogy_adapter( + host: SemianConfig["toxiproxy_upstream_host"], + port: SemianConfig["mysql_toxiproxy_port"], + semian: false, + ).semian_resource + + assert_instance_of(Semian::UnprotectedResource, resource) + end + + private + + def trilogy_adapter(**config_overrides) + ActiveRecord::ConnectionAdapters::TrilogyAdapter + .new(@configuration.merge(config_overrides)) + end +end From 80b7fb7af208c80f6154a92c2b630f1717ff9ccd Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Wed, 26 Oct 2022 15:45:00 -0400 Subject: [PATCH 03/24] Open circuit on connection errors for TrilogyAdapter --- lib/semian/trilogy_adapter.rb | 2 +- test/adapters/trilogy_adapter_test.rb | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/lib/semian/trilogy_adapter.rb b/lib/semian/trilogy_adapter.rb index b4f36b1f..4bd35b96 100644 --- a/lib/semian/trilogy_adapter.rb +++ b/lib/semian/trilogy_adapter.rb @@ -55,7 +55,7 @@ def execute(sql, *) private def resource_exceptions - [] + [ActiveRecord::StatementInvalid] end # TODO: share this with Mysql2 diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb index 2253ef1d..4803096b 100644 --- a/test/adapters/trilogy_adapter_test.rb +++ b/test/adapters/trilogy_adapter_test.rb @@ -24,6 +24,9 @@ def setup username: "root", host: SemianConfig["toxiproxy_upstream_host"], port: SemianConfig["mysql_toxiproxy_port"], + read_timeout: 2, + write_timeout: 2, + semian: SEMIAN_OPTIONS, } @adapter = trilogy_adapter end @@ -48,6 +51,20 @@ def test_semian_can_be_disabled assert_instance_of(Semian::UnprotectedResource, resource) end + def test_connection_errors_open_the_circuit + @proxy.downstream(:latency, latency: 2200).apply do + ERROR_THRESHOLD.times do + assert_raises(ActiveRecord::StatementInvalid) do + @adapter.execute("SELECT 1;") + end + end + + assert_raises(ActiveRecord::ConnectionAdapters::TrilogyAdapter::CircuitOpenError) do + @adapter.execute("SELECT 1;") + end + end + end + private def trilogy_adapter(**config_overrides) From 18070d017e56a4f4a8604580157e98e450a9213f Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Wed, 26 Oct 2022 15:58:21 -0400 Subject: [PATCH 04/24] Ensure we only rescue timeout errors for now --- lib/semian/trilogy_adapter.rb | 12 +++++++++++- test/adapters/trilogy_adapter_test.rb | 12 ++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/lib/semian/trilogy_adapter.rb b/lib/semian/trilogy_adapter.rb index 4bd35b96..4ced7595 100644 --- a/lib/semian/trilogy_adapter.rb +++ b/lib/semian/trilogy_adapter.rb @@ -55,7 +55,17 @@ def execute(sql, *) private def resource_exceptions - [ActiveRecord::StatementInvalid] + [] + end + + def acquire_semian_resource(**) + super + rescue ::ActiveRecord::StatementInvalid => error + if error.cause.is_a?(Errno::ETIMEDOUT) + semian_resource.mark_failed(error) + error.semian_identifier = semian_identifier + end + raise end # TODO: share this with Mysql2 diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb index 4803096b..79d0a550 100644 --- a/test/adapters/trilogy_adapter_test.rb +++ b/test/adapters/trilogy_adapter_test.rb @@ -65,6 +65,18 @@ def test_connection_errors_open_the_circuit end end + def test_query_errors_do_not_open_the_circuit + (ERROR_THRESHOLD).times do + assert_raises(ActiveRecord::StatementInvalid) do + @adapter.execute("ERROR!") + end + end + err = assert_raises(ActiveRecord::StatementInvalid) do + @adapter.execute("ERROR!") + end + refute_kind_of(ActiveRecord::ConnectionAdapters::TrilogyAdapter::CircuitOpenError, err) + end + private def trilogy_adapter(**config_overrides) From b5d39ab0a76cfbcb97ed58473905077a3b1a125f Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Wed, 26 Oct 2022 16:00:35 -0400 Subject: [PATCH 05/24] Namespace test --- test/adapters/trilogy_adapter_test.rb | 131 +++++++++++++------------- 1 file changed, 67 insertions(+), 64 deletions(-) diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb index 79d0a550..2832318a 100644 --- a/test/adapters/trilogy_adapter_test.rb +++ b/test/adapters/trilogy_adapter_test.rb @@ -3,84 +3,87 @@ require "test_helper" require "semian/trilogy_adapter" -class TrilogyAdapterTest < Minitest::Test - ERROR_TIMEOUT = 5 - ERROR_THRESHOLD = 1 - SEMIAN_OPTIONS = { - name: "testing", - tickets: 1, - timeout: 0, - error_threshold: ERROR_THRESHOLD, - success_threshold: 2, - error_timeout: ERROR_TIMEOUT, - } +module ActiveRecord + module ConnectionAdapters + class TrilogyAdapterTest < Minitest::Test + ERROR_TIMEOUT = 5 + ERROR_THRESHOLD = 1 + SEMIAN_OPTIONS = { + name: "testing", + tickets: 1, + timeout: 0, + error_threshold: ERROR_THRESHOLD, + success_threshold: 2, + error_timeout: ERROR_TIMEOUT, + } - def setup - @proxy = Toxiproxy[:semian_test_mysql] - Semian.destroy(:trilogy_adapter_testing) + def setup + @proxy = Toxiproxy[:semian_test_mysql] + Semian.destroy(:trilogy_adapter_testing) - @configuration = { - adapter: "trilogy", - username: "root", - host: SemianConfig["toxiproxy_upstream_host"], - port: SemianConfig["mysql_toxiproxy_port"], - read_timeout: 2, - write_timeout: 2, - semian: SEMIAN_OPTIONS, - } - @adapter = trilogy_adapter - end + @configuration = { + adapter: "trilogy", + username: "root", + host: SemianConfig["toxiproxy_upstream_host"], + port: SemianConfig["mysql_toxiproxy_port"], + read_timeout: 2, + write_timeout: 2, + semian: SEMIAN_OPTIONS, + } + @adapter = trilogy_adapter + end - def test_semian_identifier - assert_equal(:"trilogy_adapter_testing", @adapter.semian_identifier) + def test_semian_identifier + assert_equal(:"trilogy_adapter_testing", @adapter.semian_identifier) - adapter = trilogy_adapter(host: "127.0.0.1", semian: { name: nil }) - assert_equal(:"trilogy_adapter_127.0.0.1:13306", adapter.semian_identifier) + adapter = trilogy_adapter(host: "127.0.0.1", semian: { name: nil }) + assert_equal(:"trilogy_adapter_127.0.0.1:13306", adapter.semian_identifier) - adapter = trilogy_adapter(host: "example.com", port: 42, semian: { name: nil }) - assert_equal(:"trilogy_adapter_example.com:42", adapter.semian_identifier) - end + adapter = trilogy_adapter(host: "example.com", port: 42, semian: { name: nil }) + assert_equal(:"trilogy_adapter_example.com:42", adapter.semian_identifier) + end - def test_semian_can_be_disabled - resource = trilogy_adapter( - host: SemianConfig["toxiproxy_upstream_host"], - port: SemianConfig["mysql_toxiproxy_port"], - semian: false, - ).semian_resource + def test_semian_can_be_disabled + resource = trilogy_adapter( + host: SemianConfig["toxiproxy_upstream_host"], + port: SemianConfig["mysql_toxiproxy_port"], + semian: false, + ).semian_resource - assert_instance_of(Semian::UnprotectedResource, resource) - end + assert_instance_of(Semian::UnprotectedResource, resource) + end - def test_connection_errors_open_the_circuit - @proxy.downstream(:latency, latency: 2200).apply do - ERROR_THRESHOLD.times do - assert_raises(ActiveRecord::StatementInvalid) do - @adapter.execute("SELECT 1;") + def test_connection_errors_open_the_circuit + @proxy.downstream(:latency, latency: 2200).apply do + ERROR_THRESHOLD.times do + assert_raises(ActiveRecord::StatementInvalid) do + @adapter.execute("SELECT 1;") + end + end + + assert_raises(TrilogyAdapter::CircuitOpenError) do + @adapter.execute("SELECT 1;") + end end end - assert_raises(ActiveRecord::ConnectionAdapters::TrilogyAdapter::CircuitOpenError) do - @adapter.execute("SELECT 1;") + def test_query_errors_do_not_open_the_circuit + (ERROR_THRESHOLD).times do + assert_raises(ActiveRecord::StatementInvalid) do + @adapter.execute("ERROR!") + end + end + err = assert_raises(ActiveRecord::StatementInvalid) do + @adapter.execute("ERROR!") + end + refute_kind_of(TrilogyAdapter::CircuitOpenError, err) end - end - end - def test_query_errors_do_not_open_the_circuit - (ERROR_THRESHOLD).times do - assert_raises(ActiveRecord::StatementInvalid) do - @adapter.execute("ERROR!") + private + + def trilogy_adapter(**config_overrides) + TrilogyAdapter.new(@configuration.merge(config_overrides)) end end - err = assert_raises(ActiveRecord::StatementInvalid) do - @adapter.execute("ERROR!") - end - refute_kind_of(ActiveRecord::ConnectionAdapters::TrilogyAdapter::CircuitOpenError, err) - end - - private - - def trilogy_adapter(**config_overrides) - ActiveRecord::ConnectionAdapters::TrilogyAdapter - .new(@configuration.merge(config_overrides)) end end From c850713c274874730a2f7f2a9087b155c7105391 Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Wed, 26 Oct 2022 16:03:57 -0400 Subject: [PATCH 06/24] Read timeout errors test --- test/adapters/trilogy_adapter_test.rb | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb index 2832318a..42a091e2 100644 --- a/test/adapters/trilogy_adapter_test.rb +++ b/test/adapters/trilogy_adapter_test.rb @@ -79,6 +79,25 @@ def test_query_errors_do_not_open_the_circuit refute_kind_of(TrilogyAdapter::CircuitOpenError, err) end + def test_read_timeout_error_opens_the_circuit + ERROR_THRESHOLD.times do + assert_raises(ActiveRecord::StatementInvalid) do + @adapter.execute("SELECT sleep(5)") + end + end + + assert_raises(TrilogyAdapter::CircuitOpenError) do + @adapter.execute("SELECT sleep(5)") + end + + # After TrilogyAdapter::CircuitOpenError check regular queries are working fine. + result = Timecop.travel(ERROR_TIMEOUT + 1) do + @adapter.execute("SELECT 1 + 1;") + end + + assert_equal(2, result.first[0]) + end + private def trilogy_adapter(**config_overrides) From 6762924f6b6959e77d4bc345ebf858a2fa34f195 Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Wed, 26 Oct 2022 16:22:50 -0400 Subject: [PATCH 07/24] Tests for instrumentation and resource identifier tagging --- lib/semian/trilogy_adapter.rb | 2 +- test/adapters/trilogy_adapter_test.rb | 58 +++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/lib/semian/trilogy_adapter.rb b/lib/semian/trilogy_adapter.rb index 4ced7595..30f2bd2f 100644 --- a/lib/semian/trilogy_adapter.rb +++ b/lib/semian/trilogy_adapter.rb @@ -61,7 +61,7 @@ def resource_exceptions def acquire_semian_resource(**) super rescue ::ActiveRecord::StatementInvalid => error - if error.cause.is_a?(Errno::ETIMEDOUT) + if error.cause.is_a?(Errno::ETIMEDOUT) || error.cause.is_a?(Errno::ECONNREFUSED) semian_resource.mark_failed(error) error.semian_identifier = semian_identifier end diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb index 42a091e2..566e34a9 100644 --- a/test/adapters/trilogy_adapter_test.rb +++ b/test/adapters/trilogy_adapter_test.rb @@ -33,6 +33,10 @@ def setup @adapter = trilogy_adapter end + def teardown + @adapter.disconnect! + end + def test_semian_identifier assert_equal(:"trilogy_adapter_testing", @adapter.semian_identifier) @@ -98,6 +102,60 @@ def test_read_timeout_error_opens_the_circuit assert_equal(2, result.first[0]) end + def test_connect_instrumentation + notified = false + subscriber = Semian.subscribe do |event, resource, scope, adapter| + next unless event == :success + + notified = true + + assert_equal(Semian[:trilogy_adapter_testing], resource) + assert_equal(:connection, scope) + assert_equal(:trilogy_adapter, adapter) + end + + @adapter.connect! + + assert(notified, "No notifications have been emitted") + ensure + Semian.unsubscribe(subscriber) + end + + def test_query_instrumentation + @adapter.connect! + + notified = false + subscriber = Semian.subscribe do |event, resource, scope, adapter| + notified = true + assert_equal(:success, event) + assert_equal(Semian[:trilogy_adapter_testing], resource) + assert_equal(:execute, scope) + assert_equal(:trilogy_adapter, adapter) + end + + @adapter.execute("SELECT 1;") + + assert(notified, "No notifications has been emitted") + ensure + Semian.unsubscribe(subscriber) + end + + def test_network_errors_are_tagged_with_the_resource_identifier + @proxy.down do + error = assert_raises(ActiveRecord::StatementInvalid) do + @adapter.execute("SELECT 1 + 1;") + end + assert_equal(@adapter.semian_identifier, error.semian_identifier) + end + end + + def test_other_mysql_errors_are_not_tagged_with_the_resource_identifier + error = assert_raises(ActiveRecord::StatementInvalid) do + @adapter.execute("SYNTAX ERROR!") + end + assert_nil(error.semian_identifier) + end + private def trilogy_adapter(**config_overrides) From b72e9e8c12ad7dd627722ac68d61b87f8da4f706 Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Thu, 27 Oct 2022 11:03:20 -0400 Subject: [PATCH 08/24] Tests for resource acquisition --- test/adapters/trilogy_adapter_test.rb | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb index 566e34a9..579c9b68 100644 --- a/test/adapters/trilogy_adapter_test.rb +++ b/test/adapters/trilogy_adapter_test.rb @@ -156,6 +156,29 @@ def test_other_mysql_errors_are_not_tagged_with_the_resource_identifier assert_nil(error.semian_identifier) end + def test_resource_acquisition_for_connect + @adapter.connect! + Semian[:trilogy_adapter_testing].acquire do + error = assert_raises(TrilogyAdapter::ResourceBusyError) do + @adapter.reconnect! + end + assert_equal(:trilogy_adapter_testing, error.semian_identifier) + end + end + + def test_resource_acquisition_for_query + @adapter.connect! + + Semian[:trilogy_adapter_testing].acquire do + assert_raises(TrilogyAdapter::ResourceBusyError) do + @adapter.execute("SELECT 1;") + end + end + end + end + end + end + private def trilogy_adapter(**config_overrides) From ec4846b7588d483cf38187625bedfbed7bc54132 Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Fri, 28 Oct 2022 11:52:35 -0400 Subject: [PATCH 09/24] Test resource timeout and circuit breaker work --- test/adapters/trilogy_adapter_test.rb | 68 ++++++++++++++++++++++++++- 1 file changed, 67 insertions(+), 1 deletion(-) diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb index 579c9b68..0ac2dd9e 100644 --- a/test/adapters/trilogy_adapter_test.rb +++ b/test/adapters/trilogy_adapter_test.rb @@ -158,9 +158,10 @@ def test_other_mysql_errors_are_not_tagged_with_the_resource_identifier def test_resource_acquisition_for_connect @adapter.connect! + Semian[:trilogy_adapter_testing].acquire do error = assert_raises(TrilogyAdapter::ResourceBusyError) do - @adapter.reconnect! + trilogy_adapter.connect! end assert_equal(:trilogy_adapter_testing, error.semian_identifier) end @@ -175,8 +176,73 @@ def test_resource_acquisition_for_query end end end + + def test_resource_timeout_on_connect + @proxy.downstream(:latency, latency: 500).apply do + background { @adapter.connect! } + + assert_raises(TrilogyAdapter::ResourceBusyError) do + trilogy_adapter.connect! + end + end + end + + def test_circuit_breaker_on_connect + @proxy.downstream(:latency, latency: 500).apply do + background { @adapter.connect! } + + ERROR_THRESHOLD.times do + assert_raises(TrilogyAdapter::ResourceBusyError) do + trilogy_adapter.connect! + end + end + end + + yield_to_background + + assert_raises(TrilogyAdapter::CircuitOpenError) do + trilogy_adapter.connect! + end + + Timecop.travel(ERROR_TIMEOUT + 1) do + trilogy_adapter.connect! + end + end + + def test_resource_timeout_on_query + adapter2 = trilogy_adapter + + @proxy.downstream(:latency, latency: 500).apply do + background { adapter2.execute("SELECT 1 + 1;") } + + assert_raises(TrilogyAdapter::ResourceBusyError) do + @adapter.query("SELECT 1 + 1;") + end + end + end + + def test_circuit_breaker_on_query + adapter2 = trilogy_adapter + + @proxy.downstream(:latency, latency: 2200).apply do + background { trilogy_adapter.execute("SELECT 1 + 1;") } + + ERROR_THRESHOLD.times do + assert_raises(TrilogyAdapter::ResourceBusyError) do + @adapter.query("SELECT 1 + 1;") + end end end + + yield_to_background + + assert_raises(TrilogyAdapter::CircuitOpenError) do + @adapter.execute("SELECT 1 + 1;") + end + + Timecop.travel(ERROR_TIMEOUT + 1) do + assert_equal(2, @adapter.execute("SELECT 1 + 1;").to_a.flatten.first) + end end private From 10c80b8541ded300a9e133b34641e5cf8239fde6 Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Fri, 28 Oct 2022 11:52:48 -0400 Subject: [PATCH 10/24] Tests for query allowlist --- test/adapters/trilogy_adapter_test.rb | 51 +++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb index 0ac2dd9e..e1e99fe4 100644 --- a/test/adapters/trilogy_adapter_test.rb +++ b/test/adapters/trilogy_adapter_test.rb @@ -245,6 +245,57 @@ def test_circuit_breaker_on_query end end + def test_semian_allows_rollback + @adapter.execute("START TRANSACTION;") + + Semian[:trilogy_adapter_testing].acquire do + @adapter.execute("ROLLBACK;") + end + end + + def test_semian_allows_rollback_with_marginalia + @adapter.execute("START TRANSACTION;") + + Semian[:trilogy_adapter_testing].acquire do + @adapter.execute("/*foo:bar*/ ROLLBACK;") + end + end + + def test_semian_allows_commit + @adapter.execute("START TRANSACTION;") + + Semian[:trilogy_adapter_testing].acquire do + @adapter.execute("COMMIT;") + end + end + + def test_query_allowlisted_returns_false_for_binary_sql + binary_query = File.read(File.expand_path("../../fixtures/binary.sql", __FILE__)) + refute(@adapter.send(:query_allowlisted?, binary_query)) + end + + def test_semian_allows_rollback_to_safepoint + @adapter.execute("START TRANSACTION;") + @adapter.execute("SAVEPOINT foobar;") + + Semian[:trilogy_adapter_testing].acquire do + @adapter.execute("ROLLBACK TO foobar;") + end + + @adapter.execute("ROLLBACK;") + end + + def test_semian_allows_release_savepoint + @adapter.execute("START TRANSACTION;") + @adapter.execute("SAVEPOINT foobar;") + + Semian[:trilogy_adapter_testing].acquire do + @adapter.execute("RELEASE SAVEPOINT foobar;") + end + + @adapter.execute("ROLLBACK;") + end + private def trilogy_adapter(**config_overrides) From 26d3fbc7dff292aa5cddceab781202b0757f53a4 Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Mon, 31 Oct 2022 09:32:01 -0400 Subject: [PATCH 11/24] Test unconfigured adapter --- test/adapters/trilogy_adapter_test.rb | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb index e1e99fe4..129169f6 100644 --- a/test/adapters/trilogy_adapter_test.rb +++ b/test/adapters/trilogy_adapter_test.rb @@ -57,6 +57,15 @@ def test_semian_can_be_disabled assert_instance_of(Semian::UnprotectedResource, resource) end + def test_unconfigured + adapter = trilogy_adapter( + host: SemianConfig["toxiproxy_upstream_host"], + port: SemianConfig["mysql_toxiproxy_port"], + ) + + assert_equal(2, adapter.execute("SELECT 1 + 1;").to_a.flatten.first) + end + def test_connection_errors_open_the_circuit @proxy.downstream(:latency, latency: 2200).apply do ERROR_THRESHOLD.times do From 6110bd55acac8cce3df47ced05ccb6c500074199 Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Mon, 31 Oct 2022 14:23:00 -0400 Subject: [PATCH 12/24] Half open circuit timeout --- lib/semian/trilogy_adapter.rb | 14 ++++++++++ test/adapters/trilogy_adapter_test.rb | 38 +++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/lib/semian/trilogy_adapter.rb b/lib/semian/trilogy_adapter.rb index 30f2bd2f..0689de88 100644 --- a/lib/semian/trilogy_adapter.rb +++ b/lib/semian/trilogy_adapter.rb @@ -52,6 +52,20 @@ def execute(sql, *) end end + def with_resource_timeout(temp_timeout) + if connection.nil? + prev_read_timeout = @config[:read_timeout] || 0 # We're assuming defaulting the timeout to 0 won't change in Trilogy + @config.merge!(read_timeout: temp_timeout) # Create new client with config + else + prev_read_timeout = connection.read_timeout + connection.read_timeout = temp_timeout + end + yield + ensure + @config.merge!(read_timeout: prev_read_timeout) + connection&.read_timeout = prev_read_timeout + end + private def resource_exceptions diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb index 129169f6..df58d540 100644 --- a/test/adapters/trilogy_adapter_test.rb +++ b/test/adapters/trilogy_adapter_test.rb @@ -305,6 +305,44 @@ def test_semian_allows_release_savepoint @adapter.execute("ROLLBACK;") end + def test_changes_timeout_when_half_open_and_configured + adapter = trilogy_adapter(semian: SEMIAN_OPTIONS.merge(half_open_resource_timeout: 1)) + + @proxy.downstream(:latency, latency: 3000).apply do + (ERROR_THRESHOLD * 2).times do + assert_raises(ActiveRecord::StatementInvalid) do + adapter.execute("SELECT 1 + 1;") + end + end + end + + assert_raises(TrilogyAdapter::CircuitOpenError) do + adapter.execute("SELECT 1 + 1;") + end + + Timecop.travel(ERROR_TIMEOUT + 1) do + @proxy.downstream(:latency, latency: 1500).apply do + assert_raises(ActiveRecord::StatementInvalid) do + adapter.execute("SELECT 1 + 1;") + end + end + end + + Timecop.travel(ERROR_TIMEOUT * 2 + 1) do + adapter.execute("SELECT 1 + 1;") + adapter.execute("SELECT 1 + 1;") + + # Timeout has reset to the normal 2 seconds now that Circuit is closed + @proxy.downstream(:latency, latency: 1500).apply do + adapter.execute("SELECT 1 + 1;") + end + end + + raw_connection = adapter.send(:connection) + assert_equal(2, raw_connection.read_timeout) + assert_equal(2, raw_connection.write_timeout) + end + private def trilogy_adapter(**config_overrides) From 5ef74b2031b8d88e2787dc2cd40098c671c330bc Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Mon, 31 Oct 2022 15:03:23 -0400 Subject: [PATCH 13/24] Test to ensure circuit open errors won't trigger circuit breaker --- test/adapters/trilogy_adapter_test.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb index df58d540..139244d8 100644 --- a/test/adapters/trilogy_adapter_test.rb +++ b/test/adapters/trilogy_adapter_test.rb @@ -343,6 +343,22 @@ def test_changes_timeout_when_half_open_and_configured assert_equal(2, raw_connection.write_timeout) end + def test_circuit_open_errors_do_not_trigger_the_circuit_breaker + @proxy.down do + err = assert_raises(ActiveRecord::StatementInvalid) do + @adapter.execute("SELECT 1;") + end + 2.times do + assert_raises(TrilogyAdapter::CircuitOpenError) do + @adapter.execute("SELECT 1;") + end + error = Semian[:trilogy_adapter_testing].circuit_breaker.last_error + assert_equal(ActiveRecord::StatementInvalid, error.class) + assert_equal(Errno::ECONNREFUSED, error.cause.class) + end + end + end + private def trilogy_adapter(**config_overrides) From 2c6fcfadf3e41f4975810148399653f3872e69fd Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Mon, 31 Oct 2022 15:03:45 -0400 Subject: [PATCH 14/24] Note on rescuing ActiveRecord::StatementInvalid from acquire_semian_resource --- lib/semian/trilogy_adapter.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/semian/trilogy_adapter.rb b/lib/semian/trilogy_adapter.rb index 0689de88..b3a3b5a6 100644 --- a/lib/semian/trilogy_adapter.rb +++ b/lib/semian/trilogy_adapter.rb @@ -74,7 +74,15 @@ def resource_exceptions def acquire_semian_resource(**) super - rescue ::ActiveRecord::StatementInvalid => error + # We're going to need to rescue ConnectionNotEstablished here + # and fix this upstream in the Trilogy adapter -- right now, #new_client raises + # raw ECONNREFUSED errors + # Also, we shouldn't be wrapping TIMEDOUT and ECONNREFUSED in StatementInvalid => use + # more appropriate error classes + # We see ECONNREFUSED wrapped as an AR::StatementInvalid exception instead of the + # raw one when we go through #execute, because it gets translated in #with_raw_connection + # I think #new_client should just be more nuanced + rescue ActiveRecord::StatementInvalid => error if error.cause.is_a?(Errno::ETIMEDOUT) || error.cause.is_a?(Errno::ECONNREFUSED) semian_resource.mark_failed(error) error.semian_identifier = semian_identifier From db32d287dbddf92fbb46906b64c2a261a5ac5b9d Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Tue, 1 Nov 2022 15:12:16 -0400 Subject: [PATCH 15/24] Some small fixes --- lib/semian/trilogy_adapter.rb | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/semian/trilogy_adapter.rb b/lib/semian/trilogy_adapter.rb index b3a3b5a6..428f3ba8 100644 --- a/lib/semian/trilogy_adapter.rb +++ b/lib/semian/trilogy_adapter.rb @@ -28,13 +28,14 @@ module TrilogyAdapter attr_reader :raw_semian_options, :semian_identifier - def initialize(options) - @raw_semian_options = options.delete(:semian) + def initialize(*options) + *, config = options + @raw_semian_options = config.delete(:semian) @semian_identifier = begin name = semian_options && semian_options[:name] unless name - host = options[:host] || "localhost" - port = options[:port] || 3306 + host = config[:host] || "localhost" + port = config[:port] || 3306 name = "#{host}:#{port}" end :"trilogy_adapter_#{name}" @@ -42,12 +43,12 @@ def initialize(options) super end - def execute(sql, *) + def execute(sql, name = nil, async: false) if query_allowlisted?(sql) - super + super(sql, name, async: async) else acquire_semian_resource(adapter: :trilogy_adapter, scope: :execute) do - super + super(sql, name, async: async) end end end @@ -55,14 +56,14 @@ def execute(sql, *) def with_resource_timeout(temp_timeout) if connection.nil? prev_read_timeout = @config[:read_timeout] || 0 # We're assuming defaulting the timeout to 0 won't change in Trilogy - @config.merge!(read_timeout: temp_timeout) # Create new client with config + @config[:read_timeout] = temp_timeout # Create new client with config else prev_read_timeout = connection.read_timeout connection.read_timeout = temp_timeout end yield ensure - @config.merge!(read_timeout: prev_read_timeout) + @config[:read_timeout] = prev_read_timeout connection&.read_timeout = prev_read_timeout end From 7dba1935813866f2b1dd9a17331189fd35dd140f Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Tue, 8 Nov 2022 15:20:49 -0500 Subject: [PATCH 16/24] TrilogyAdapter#execute should take allow_retry option --- Gemfile | 2 +- Gemfile.lock | 11 ++++++++++- lib/semian/trilogy_adapter.rb | 6 +++--- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/Gemfile b/Gemfile index 3541ec32..8d4a4580 100644 --- a/Gemfile +++ b/Gemfile @@ -17,7 +17,7 @@ group :test do # The last stable version for MacOS ARM darwin gem "grpc", "1.47.0" gem "mysql2", "~> 0.5" - gem "activerecord-trilogy-adapter" + gem "activerecord-trilogy-adapter", github: "github/activerecord-trilogy-adapter", branch: "main" gem "activerecord", github: "rails/rails", branch: "main" gem "hiredis", "~> 0.6" # NOTE: v0.12.0 required for ruby 3.2.0. https://github.com/redis-rb/redis-client/issues/58 diff --git a/Gemfile.lock b/Gemfile.lock index 115dcdb6..6baa4157 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,3 +1,12 @@ +GIT + remote: https://github.com/github/activerecord-trilogy-adapter.git + revision: b1f0cd5d8305285bd77ab3f711a1e43f52fedd31 + branch: main + specs: + activerecord-trilogy-adapter (2.1.0) + activerecord (~> 7.1.a) + trilogy (>= 2.1.1) + GIT remote: https://github.com/rails/rails.git revision: 73388a2350060045328fdd409fa2282b4764b90f @@ -116,7 +125,7 @@ PLATFORMS DEPENDENCIES activerecord! - activerecord-trilogy-adapter + activerecord-trilogy-adapter! benchmark-memory grpc (= 1.47.0) hiredis (~> 0.6) diff --git a/lib/semian/trilogy_adapter.rb b/lib/semian/trilogy_adapter.rb index 428f3ba8..166b2edd 100644 --- a/lib/semian/trilogy_adapter.rb +++ b/lib/semian/trilogy_adapter.rb @@ -43,12 +43,12 @@ def initialize(*options) super end - def execute(sql, name = nil, async: false) + def execute(sql, name = nil, async: false, allow_retry: false) if query_allowlisted?(sql) - super(sql, name, async: async) + super(sql, name, async: async, allow_retry: allow_retry) else acquire_semian_resource(adapter: :trilogy_adapter, scope: :execute) do - super(sql, name, async: async) + super(sql, name, async: async, allow_retry: allow_retry) end end end From f9f0294169cafe08ea6d9847af813e960acc35a7 Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Thu, 26 Jan 2023 10:43:41 -0500 Subject: [PATCH 17/24] Update to new error class branches --- Gemfile | 3 ++- Gemfile.lock | 35 +++++++++++++++++++---------------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/Gemfile b/Gemfile index 8d4a4580..2b0fb7a9 100644 --- a/Gemfile +++ b/Gemfile @@ -17,7 +17,8 @@ group :test do # The last stable version for MacOS ARM darwin gem "grpc", "1.47.0" gem "mysql2", "~> 0.5" - gem "activerecord-trilogy-adapter", github: "github/activerecord-trilogy-adapter", branch: "main" + gem "trilogy", github: "https://github.com/github/trilogy/pull/41", glob: "contrib/ruby/*.gemspec" + gem "activerecord-trilogy-adapter", github: "https://github.com/github/activerecord-trilogy-adapter/pull/24" gem "activerecord", github: "rails/rails", branch: "main" gem "hiredis", "~> 0.6" # NOTE: v0.12.0 required for ruby 3.2.0. https://github.com/redis-rb/redis-client/issues/58 diff --git a/Gemfile.lock b/Gemfile.lock index 6baa4157..486c0ad6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,12 +1,20 @@ GIT remote: https://github.com/github/activerecord-trilogy-adapter.git - revision: b1f0cd5d8305285bd77ab3f711a1e43f52fedd31 - branch: main + revision: 4143db9b0a1529c44f88743b24206e42e987679d + ref: refs/pull/24/head specs: activerecord-trilogy-adapter (2.1.0) activerecord (~> 7.1.a) trilogy (>= 2.1.1) +GIT + remote: https://github.com/github/trilogy.git + revision: b54138b188f19722d7f94614ae72064683e56276 + ref: refs/pull/41/head + glob: contrib/ruby/*.gemspec + specs: + trilogy (2.2.0) + GIT remote: https://github.com/rails/rails.git revision: 73388a2350060045328fdd409fa2282b4764b90f @@ -32,19 +40,6 @@ PATH GEM remote: https://rubygems.org/ specs: - activemodel (7.0.4.1) - activesupport (= 7.0.4.1) - activerecord (7.0.4.1) - activemodel (= 7.0.4.1) - activesupport (= 7.0.4.1) - activesupport (7.0.4.1) - concurrent-ruby (~> 1.0, >= 1.0.2) - i18n (>= 1.6, < 2) - minitest (>= 5.1) - tzinfo (~> 2.0) - activerecord-trilogy-adapter (2.1.0) - activerecord (~> 7.1.a) - trilogy (>= 2.1.1) ast (2.4.2) benchmark-memory (0.2.0) memory_profiler (~> 1) @@ -53,11 +48,19 @@ GEM concurrent-ruby (1.1.10) connection_pool (2.3.0) google-protobuf (3.21.12) + google-protobuf (3.21.12-x86_64-darwin) + google-protobuf (3.21.12-x86_64-linux) googleapis-common-protos-types (1.5.0) google-protobuf (~> 3.14) grpc (1.47.0) google-protobuf (~> 3.19) googleapis-common-protos-types (~> 1.0) + grpc (1.47.0-x86_64-darwin) + google-protobuf (~> 3.19) + googleapis-common-protos-types (~> 1.0) + grpc (1.47.0-x86_64-linux) + google-protobuf (~> 3.19) + googleapis-common-protos-types (~> 1.0) hiredis (0.6.3) hiredis-client (0.12.1) redis-client (= 0.12.1) @@ -110,7 +113,6 @@ GEM ruby-progressbar (1.11.0) ruby2_keywords (0.0.5) toxiproxy (2.0.2) - trilogy (2.2.0) tzinfo (2.0.5) concurrent-ruby (~> 1.0) unicode-display_width (2.4.2) @@ -144,6 +146,7 @@ DEPENDENCIES rubocop-shopify semian! toxiproxy + trilogy! webrick BUNDLED WITH From a30429dc1e3ccb540733a305d3a6fa7f2fa891c5 Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Thu, 26 Jan 2023 11:55:37 -0500 Subject: [PATCH 18/24] Changes to Trilogy adapter for new error classes --- lib/semian/trilogy_adapter.rb | 33 ++++++++++++--------------- test/adapters/trilogy_adapter_test.rb | 26 ++++++++++----------- 2 files changed, 28 insertions(+), 31 deletions(-) diff --git a/lib/semian/trilogy_adapter.rb b/lib/semian/trilogy_adapter.rb index 166b2edd..8ac93055 100644 --- a/lib/semian/trilogy_adapter.rb +++ b/lib/semian/trilogy_adapter.rb @@ -7,9 +7,9 @@ module ActiveRecord module ConnectionAdapters class TrilogyAdapter - StatementInvalid.include(::Semian::AdapterError) + ActiveRecord::ActiveRecordError.include(::Semian::AdapterError) - class SemianError < StatementInvalid + class SemianError < ActiveRecordError def initialize(semian_identifier, *args) super(*args) @semian_identifier = semian_identifier @@ -30,6 +30,7 @@ module TrilogyAdapter def initialize(*options) *, config = options + @read_timeout = config[:read_timeout] || 0 @raw_semian_options = config.delete(:semian) @semian_identifier = begin name = semian_options && semian_options[:name] @@ -54,41 +55,37 @@ def execute(sql, name = nil, async: false, allow_retry: false) end def with_resource_timeout(temp_timeout) + connection_was_nil = false + if connection.nil? - prev_read_timeout = @config[:read_timeout] || 0 # We're assuming defaulting the timeout to 0 won't change in Trilogy - @config[:read_timeout] = temp_timeout # Create new client with config + prev_read_timeout = @read_timeout # Use read_timeout from when we first connected + connection_was_nil = true else prev_read_timeout = connection.read_timeout connection.read_timeout = temp_timeout end + yield + + connection.read_timeout = temp_timeout if connection_was_nil ensure - @config[:read_timeout] = prev_read_timeout connection&.read_timeout = prev_read_timeout end private - def resource_exceptions - [] - end - def acquire_semian_resource(**) super - # We're going to need to rescue ConnectionNotEstablished here - # and fix this upstream in the Trilogy adapter -- right now, #new_client raises - # raw ECONNREFUSED errors - # Also, we shouldn't be wrapping TIMEDOUT and ECONNREFUSED in StatementInvalid => use - # more appropriate error classes - # We see ECONNREFUSED wrapped as an AR::StatementInvalid exception instead of the - # raw one when we go through #execute, because it gets translated in #with_raw_connection - # I think #new_client should just be more nuanced rescue ActiveRecord::StatementInvalid => error - if error.cause.is_a?(Errno::ETIMEDOUT) || error.cause.is_a?(Errno::ECONNREFUSED) + if error.cause.is_a?(Trilogy::TimeoutError) semian_resource.mark_failed(error) error.semian_identifier = semian_identifier end raise + end + + def resource_exceptions + [ActiveRecord::ConnectionNotEstablished] end # TODO: share this with Mysql2 diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb index 139244d8..b7902733 100644 --- a/test/adapters/trilogy_adapter_test.rb +++ b/test/adapters/trilogy_adapter_test.rb @@ -69,7 +69,7 @@ def test_unconfigured def test_connection_errors_open_the_circuit @proxy.downstream(:latency, latency: 2200).apply do ERROR_THRESHOLD.times do - assert_raises(ActiveRecord::StatementInvalid) do + assert_raises(ActiveRecord::ConnectionNotEstablished) do @adapter.execute("SELECT 1;") end end @@ -104,7 +104,7 @@ def test_read_timeout_error_opens_the_circuit end # After TrilogyAdapter::CircuitOpenError check regular queries are working fine. - result = Timecop.travel(ERROR_TIMEOUT + 1) do + result = time_travel(ERROR_TIMEOUT + 1) do @adapter.execute("SELECT 1 + 1;") end @@ -151,7 +151,7 @@ def test_query_instrumentation def test_network_errors_are_tagged_with_the_resource_identifier @proxy.down do - error = assert_raises(ActiveRecord::StatementInvalid) do + error = assert_raises(ActiveRecord::ConnectionNotEstablished) do @adapter.execute("SELECT 1 + 1;") end assert_equal(@adapter.semian_identifier, error.semian_identifier) @@ -213,7 +213,7 @@ def test_circuit_breaker_on_connect trilogy_adapter.connect! end - Timecop.travel(ERROR_TIMEOUT + 1) do + time_travel(ERROR_TIMEOUT + 1) do trilogy_adapter.connect! end end @@ -249,7 +249,7 @@ def test_circuit_breaker_on_query @adapter.execute("SELECT 1 + 1;") end - Timecop.travel(ERROR_TIMEOUT + 1) do + time_travel(ERROR_TIMEOUT + 1) do assert_equal(2, @adapter.execute("SELECT 1 + 1;").to_a.flatten.first) end end @@ -309,8 +309,8 @@ def test_changes_timeout_when_half_open_and_configured adapter = trilogy_adapter(semian: SEMIAN_OPTIONS.merge(half_open_resource_timeout: 1)) @proxy.downstream(:latency, latency: 3000).apply do - (ERROR_THRESHOLD * 2).times do - assert_raises(ActiveRecord::StatementInvalid) do + ERROR_THRESHOLD.times do + assert_raises(ActiveRecord::ConnectionNotEstablished) do adapter.execute("SELECT 1 + 1;") end end @@ -320,15 +320,16 @@ def test_changes_timeout_when_half_open_and_configured adapter.execute("SELECT 1 + 1;") end - Timecop.travel(ERROR_TIMEOUT + 1) do + # Circuit moves to half-open state, so 1500 of latency should result in error + time_travel(ERROR_TIMEOUT + 1) do @proxy.downstream(:latency, latency: 1500).apply do - assert_raises(ActiveRecord::StatementInvalid) do + assert_raises(ActiveRecord::ConnectionNotEstablished) do adapter.execute("SELECT 1 + 1;") end end end - Timecop.travel(ERROR_TIMEOUT * 2 + 1) do + time_travel(ERROR_TIMEOUT * 2 + 1) do adapter.execute("SELECT 1 + 1;") adapter.execute("SELECT 1 + 1;") @@ -345,7 +346,7 @@ def test_changes_timeout_when_half_open_and_configured def test_circuit_open_errors_do_not_trigger_the_circuit_breaker @proxy.down do - err = assert_raises(ActiveRecord::StatementInvalid) do + err = assert_raises(ActiveRecord::ConnectionNotEstablished) do @adapter.execute("SELECT 1;") end 2.times do @@ -353,8 +354,7 @@ def test_circuit_open_errors_do_not_trigger_the_circuit_breaker @adapter.execute("SELECT 1;") end error = Semian[:trilogy_adapter_testing].circuit_breaker.last_error - assert_equal(ActiveRecord::StatementInvalid, error.class) - assert_equal(Errno::ECONNREFUSED, error.cause.class) + assert_equal(ActiveRecord::ConnectionNotEstablished, error.class) end end end From eca4b8734641d2173a593ff2f02da397a2779beb Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Thu, 26 Jan 2023 13:23:02 -0500 Subject: [PATCH 19/24] WIP -- generic AR adapter --- lib/semian/active_record_adapter.rb | 118 ++++++++++++++++++++++++++ test/adapters/trilogy_adapter_test.rb | 2 +- 2 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 lib/semian/active_record_adapter.rb diff --git a/lib/semian/active_record_adapter.rb b/lib/semian/active_record_adapter.rb new file mode 100644 index 00000000..8a6d2886 --- /dev/null +++ b/lib/semian/active_record_adapter.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +require "semian/adapter" +require "active_record/connection_adapters/abstract_adapter" + +module ActiveRecord + module ConnectionAdapters + class ActiveRecordAdapter + ActiveRecord::ActiveRecordError.include(::Semian::AdapterError) + + class SemianError < ActiveRecordError + def initialize(semian_identifier, *args) + super(*args) + @semian_identifier = semian_identifier + end + end + + ResourceBusyError = Class.new(SemianError) + CircuitOpenError = Class.new(SemianError) + end + end +end + +module Semian + module ActiveRecordAdapter + include Semian::Adapter + + attr_reader :raw_semian_options, :semian_identifier + + def initialize(*options) + *, config = options + @read_timeout = config[:read_timeout] || 0 + @raw_semian_options = config.delete(:semian) + @semian_identifier = begin + name = semian_options && semian_options[:name] + unless name + host = config[:host] || "localhost" + port = config[:port] || 3306 + name = "#{host}:#{port}" + end + :"activerecord_adapter_#{name}" + end + super + end + + def execute(sql, name = nil, async: false, allow_retry: false) + if query_allowlisted?(sql) + super(sql, name, async: async, allow_retry: allow_retry) + else + acquire_semian_resource(adapter: :activerecord_adapter, scope: :execute) do + super(sql, name, async: async, allow_retry: allow_retry) + end + end + end + + def with_resource_timeout(temp_timeout) + connection_was_nil = false + + if connection.nil? + prev_read_timeout = @read_timeout # Use read_timeout from when we first connected + connection_was_nil = true + else + prev_read_timeout = connection.read_timeout + connection.read_timeout = temp_timeout + end + + yield + + connection.read_timeout = temp_timeout if connection_was_nil + ensure + connection&.read_timeout = prev_read_timeout + end + + private + + # AR adapter translates some of the raw connection errors, so we need special handling for the different adapters + def acquire_semian_resource(**) + super + rescue ActiveRecord::StatementInvalid => error + # if error.cause.is_a?(Trilogy::TimeoutError) + # semian_resource.mark_failed(error) + # error.semian_identifier = semian_identifier + # end + raise + end + + def resource_exceptions + [ActiveRecord::ConnectionNotEstablished] + end + + # TODO: share this with Mysql2 + QUERY_ALLOWLIST = Regexp.union( + %r{\A(?:/\*.*?\*/)?\s*ROLLBACK}i, + %r{\A(?:/\*.*?\*/)?\s*COMMIT}i, + %r{\A(?:/\*.*?\*/)?\s*RELEASE\s+SAVEPOINT}i, + ) + + def query_allowlisted?(sql, *) + QUERY_ALLOWLIST.match?(sql) + rescue ArgumentError + return false unless sql.valid_encoding? + + raise + end + + def connect(*args) + acquire_semian_resource(adapter: :activerecord_adapter, scope: :connection) do + super + end + end + + def exceptions_to_handle + [Trilogy::TimeoutError] + end + end +end + +ActiveRecord::ConnectionAdapters::AbstractAdapter.prepend(Semian::TrilogyAdapter) diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb index b7902733..4fc7f46e 100644 --- a/test/adapters/trilogy_adapter_test.rb +++ b/test/adapters/trilogy_adapter_test.rb @@ -333,7 +333,7 @@ def test_changes_timeout_when_half_open_and_configured adapter.execute("SELECT 1 + 1;") adapter.execute("SELECT 1 + 1;") - # Timeout has reset to the normal 2 seconds now that Circuit is closed + # Timeout has reset to the normal 2 seconds now that circuit is closed @proxy.downstream(:latency, latency: 1500).apply do adapter.execute("SELECT 1 + 1;") end From c42cd82c910badcce1294138d0d94c723308833d Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Fri, 27 Jan 2023 10:28:15 -0500 Subject: [PATCH 20/24] Revert back to modifying config for half open resource timeout --- lib/semian/trilogy_adapter.rb | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/lib/semian/trilogy_adapter.rb b/lib/semian/trilogy_adapter.rb index 8ac93055..0c70703a 100644 --- a/lib/semian/trilogy_adapter.rb +++ b/lib/semian/trilogy_adapter.rb @@ -30,7 +30,6 @@ module TrilogyAdapter def initialize(*options) *, config = options - @read_timeout = config[:read_timeout] || 0 @raw_semian_options = config.delete(:semian) @semian_identifier = begin name = semian_options && semian_options[:name] @@ -55,20 +54,16 @@ def execute(sql, name = nil, async: false, allow_retry: false) end def with_resource_timeout(temp_timeout) - connection_was_nil = false - if connection.nil? - prev_read_timeout = @read_timeout # Use read_timeout from when we first connected - connection_was_nil = true + prev_read_timeout = @config[:read_timeout] || 0 + @config.merge!(read_timeout: temp_timeout) # Create new client with temp_timeout for read timeout else prev_read_timeout = connection.read_timeout connection.read_timeout = temp_timeout end - yield - - connection.read_timeout = temp_timeout if connection_was_nil ensure + @config.merge!(read_timeout: prev_read_timeout) connection&.read_timeout = prev_read_timeout end From 74723905146d240ca9f265da6773c5499817199f Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Fri, 27 Jan 2023 16:28:13 -0500 Subject: [PATCH 21/24] SemianError should inherit from StatementInvalid --- lib/semian/trilogy_adapter.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/semian/trilogy_adapter.rb b/lib/semian/trilogy_adapter.rb index 0c70703a..2b5a3c60 100644 --- a/lib/semian/trilogy_adapter.rb +++ b/lib/semian/trilogy_adapter.rb @@ -9,7 +9,8 @@ module ConnectionAdapters class TrilogyAdapter ActiveRecord::ActiveRecordError.include(::Semian::AdapterError) - class SemianError < ActiveRecordError + # Ensure we can rescue ResourceBusyError and CircuitOpenError as ActiveRecord::StatementInvalid + class SemianError < StatementInvalid def initialize(semian_identifier, *args) super(*args) @semian_identifier = semian_identifier From 3b43ec57aa41247291987811bb3feda798cc3086 Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Mon, 30 Jan 2023 10:44:03 -0500 Subject: [PATCH 22/24] Ensure active? is circuit broken --- lib/semian/trilogy_adapter.rb | 6 +++ test/adapters/trilogy_adapter_test.rb | 71 +++++++++++++++++++++++++-- 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/lib/semian/trilogy_adapter.rb b/lib/semian/trilogy_adapter.rb index 2b5a3c60..6b267596 100644 --- a/lib/semian/trilogy_adapter.rb +++ b/lib/semian/trilogy_adapter.rb @@ -54,6 +54,12 @@ def execute(sql, name = nil, async: false, allow_retry: false) end end + def active? + acquire_semian_resource(adapter: :trilogy_adapter, scope: :ping) do + super + end + end + def with_resource_timeout(temp_timeout) if connection.nil? prev_read_timeout = @config[:read_timeout] || 0 diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb index 4fc7f46e..f2a04b78 100644 --- a/test/adapters/trilogy_adapter_test.rb +++ b/test/adapters/trilogy_adapter_test.rb @@ -123,7 +123,9 @@ def test_connect_instrumentation assert_equal(:trilogy_adapter, adapter) end - @adapter.connect! + # We can't use the public #connect! API here because we'll call + # active?, which will scope the event to :ping. + @adapter.send(:connect) assert(notified, "No notifications have been emitted") ensure @@ -149,6 +151,25 @@ def test_query_instrumentation Semian.unsubscribe(subscriber) end + def test_active_instrumentation + @adapter.connect! + + notified = false + subscriber = Semian.subscribe do |event, resource, scope, adapter| + notified = true + assert_equal(:success, event) + assert_equal(Semian[:trilogy_adapter_testing], resource) + assert_equal(:ping, scope) + assert_equal(:trilogy_adapter, adapter) + end + + @adapter.active? + + assert(notified, "No notifications has been emitted") + ensure + Semian.unsubscribe(subscriber) + end + def test_network_errors_are_tagged_with_the_resource_identifier @proxy.down do error = assert_raises(ActiveRecord::ConnectionNotEstablished) do @@ -186,6 +207,16 @@ def test_resource_acquisition_for_query end end + def test_resource_acquisition_for_ping + @adapter.connect! + + Semian[:trilogy_adapter_testing].acquire do + assert_raises(TrilogyAdapter::ResourceBusyError) do + @adapter.active? + end + end + end + def test_resource_timeout_on_connect @proxy.downstream(:latency, latency: 500).apply do background { @adapter.connect! } @@ -231,8 +262,6 @@ def test_resource_timeout_on_query end def test_circuit_breaker_on_query - adapter2 = trilogy_adapter - @proxy.downstream(:latency, latency: 2200).apply do background { trilogy_adapter.execute("SELECT 1 + 1;") } @@ -254,6 +283,42 @@ def test_circuit_breaker_on_query end end + def test_resource_timeout_on_ping + adapter2 = trilogy_adapter + + @proxy.downstream(:latency, latency: 500).apply do + background { adapter2.execute("SELECT 1 + 1;") } + + assert_raises(TrilogyAdapter::ResourceBusyError) do + @adapter.active? + end + end + end + + def test_circuit_breaker_on_ping + @proxy.downstream(:latency, latency: 2200).apply do + background { trilogy_adapter.execute("SELECT 1 + 1;") } + + ERROR_THRESHOLD.times do + assert_raises(TrilogyAdapter::ResourceBusyError) do + @adapter.query("SELECT 1 + 1;") + end + end + end + + yield_to_background + + assert_raises(TrilogyAdapter::CircuitOpenError) do + @adapter.active? + end + + time_travel(ERROR_TIMEOUT + 1) do + # Connection is no longer active, but we shouldn't be raising CircuitOpenError + # anymore. + refute @adapter.active? + end + end + def test_semian_allows_rollback @adapter.execute("START TRANSACTION;") From 95e37a5d1e0981b712ea41eb08ba1a728511e2c2 Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Mon, 30 Jan 2023 10:46:47 -0500 Subject: [PATCH 23/24] Linter --- lib/semian/active_record_adapter.rb | 2 +- lib/semian/trilogy_adapter.rb | 2 +- test/adapters/trilogy_adapter_test.rb | 17 ++++++++++++++--- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/lib/semian/active_record_adapter.rb b/lib/semian/active_record_adapter.rb index 8a6d2886..377777c7 100644 --- a/lib/semian/active_record_adapter.rb +++ b/lib/semian/active_record_adapter.rb @@ -82,7 +82,7 @@ def acquire_semian_resource(**) # error.semian_identifier = semian_identifier # end raise - end + end def resource_exceptions [ActiveRecord::ConnectionNotEstablished] diff --git a/lib/semian/trilogy_adapter.rb b/lib/semian/trilogy_adapter.rb index 6b267596..7169bd3f 100644 --- a/lib/semian/trilogy_adapter.rb +++ b/lib/semian/trilogy_adapter.rb @@ -84,7 +84,7 @@ def acquire_semian_resource(**) error.semian_identifier = semian_identifier end raise - end + end def resource_exceptions [ActiveRecord::ConnectionNotEstablished] diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb index f2a04b78..b83e0bbd 100644 --- a/test/adapters/trilogy_adapter_test.rb +++ b/test/adapters/trilogy_adapter_test.rb @@ -38,12 +38,14 @@ def teardown end def test_semian_identifier - assert_equal(:"trilogy_adapter_testing", @adapter.semian_identifier) + assert_equal(:trilogy_adapter_testing, @adapter.semian_identifier) adapter = trilogy_adapter(host: "127.0.0.1", semian: { name: nil }) + assert_equal(:"trilogy_adapter_127.0.0.1:13306", adapter.semian_identifier) adapter = trilogy_adapter(host: "example.com", port: 42, semian: { name: nil }) + assert_equal(:"trilogy_adapter_example.com:42", adapter.semian_identifier) end @@ -81,7 +83,7 @@ def test_connection_errors_open_the_circuit end def test_query_errors_do_not_open_the_circuit - (ERROR_THRESHOLD).times do + ERROR_THRESHOLD.times do assert_raises(ActiveRecord::StatementInvalid) do @adapter.execute("ERROR!") end @@ -89,6 +91,7 @@ def test_query_errors_do_not_open_the_circuit err = assert_raises(ActiveRecord::StatementInvalid) do @adapter.execute("ERROR!") end + refute_kind_of(TrilogyAdapter::CircuitOpenError, err) end @@ -138,6 +141,7 @@ def test_query_instrumentation notified = false subscriber = Semian.subscribe do |event, resource, scope, adapter| notified = true + assert_equal(:success, event) assert_equal(Semian[:trilogy_adapter_testing], resource) assert_equal(:execute, scope) @@ -157,6 +161,7 @@ def test_active_instrumentation notified = false subscriber = Semian.subscribe do |event, resource, scope, adapter| notified = true + assert_equal(:success, event) assert_equal(Semian[:trilogy_adapter_testing], resource) assert_equal(:ping, scope) @@ -175,6 +180,7 @@ def test_network_errors_are_tagged_with_the_resource_identifier error = assert_raises(ActiveRecord::ConnectionNotEstablished) do @adapter.execute("SELECT 1 + 1;") end + assert_equal(@adapter.semian_identifier, error.semian_identifier) end end @@ -183,6 +189,7 @@ def test_other_mysql_errors_are_not_tagged_with_the_resource_identifier error = assert_raises(ActiveRecord::StatementInvalid) do @adapter.execute("SYNTAX ERROR!") end + assert_nil(error.semian_identifier) end @@ -193,6 +200,7 @@ def test_resource_acquisition_for_connect error = assert_raises(TrilogyAdapter::ResourceBusyError) do trilogy_adapter.connect! end + assert_equal(:trilogy_adapter_testing, error.semian_identifier) end end @@ -315,7 +323,7 @@ def test_circuit_breaker_on_ping time_travel(ERROR_TIMEOUT + 1) do # Connection is no longer active, but we shouldn't be raising CircuitOpenError # anymore. - refute @adapter.active? + refute_predicate(@adapter, :active?) end end @@ -345,6 +353,7 @@ def test_semian_allows_commit def test_query_allowlisted_returns_false_for_binary_sql binary_query = File.read(File.expand_path("../../fixtures/binary.sql", __FILE__)) + refute(@adapter.send(:query_allowlisted?, binary_query)) end @@ -405,6 +414,7 @@ def test_changes_timeout_when_half_open_and_configured end raw_connection = adapter.send(:connection) + assert_equal(2, raw_connection.read_timeout) assert_equal(2, raw_connection.write_timeout) end @@ -419,6 +429,7 @@ def test_circuit_open_errors_do_not_trigger_the_circuit_breaker @adapter.execute("SELECT 1;") end error = Semian[:trilogy_adapter_testing].circuit_breaker.last_error + assert_equal(ActiveRecord::ConnectionNotEstablished, error.class) end end From 2b1d8ce2c3b4d2e59cea5d4473ac7f821d5eaf1b Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Mon, 30 Jan 2023 15:58:54 -0500 Subject: [PATCH 24/24] Rescue ResourceBusy and CircuitOpen errors stemming from TrilogyAdapter#active? --- lib/semian/trilogy_adapter.rb | 5 ++ test/adapters/trilogy_adapter_test.rb | 67 +++++---------------------- 2 files changed, 16 insertions(+), 56 deletions(-) diff --git a/lib/semian/trilogy_adapter.rb b/lib/semian/trilogy_adapter.rb index 7169bd3f..c64f8101 100644 --- a/lib/semian/trilogy_adapter.rb +++ b/lib/semian/trilogy_adapter.rb @@ -27,6 +27,9 @@ module Semian module TrilogyAdapter include Semian::Adapter + ResourceBusyError = ::ActiveRecord::ConnectionAdapters::TrilogyAdapter::ResourceBusyError + CircuitOpenError = ::ActiveRecord::ConnectionAdapters::TrilogyAdapter::CircuitOpenError + attr_reader :raw_semian_options, :semian_identifier def initialize(*options) @@ -58,6 +61,8 @@ def active? acquire_semian_resource(adapter: :trilogy_adapter, scope: :ping) do super end + rescue ResourceBusyError, CircuitOpenError => error + false end def with_resource_timeout(temp_timeout) diff --git a/test/adapters/trilogy_adapter_test.rb b/test/adapters/trilogy_adapter_test.rb index b83e0bbd..a299f0a4 100644 --- a/test/adapters/trilogy_adapter_test.rb +++ b/test/adapters/trilogy_adapter_test.rb @@ -198,7 +198,7 @@ def test_resource_acquisition_for_connect Semian[:trilogy_adapter_testing].acquire do error = assert_raises(TrilogyAdapter::ResourceBusyError) do - trilogy_adapter.connect! + trilogy_adapter.send(:connect) # Avoid going through connect!, which will call #active? end assert_equal(:trilogy_adapter_testing, error.semian_identifier) @@ -215,22 +215,12 @@ def test_resource_acquisition_for_query end end - def test_resource_acquisition_for_ping - @adapter.connect! - - Semian[:trilogy_adapter_testing].acquire do - assert_raises(TrilogyAdapter::ResourceBusyError) do - @adapter.active? - end - end - end - def test_resource_timeout_on_connect @proxy.downstream(:latency, latency: 500).apply do background { @adapter.connect! } assert_raises(TrilogyAdapter::ResourceBusyError) do - trilogy_adapter.connect! + trilogy_adapter.send(:connect) # Avoid going through connect!, which will call #active? end end end @@ -241,7 +231,7 @@ def test_circuit_breaker_on_connect ERROR_THRESHOLD.times do assert_raises(TrilogyAdapter::ResourceBusyError) do - trilogy_adapter.connect! + trilogy_adapter.send(:connect) # Avoid going through connect!, which will call #active? end end end @@ -291,42 +281,6 @@ def test_circuit_breaker_on_query end end - def test_resource_timeout_on_ping - adapter2 = trilogy_adapter - - @proxy.downstream(:latency, latency: 500).apply do - background { adapter2.execute("SELECT 1 + 1;") } - - assert_raises(TrilogyAdapter::ResourceBusyError) do - @adapter.active? - end - end - end - - def test_circuit_breaker_on_ping - @proxy.downstream(:latency, latency: 2200).apply do - background { trilogy_adapter.execute("SELECT 1 + 1;") } - - ERROR_THRESHOLD.times do - assert_raises(TrilogyAdapter::ResourceBusyError) do - @adapter.query("SELECT 1 + 1;") - end - end - end - - yield_to_background - - assert_raises(TrilogyAdapter::CircuitOpenError) do - @adapter.active? - end - - time_travel(ERROR_TIMEOUT + 1) do - # Connection is no longer active, but we shouldn't be raising CircuitOpenError - # anymore. - refute_predicate(@adapter, :active?) - end - end - def test_semian_allows_rollback @adapter.execute("START TRANSACTION;") @@ -421,17 +375,18 @@ def test_changes_timeout_when_half_open_and_configured def test_circuit_open_errors_do_not_trigger_the_circuit_breaker @proxy.down do - err = assert_raises(ActiveRecord::ConnectionNotEstablished) do - @adapter.execute("SELECT 1;") - end - 2.times do - assert_raises(TrilogyAdapter::CircuitOpenError) do + ERROR_THRESHOLD.times do + assert_raises(ActiveRecord::ConnectionNotEstablished) do @adapter.execute("SELECT 1;") end - error = Semian[:trilogy_adapter_testing].circuit_breaker.last_error + end - assert_equal(ActiveRecord::ConnectionNotEstablished, error.class) + assert_raises(TrilogyAdapter::CircuitOpenError) do + @adapter.execute("SELECT 1;") end + error = Semian[:trilogy_adapter_testing].circuit_breaker.last_error + + assert_equal(ActiveRecord::ConnectionNotEstablished, error.class) end end