Remove query of ENV on each circuit breaker acquire. #427

miry · 2022-10-09T12:38:33Z

Found that there is checking on ENV variable on each acquire invoking method.

Move logic of disabled? for Circuit breaker to Initialization part.
Circtuit breaker should not have any knowledge of ENV or configurations.

Update using of SEMIAN_DISABLED to disable Semian without creating any resources.

It would allow only check only once if CircuitBreaker exists in resource acquire.
If there is any of ENV SEMIAN_CIRCUIT_BREAKER_DISABLED or SEMIAN_DISABLED exists,
then it would disable Circuit breaker initialization.

Environment variable SEMIAN_CIRCUIT_BREAKER_DISABLED has higher priority over explicit option circuit_breaker: true.

Changes: When environment variable SEMIAN_DISABLED is set, then it would return UnprotectedResource, instead of ProtectedResource.

CHANGELOG.md

lib/semian.rb

AbdulRahmanAlHamali

LGTM, but I see no documentation in the README about these environment variables. Might be worth including that?

CHANGELOG.md

jgaskins

Feedback is mostly questions. I haven't looked too deeply into the APIs in this gem.

Also TIL ENV[] is slower than ENV.key?

Warming up --------------------------------------
                  []   511.958k i/100ms
               .key?   928.543k i/100ms
Calculating -------------------------------------
                  []      5.104M (± 0.6%) i/s -     25.598M in   5.015834s
               .key?      9.333M (± 0.4%) i/s -     47.356M in   5.073909s

Comparison:
               .key?:  9333316.7 i/s
                  []:  5103582.6 i/s - 1.83x  (± 0.00) slower

Benchmark code

# frozen_string_literal: true
require "benchmark/ips"

Benchmark.ips do |x|
  x.report("[]") { ENV["HOME"] }
  x.report(".key?") { ENV.key?("HOME") }
  x.compare!
end

test/semian_test.rb

lib/semian/circuit_breaker.rb

lib/semian/platform.rb

miry · 2022-11-14T14:42:12Z

Hi folks. If you have time, pls review again.

jgaskins

Mostly clarity in docs but also a compatibility question

README.md

lib/semian/circuit_breaker.rb

jgaskins · 2022-11-14T15:52:25Z

test/circuit_breaker_test.rb

-  def test_semian_wide_env_var_disables_circuit_breaker
+  def test_semian_wide_env_var_does_not_create_resource
    ENV["SEMIAN_DISABLED"] = "1"
-    open_circuit!

-    assert_circuit_closed
+    resource = Semian.register(
+      :global_env_var_disables_circuit_breaker,
+      tickets: 1,
+      error_threshold: 2,
+      error_timeout: 5,
+      success_threshold: 1,
+    )
+
+    assert_nil(resource)
+  ensure
+    ENV.delete("SEMIAN_DISABLED")
+  end
+
+  def test_semian_wide_env_disables_totaly_even_with_explicit_circuit_breaker
+    ENV["SEMIAN_DISABLED"] = "1"
+
+    resource = Semian.register(
+      :global_env_var_disables_circuit_breaker,
+      bulkhead: true,
+      tickets: 1,
+      circuit_breaker: true,
+      error_threshold: 2,
+      error_timeout: 5,
+      success_threshold: 1,
+    )
+
+    assert_nil(resource)


Looks like we always returned a non-nil value from .register before (or raised an exception). Is this method exposed to apps using Semian?

If so, and I'm reading it correctly, this may not be backwards-compatible.

Mostly. The problem that before it would not be raise, but NOOP. Because it created always resource objects, and skip logic during on acquire.

Example when it should raise:

Semian.register(:raise, {bulkhead: false, circuit_breaker: false})

That makes sense, but the contract of this method has still changed in this PR, if I understand correctly. If the return value of Semian.register was ever intended to be meaningful outside the gem, apps could now be receiving nil values that they weren't before.

That is true. I am going to stress it in changelog and also in gemspec post install notifications.

I have found that Semian.register requires return some resource.

Using NetHTTP adapter as example, I found that it raises error if semian is disabled.
Semian resource for network is initialized on acquire if it is missing.

