[BUG] Awkward usage when trying to evaluate a boolean flag in a complex conditional

hairyhenderson commented 6 months ago

Observed behavior

(Related to #259, but that one was closed, so filing this instead)

I just ran into a papercut when working on an experiment to move from an in-house feature management system to OpenFeature. The previous system allowed calling a simple IsEnabled(flag string) bool method, which I'd like to replace with a call to client.BooleanValue. Mostly this works fine, but I just ran into an awkward case for some code with a fairly convoluted conditional:

if cond1 && (cond2 || (cond3 && features.IsEnabled(ctx, "myflag"))) {
...

In this case it's impossible to simply replace the features.IsEnabled call with client.BooleanValue due to the err return value. And I'd rather avoid evaluating the flag altogether if cond1 is false.

This isn't insurmountable, but it does make it more awkward to use OpenFeature in this case, and will cause adoption friction.

Expected Behavior

The package should be less awkward to integrate with an existing project.

Could an additional set of methods be considered like these?

func (c *Client) MustBoolean(ctx context.Context, flag string, defaultValue bool, evalCtx EvaluationContext, options ...Option) bool

func (c *Client) MustFloat(ctx context.Context, flag string, defaultValue float64, evalCtx EvaluationContext, options ...Option) float64

func (c *Client) MustInt(ctx context.Context, flag string, defaultValue int64, evalCtx EvaluationContext, options ...Option) int64

func (c *Client) MustObject(ctx context.Context, flag string, defaultValue interface{}, evalCtx EvaluationContext, options ...Option) interface{}

func (c *Client) MustString(ctx context.Context, flag string, defaultValue string, evalCtx EvaluationContext, options ...Option) string

Steps to reproduce

(See above)

hairyhenderson commented 6 months ago

If this is acceptable to the maintainers, I'm willing to contribute a PR for it 😉

Kavindu-Dodan commented 6 months ago

@hairyhenderson thank you for bringing this up. Given that this concern is raised again in a short time shows the need for a common agreement.

@toddbaert @thomaspoignant tagging you as I think a decision is needed from the TC level.

If this is acceptable to the maintainers, I'm willing to contribute a PR for it

Thank you. If we are to support non-error evaluations, then I would go with a set of facade methods for existing error methods. But let's first wait for TC decision on this.

note - CNCF openfeature-go discussion https://cloud-native.slack.com/archives/C06HAN9KP9Q/p1712248379124169

thomaspoignant commented 6 months ago

Adding in the issue some of the research I've made on different SDKs available (as mentioned in slack):

flagsmith: https://docs.flagsmith.com/clients/server-side#get-flags-for-an-identity
launchdarkly: https://docs.launchdarkly.com/sdk/server-side/go#getting-started
devcycle: https://docs.devcycle.com/sdk/server-side-sdks/go/go-usage#get-and-use-variable-by-key
go-feature-flag: https://gofeatureflag.org/docs/openfeature_sdk/server_providers/openfeature_go#evaluate-your-flag
Unleash seems to have an extra signature that looks like what was discussed in the issue: https://docs.getunleash.io/reference/sdks/go#3-use-unleash

That being said I am not opposed to having a sugar syntax to make things easier for the developer adoption.

I am not a fan of the Must prefix because I have trouble to understand exactly what we want to do with this prefix. Should we cast if we don't have the right type?

I like the IsEnabled better for boolean for example, but I don't have better ideas for the other types.

toddbaert commented 6 months ago

Somebody brought this up and KubeCon, and the way they put it was very astute... The spec says (in 1.4.10):

Methods, functions, or operations on the client MUST NOT throw exceptions, or otherwise abnormally terminate. Flag evaluation calls must always return the default value in the event of abnormal execution. Exceptions include functions or methods for the purposes for configuration or setup.

In Java, .NET, JS... this means these should never throw. The Go equivalent of throwing is returning an error... for that reason, on some level, our Go evaluation API is non-conforming... The fact that we return an error means these functions, as far as Go idioms are concerned, can in fact abnormally terminate. This issue illustrates exactly why this is a problem.

I don't know, practically, what path we want to take, but I think this is a pretty serious problem and we need to find a way to fix it (breaking or not).

Kavindu-Dodan commented 6 months ago

@toddbaert I think this was discussed here https://github.com/open-feature/go-sdk/issues/259. It was resolved as the author got their concern resolved with hook usage.

hairyhenderson commented 6 months ago

I am not a fan of the Must prefix

Yeah, that's fair. Often when I see functions with that prefix in the wild, it means "panic where you'd normally error", but that isn't really what we want here.

Naming-wise, how about just Boolean, Float, etc... - the Value suffix has always seemed odd to me, as it's implicit that you're getting a value out of the method call...

hairyhenderson commented 6 months ago

I've issued https://github.com/open-feature/go-sdk/pull/263 to demonstrate how this could work...

Kavindu-Dodan commented 6 months ago

@hairyhenderson thank you for your contribution :)

open-feature / go-sdk