Compatibility with circuitbox 2.0.0

[a-maas]

Stakeholder Overview (learn more)

There were some big breaking changes introduced in circuitbox version 2.0 that are affecting the way this gem works with the circuit. circuitbox version 1.1.1 is incompatible with Ruby 3.X so downgrading is not an option. The most notable changes relevant for this gem are:

• run, by default, will raise on failure, where it previously returned nil. You can pass the argument exception: false argument to run to disable this new default behavior. This is backward compatible with v1.1.1, since the exception argument is a no-op until v2.0.
• Configuring a logger is deprecated, in favor of using a notifier.
• The cache option when initializing a circuit has been renamed to circuit_store.
• The class level reset method (used in testing) on Circuitbox has been removed.
• attr_accessor's on CircuitBreaker have been changed to attr_reader, or in some cases removed. This means that the circuit cannot be reconfigured after initialization without a full reset of Circuitbox. I'm not sure this really worked in v1 either, but the attr_accessor's at least gave the illusion that you could reconfigure the circuit.

Risk Estimate (learn more)

• ✅ Big/complex change
• ✅ Big splash zone
• ✅ High stakes if errors occur
• ✅ Low confidence
• ✅ Not hidden by feature flag

Changes

• Replace run with run(exception: false.
• Replace deprecated circuitbox reset with the new way to reset cached circuitbox for circuitbox v2.
• Use GitHub Actions instead of CircleCI, including a build matrix of ruby versions and circuitbox v1 and v2.

Updated Dependencies

• Support for circuitbox 2.0.0

[drinks]

Code changes seem to be as advertised, asked a question with regard to our desired functionality.

[drinks]

Double-checking: we want nil in all failure cases here so that the gem will apply a fallback rate, correct? Or could this potentially amount to silently swallowing invoice or distribute_tax requests?

[a-maas]

Yes, correct. You added the details around invoice and distribute below.

[drinks]

Might be nice to have a circuit-open test case here just to understand what would happen...

[a-maas]

I think these tests should cover that

[drinks]

Good call--I didn't see a distribute tax / circuit open case, but good that we have one for invoice.

[a-maas]

I will add a distribute tax circuit test.

[drinks]

Looked quickly at what we're doing for invoice and distribute_tax and it seems like the fallback strategy is to raise, so it's good that we'll be notified, but I think the caveat is that iirc invalid quotation requests count against the circuit failure count, so we might end up with an open circuit and operable upstream endpoint. Most invoice and distribute_tax operations are async, I believe, so maybe not an issue as long as we continue to raise. Very important to ensure that we don't return nil, however.

There may be some value in omitting the circuit for those operations entirely, and letting exponential backoff handle outage states instead of what feels like a more heavy handed approach with the status quo, but that's probably out of scope for this change.

[a-maas]

@drinks yeah, I agree the circuit setup is not ideal and we could have a variety of false positive circuit failures. Any Savon error will count as a failure. Luckily, I think the failure rate is high enough that, AFAIK, we haven't seen any issues with this during our years of use.