As workaround I return UnprotectedResource if SEMIAN_DISABLED exists.

… method. Move logic of disabled? for Circuit breaker to Initalization part. Circtuit breaker should have any knowledge of ENV or configurations. Update using of SEMIAN_DISABLED to disable Semian without creating any resources. It would allow only check only once if CircuitBreaker exists in resource acquire. If there is any of ENV `SEMIAN_CIRCUIT_BREAKER_DISABLED` or `SEMIAN_DISABLED` exists, then it would disable Circuit breaker initialization.

miry self-assigned this Oct 9, 2022

miry added the Semian label Oct 9, 2022

miry marked this pull request as ready for review October 9, 2022 13:14

miry changed the title ~~Remove query of ENV on each circuit breaker acquire.~~ WIP: Remove query of ENV on each circuit breaker acquire. Oct 11, 2022

miry force-pushed the remove-env branch from 6bd208e to 5392db1 Compare October 11, 2022 15:51

miry changed the title ~~WIP: Remove query of ENV on each circuit breaker acquire.~~ Remove query of ENV on each circuit breaker acquire. Oct 16, 2022

miry force-pushed the remove-env branch 2 times, most recently from c324d39 to 87ab933 Compare October 16, 2022 11:54

miry requested review from casperisfine, a team, jgaskins, djlebersilvestre and AbdulRahmanAlHamali and removed request for a team October 16, 2022 18:27

casperisfine reviewed Oct 17, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

lib/semian.rb Outdated Show resolved Hide resolved

AbdulRahmanAlHamali reviewed Oct 19, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

jgaskins reviewed Oct 20, 2022

View reviewed changes

test/semian_test.rb Outdated Show resolved Hide resolved

lib/semian/circuit_breaker.rb Show resolved Hide resolved

lib/semian/circuit_breaker.rb Show resolved Hide resolved

lib/semian/platform.rb Show resolved Hide resolved

miry force-pushed the remove-env branch from 87ab933 to 7c08f4c Compare November 11, 2022 17:43

miry mentioned this pull request Nov 11, 2022

Extract bulkheading ENV settings to configuration part #442

Open

jgaskins reviewed Nov 14, 2022

View reviewed changes

miry force-pushed the remove-env branch 3 times, most recently from 2efef40 to dd417b2 Compare November 14, 2022 20:14

miry force-pushed the remove-env branch from dd417b2 to fe813e8 Compare November 14, 2022 20:15

casperisfine approved these changes Nov 15, 2022

View reviewed changes

miry merged commit fe813e8 into Shopify:master Nov 15, 2022

miry deleted the remove-env branch November 15, 2022 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove query of ENV on each circuit breaker acquire. #427

Remove query of ENV on each circuit breaker acquire. #427

miry commented Oct 9, 2022 •

edited

AbdulRahmanAlHamali left a comment

jgaskins left a comment

miry commented Nov 14, 2022

jgaskins left a comment

jgaskins Nov 14, 2022

miry Nov 14, 2022

miry Nov 14, 2022

jgaskins Nov 14, 2022

miry Nov 14, 2022 •

edited

miry Nov 14, 2022 •

edited

Remove query of ENV on each circuit breaker acquire. #427

Remove query of ENV on each circuit breaker acquire. #427

Conversation

miry commented Oct 9, 2022 • edited

AbdulRahmanAlHamali left a comment

Choose a reason for hiding this comment

jgaskins left a comment

Choose a reason for hiding this comment

miry commented Nov 14, 2022

jgaskins left a comment

Choose a reason for hiding this comment

jgaskins Nov 14, 2022

Choose a reason for hiding this comment

miry Nov 14, 2022

Choose a reason for hiding this comment

miry Nov 14, 2022

Choose a reason for hiding this comment

jgaskins Nov 14, 2022

Choose a reason for hiding this comment

miry Nov 14, 2022 • edited

Choose a reason for hiding this comment

miry Nov 14, 2022 • edited

Choose a reason for hiding this comment

miry commented Oct 9, 2022 •

edited

miry Nov 14, 2022 •

edited

miry Nov 14, 2022 •

edited