Proposal: Add flag_enabled and variant (name) to EvaluationResponse

[markphelps]

While looking at implementing an OpenFeature provider for Flipt I've found a few areas where the EvaluationResponse doesn't always return 'enough' info to determine why a request has failed or not matched.

One scenario is if the flag is disabled:

flipt/server/evaluator.go

Lines 78 to 81 in 301ee60

	if !flag.Enabled {
	resp.Match = false
	return resp, nil
	}

Here we simply set Match = false and return, short-circuiting the rest of the evaluation logic. This doesn't really give the caller any context as to why there was no match. It could be that the flag is disabled as in this example, OR it could be that no constraints matched so there is no variant to return.

We've decided in the past that a request to evaluate a flag that is disabled should not return an error to the caller, but we still need a way to notify to the caller that the flag is disabled.

I have two proposed solutions in mind to solve this (as well as an additional option specific to implementing an OpenFeature provider):

Option 1: Add new field to the EvaluationResponse body of flag_enabled or flag_disabled

Here we would simply add flag_enabled (or flag_disabled if we wanted to take advantage of Go's defaults) of type boolean to the proto:

message EvaluationResponse {
  string request_id = 1;
  string entity_id = 2;
  map<string, string> request_context = 3;
  bool match = 4;
  string flag_key = 5; 
  ... skipped fields
  bool flag_enabled = 11; // or flag_disabled to take advantage of defaults

Then we would set flag_enabled/disabled to false (or true) if the flag was disabled, which would allow the user to imply that's why match == false.

Option 2: Add the entire Flag object to the EvaluationResponse

In this option, we would add the entire Flag object to the EvaluationResponse, however, it would not include all variants for brevity. This would be especially necessary for BatchEvaluationRequest s.

We might also consider deprecating flag_key if we went with this approach, as it would be present in the flag object.

message EvaluationResponse {
  string request_id = 1;
  string entity_id = 2;
  map<string, string> request_context = 3;
  bool match = 4;
  string flag_key = 5 [deprecated = true];]; 
 ... skipped fields
  Flag flag = 11;

Related Add variant (string) or variant (object) to EvaluationResponse

Related to implementing an OpenFeature provider, they allow for exposing the variant in the response. In our case value is the value of the variant, however we could set variant to be the name of the variant in Flipt if present (it's optional).

Depending on which option we choose for representing flag enablement, we could do a similar thing for returning the variant (either the name or the entire object) like so:

Variant Option 1: Adding variant or variant_name as a field

message EvaluationResponse {
  string request_id = 1;
  string entity_id = 2;
  map<string, string> request_context = 3;
  bool match = 4;
  string flag_key = 5; 
  ... skipped fields
  string value = 8;
  ... more skipped fields
  bool flag_enabled = 11;
  string variant = 12;

Variant Option 2: Adding variant as an object

message EvaluationResponse {
  string request_id = 1;
  string entity_id = 2;
  map<string, string> request_context = 3;
  bool match = 4;
  string flag_key = 5 [deprecated = true];]; 
  ... skipped fields
  string value = 8;
  double request_duration_millis = 9;
  ... more skipped fields
  Flag flag = 11;
  Variant variant = 12; 
}

Thoughts

Just typing this out I think I've determined my preference is to continue using flat values in the response and not begin adding in subobjects as it (IMO):

1. doesn't require us to deprecate any existing fields
2. would be less verbose in the response bodies
3. reduces some of the ambiguity compared to if we were to add the entire flag and variant objects in the response (ie: should the consumer use value or variant.key? and why are variants nil on the returned flag object?)
4. most of the fields on the objects themselves are not relevant at evaluation time (ex: do you care about the flag.created_at and flag.updated_at?)

I guess a third option would be to introduce another subtype to the EvaluationResponse like:

message EvaluationDetails {
   bool flag_enabled;
   string variant_name;
}

message EvaluationResponse {
  string request_id = 1;
  string entity_id = 2;
  map<string, string> request_context = 3;
  bool match = 4;
  string flag_key = 5 [deprecated = true];]; 
  ... skipped fields
  string value = 8;
  double request_duration_millis = 9;
  ... more skipped fields
  EvaluationDetails details 11;
}

Although this feels somewhat a bit weird too 🤷🏻

Anyways, this issue is mainly to gather feedback from end users to determine which approach they would prefer before coming up with a solution. Alternatively, if there are any other ideas on how to solve this, I'd love to capture those/discuss here a well!

Thank you for reading 📖

[GeorgeMac]

Nice write-up!

Another suggestion taken directly from OpenFeature's ResolutionDetails structure:

A Reason string / enum field:

Reason	Description
DISABLED	Value was derived due to the flag being in a disabled state
MATCH	Value was derived from a variant due to a match on the provided context
ERROR	Value is the result of an error

I agree with the sentiment you expressed on keeping this flat.

[markphelps]

Yeah I'm liking adding a reason enum