Server SDKs resilient to network/http errors

[devnev]

For my use-case, I would like to avoid having Flipt on the critical path to request handling, i.e. high latency or errors from Flipt should not fail a request. This can already be accomplished, but in some languages it requires awareness and a bunch of surrounding code at every location where flipt is invoked.

e.g. its OK in Rust:

if (flipt.evaluation.boolean(...).await.unwrap()) { ... } // unwrap makes potential panic clear, and can easily be replaced with:
if (flipt.evaluation.boolean(...).await.map_or(true, |r| r.enabled)) { ... }

but in PHP, its easy to miss, and noisy to fix:

if ($flipt->boolean(...)->getEnabled()) { ... } // request failures not handled, easily missed
// vs
try {
  $enabled = $flipt->boolean(...)->getEnabled();
} catch (\Psr\Http\Client\RequestException $e) {
  $enabled = true;
}
if ($enabled) { ... }

So I'm wondering if SDKs where this is relevant could include aids to make callers of flipt more resilient.

I can imagine a few approaches:

• Flipt clients have additional evaluation methods that take a fallback value: the simplest, but also doesn't make it visible when a developer forgets to call the method with a fallback
• Separate client interface where all methods require fallback values: odd duplication of the client, you the non-resilient method can't accidentally be called on the resilient client. Of course, that still requires that the right client be instantiated, but client instantiation ideally happens in just one or two places in the program, not at every evaluation site, making them easier to vet.
• Give the client an option for producing fallback values in case of exceptions. Leaves the callers and general usage unchanged, but it implies every flag evaluation can take the same fallback value, which seems questionable for booleans and infeasible for variants.
• Leave it as is, users with this need have to wrap the client themselves, and be more susceptable to flipt errors.

[markphelps]

Hey @devnev thanks for the issue. I agree we should do a better job of handling failures and likely provide a fallback/default value.

I think we could just go ahead and implement this and create new versions of the sdks. Alternatively we could do it in an iterative way by introducing methods with fallbacks like you described in addition to the existing methods and deprecate the existing ones and then do major version updates to swap to those with defaults. Wdyt?

[devnev]

@markphelps For myself, I would like to get to SDKs that require a fallback parameter as quickly as possible, so a new version with breaking changes would be fine. But I don't know about your general userbase or your release policies, you will have to make the call on how you wish to procede.

[devnev]

Ironically, my PHP example somewhat confirms the issue, as catching \Psr\Http\Client\RequestException misses connection errors, and \Psr\Http\Client\ClientExceptionInterface should be caught instead.

[erka]

@devnev would it help If we add the @throws \Psr\Http\Client\ClientExceptionInterface in PHP annotations as first quick step?

[devnev]

@erka My "immediate" fix has been to wrap the client I'm using and expose the wrapper instead. IIUC the annotation would require PHPStan to have any effect, is that correct? I'd definitely prefer not having to integrate another tool, and finding out which editors support it. Additionally, as shown in the initial example, exception handling in PHP (and most languages with exceptions) is quite clunky.

[erka]

@devnev you are a happy person. Company usually requires to have such tools during the PR review and PHPStan could help with those findings.

It would be great if you could check #308 and share your feedback about booleanValue and variantValue functions? Your code above could be like

if ($flipt->booleanValue('flag1', true, ...)) { ... } 

wdyt?

[devnev]

@erka thanks for the PR. The new methods make sense for me, and the annotations mean with the right tooling it would check my box on visibility.

• Is there a particular reason for having a setLogger method rather than making the logger a constructor parameter - or having both? Either works for me, but it may break FliptClient construction across multiple statements.
• Catching \Throwable seems very broad, is this good practice vs catching \ExceptionA|ExceptionB?

[erka]

@devnev Thank you for your feedback!

• Is there a particular reason for having a setLogger method rather than making the logger a constructor parameter - or having both? Either works for me, but it may break FliptClient construction across multiple statements.

I think the constructor has too much params already so I don't want to change it. Also PSR-3 has Psr\Log\LoggerAwareInterface which has the same method. So I hope that it's a common practice.

• Catching \Throwable seems very broad, is this good practice vs catching \ExceptionA|ExceptionB?

Great catch! I have addressed it. Sorry that I force pushed into the branch and it's hard to see the changes.

[devnev]

Thanks. Browsing through the clients, it still feels like the question applies to most of them. The Java client is even throwing RuntimeExceptions to avoid having checked exceptions. And in discussionf of most languages I've generally heard engineers preferring to not have checked exceptions.

[erka]

...The Java client is even throwing RuntimeExceptions ...

@devnev Thank you for pointing that out. I will take a look on it and apply the same patterns.