Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(flagd) Return the provided default value in case of error #22

Closed
alxckn opened this issue Apr 16, 2024 · 6 comments · Fixed by #25
Closed

(flagd) Return the provided default value in case of error #22

alxckn opened this issue Apr 16, 2024 · 6 comments · Fixed by #25

Comments

@alxckn
Copy link
Contributor

alxckn commented Apr 16, 2024

Each flag resolution method's signature requires a default_value. Currently it isn't used by the flagd provider.
It should be returned as value in case of error (here)

@alxckn
Copy link
Contributor Author

alxckn commented Apr 23, 2024

As discussed in #24 (comment), providers should rather raise in case of error, letting the SDK handle the error (and run necessary hooks). Then I guess it should probably be the responsibility of the SDK as well to craft a ResolutionDetail response with the default value in it. Does this make sense @beeme1mr ?

@beeme1mr
Copy link
Member

Yes, exactly.

@alxckn
Copy link
Contributor Author

alxckn commented Apr 23, 2024

Note at the moment, there is no error management in the SDK I believe: https://github.com/open-feature/ruby-sdk/blob/main/lib/open_feature/sdk/client.rb#L22-L36

Until hooks are properly implemented, the error will just bubble up

@alxckn
Copy link
Contributor Author

alxckn commented Apr 23, 2024

@beeme1mr @maxveldink
Looking at other provider implementations, in-memory, meta provider, the current pattern is to let provider implementations populate an error ResolutionDetails in case of issue.
I think we should align so the change is consistent across providers. Perhaps we could deal with this along these lines:

  • SDK exposes a ProviderError(error_code, reason) struct
  • In case of error, provider returns ProviderError instead of ResolutionDetails
  • SDK pattern matches against the returned type, either forwards a ResolutionDetails or builds one using the ProviderError attributes and the default_value

@maxveldink
Copy link
Member

maxveldink commented Apr 29, 2024

Yeah, we're missing the appropriate handling of provider errors in the client. Would you mind opening an issue on the ruby-sdk repo to track this discussion?

So, I think the error use case is already covered in the ResolutionDetails spec type. If the provider implementation knows about an error, it returns a ResolutionDetails with the error code, error message and probably reason fields filled out, then uses the supplied default_value to the call for the value field. The OF client then passes all that information through to the user as an EvaluationDetails. Once we have hook support, you could log/in whatever there or with a passed-in provider hook. I've written a few providers that do very little in the error handling department. Our internal LaunchDarkly provider implementation does much more work around error handling and logging. We have more visibility when flag misses, type mismatches, or LD-specific network errors.

We're not covering the case where an exception is thrown in a provider implementation (or any abnormal execution of a provider that does something that isn't spec-compliant). We must protect against that in the client and return a default value when the error fields are filled out.

@alxckn
Copy link
Contributor Author

alxckn commented Apr 30, 2024

I've created an issue on the ruby-sdk repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants