Should retrieving a value really provide an error?

[dmathieu]

Requirements

I am opening this issue after chatting about it with someone (sorry, I didn't look at your badge) at KubeCon EU.

Right now, as documented on the README, retrieving a value provides two return values: whether the flag is enabled, and an error.

v2Enabled, err := client.BooleanValue(
    context.Background(), "v2_enabled", true, openfeature.EvaluationContext{},
 )

This is a bit surprising, as I wouldn't expect to have to do error handling every time I fetch a feature flag. If an error occured there, I'd expect to be able to define a default (does it fail open or closed), but be able to proceed through my application normally.

Of course, there should be a way to retrieve those errors and report them back. But I'd have expected that as a global handler. Kind of what OpenTelemetry Go does with its Error handler.

While Go doesn't have exceptions, it feels like returning errors here also kind of breaks specification 1.4.10.

I am aware this is a breaking change and is unlikely to be shipped now that 1.0 is out of the gate. But I still wanted to raise this issue, as I believe it could be a pretty big blocker to adoption of OpenFeature in Go.

[davejohnston]

I think this form of error handling is pretty standard across go libraries. There have been alot of debates about it, but I belive this is still the accepted "go idiomatic' way of handing errors.

That said the Error handler is a good idea, but I'd imagine that would be used as an optional feature rather than a replacement. If the user initialises it, the errors could be written to a channel as well as returned.

In terms of the actual call to boolVariation and the purpose of error. You could of course simply choose to ignore it in the normal way, so if you want to proceed without checking you can just carry on. The default value passed in will be used in the event of an error.

v2Enabled, _ := client.BooleanValue(context.Background(), "v2_enabled", true, openfeature.EvaluationContext{})

One of the advantages of returning the error, is that you then know you are receiving the default value (otherwise its unclear that you got a default value, or an actual evaluated value due to a rule). In the basic example above that doesn't make much sense, because the default is hard-coded as true, but its not uncommon to populate the default using a function, something like:

// getDefaultForFlag will check if an environment variable matching the flag name exists and use that
func getDefaultForFlag(flagName string, defaultVal bool) bool {
    // First check if a environment variables exists for this flagf
   myDefault := os.GetEnv(flagName)
   if myDefault != "" {
    // should probably check this before returning
       return myDefault
   }

   return defaultVal
}
v2Enabled, _ := client.BooleanValue(context.Background(), "v2_enabled", getDefaultForFlag("v2_enabled", true), openfeature.EvaluationContext{})

In this example, the defaultValue being used is decided at runtime. This is useful for unit testing, or to let developers who are working locally without an authenticated SDK to change flag values. The presence of the error will help distinguish that they got the default, or a actual evaluated value (of course boolVariationDetails will also do this, but with more detailed information on why).

[Kavindu-Dodan]

@dmathieu thank you for bringing this up and I think this is a valid concern. @davejohnston thank you for the historical background on this decision.

Even with this design decision, we still do not violate the specification Requirement 1.4.10 - Methods, functions, or operations on the client MUST NOT throw exceptions, or otherwise abnormally terminate. This is because there's no abnormal termination in the flow. Instead, I think the errors help to isolate errored evaluations.

For example, in Java, you have to check any evaluation error by first performing a detailed evaluation and then checking error in the detail,

  FlagEvaluationDetails<Boolean> details = client.getBooleanDetails(FLAG_KEY, false);
  if (details.getErrorCode() == null){
      // evaluation error
  } else {
      // evaluation success
  }

Such logic is required if you cannot rely on the default value alone and need to handle evaluation errors. And like @davejohnston said, you can ignore the error in Go (as well as in Java) if you can rely on the default value.

[dmathieu]

Thank you both for your replies.

The problem with ignoring errors is actually that they are ignored, and won't be handled.

At the same this, this is subjective, but when I retrieve the value of a feature flag, I should be able to just retrieve that value and have my code focused on what it's doing, not the flags. Having to handle errors every time makes flags less interesting to use, as they pollute the rest of the code. Hence my proposal to have a global handler which receives all errors and allows reporting them.

[Kavindu-Dodan]

Hence my proposal to have a global handler which receives all errors and allows reporting them

If you have not come across yet, you can utilize a custom error hook [1] to collect any evaluation errors in a central hook. For example, this could be an error logging hook. Do you think this solves your concern?

For example,

type errorHook struct {
    openfeature.UnimplementedHook
}

func (e errorHook) Error(ctx context.Context, hookContext openfeature.HookContext, err error, hookHints openfeature.HookHints) {
    slog.Log(ctx, slog.LevelError, fmt.Sprintf("OF error: %v", e))
}

func main(){
...
    openfeature.AddHooks(errorHook{})
}

[1] - https://openfeature.dev/docs/reference/concepts/hooks/#error

[dmathieu]

Thank you. I'll close this issue.