Add flag-evaluation spec, Makefile, initial spec structure

[toddbaert]

First pass at a partial spec, based on cumulative findings from our research repo. Only the Evaluation API is proposed (specification/flag-evaluation/flag-evaluation.md, also note the associated assertion JSON file: specification/flag-evaluation/flag-evaluation.json). The best way to view the actual spec is probably to view the branch directly, since it's mostly markdown: https://github.com/open-feature/spec/tree/spec/flag-evaluation-api/specification

One of the goals was to design the structure of the spec, so I've included stubs for some of the other parts.

Please also checkout the specification/README.md, which contains an overview, conformance clause, and document statuses.

For ease of generating the JSON assertion files, I've also added a very simple Makefile. If you're interested in looking at spec only, please make use of the Github's file-type filter:

UPDATE:

I believe I've addressed all the concerns. I've also opened: #36 in response to comments about CI and various helpful automation suggestions. Please remember, everything in the spec at the moment is still status: experimental, so don't think that anything here is final!

[toddbaert]

Please note this. It seems like the optimal way to deal with (or rather, not deal with) this "flag-value/flag activity" impedance mismatch across providers.

[justinabrahms]

This seems surprising to me. This single API will tell you "Is this enabled in the UI" and also "is the flag evaluating to true"? Why complecting these two concerns?

[InTheCloudDan]

Is there additional information around this? I'm having some trouble parsing the use case that isn't handled internally in the provider.

[toddbaert]

@justinabrahms

This single API will tell you "Is this enabled in the UI" and also "is the flag evaluating to true"?

Not quite (unless I'm misunderstanding you)

The issue is that some providers allow logically boolean flags (enabled/disabled) WITH additional associated boolean payloads (every flag has a bool and an optional number/string/boolean payload). Because some providers work that way, I think it forces us to have this kind of resolution - we need a method for the boolean flag, and a separate method to get it's associated boolean payload.

In providers that DONT have this concept (most of them), isEnabled() and getBooleanValue() do the same thing, isEnabled() is just syntactic sugar.

I'm open to other solutions to the problem though.

[toddbaert]

@InTheCloudDan

Is there additional information around this? I'm having some trouble parsing the use case that isn't handled internally in the provider.

See: https://github.com/open-feature/spec/pull/35/files#r855378994. It is really internally handled by the provider, but I wanted to call it out here because it's relevant to the expected behavior of the evaluation API (we can remove it if it's confusing and only include it in the provider spec).

Maybe this proposal isn't the best solution, but @dabeeeenster has brought the issue up a few times, and this is the best I can come up with. Open to more suggestions though.

[dyladan]

Hmm thanks for the clarification. The wording here makes more sense with this in mind.

It was supporting this that I was attempting to do, but after this discussion, I'm wondering if it's not better to completely remove the isEnabled() method entirely, and let the Flagsmith provider determine how to handle resolving getBooleanValue in their case.

I'm supportive of this tbh. Seems very provider-specific

[beeme1mr]

I'm in favor of removing isEnabled(). In my opinion, it seems very unlikely that an application author would expect isEnabled() and getBooleanValue() to behave differently. Removing it would eliminate that confusion.

[dabeeeenster]

flagsmith.getValue('toddsFlag') which can return a boolean completely independent of the returned value of hasFeature above, true or false. @dabeeeenster please correct me here, if I'm wrong.

Correct.

This is something of a hangover from pure feature flags and a more complete flag and remote config solution. We wanted to give people a simple boolean state for a flag but then be able to enhance it with essentially a general String value, but then what if they store True in the string value, and how does that typing get supported through all the SDKs and their various type handling.

Hard to say what the best general solution is here. I think if the OpenFeature SDK can provide its best interpretation of the intention from the provider's interface but let developers have access to the base data is probably the optimal solution.

I believe most vendors have a similar approach of booleans and additional config/multi-variate values but I cant (and don't want to !) speak for them.

[beeme1mr]

If this functionality is there due to historical reasons, I propose that we do not add this to the spec. I think the expected behavior for OpenFeature application developers would be that a disabled feature flag does not have any meaningful auxiliary value. In this case, it would simply return the default value provided during the evaluation. However, the provider pattern is flexible enough to allow each vendor to decide how best to implement the interface. In this case, the getBooleanValue() could get the value from the state of the flag or from the getValue() method.

[toddbaert]

For now, I've removed the isEnabled() and isEnabledDetails() methods. We can always open up a new issue for this if we think it's something that's absolutely necessary, but I agree that the provider interface (spec to come) gives enough flexibility to address this in numerous ways.

[toddbaert]

These are all dependent on the outcome of the discussion here: #32 (comment)

[dyladan]

Error code was quite a contentious topic in opentelemetry which we went back and forth on for a long time. I would think about what usecase you're trying to solve by having an error code and see if you can solve it in a more explicit way. Maybe a "retryable" boolean?

[beeme1mr]

The main use cases we're focusing on is telemetry and debugging.

[toddbaert]

@dyladan What @beeme1mr says is correct. We believe it to be unlikely that anyone would write code based on this value. It's mostly useful for debugging purposes, and for monitoring the performance and availability underlying flagging system.

[toddbaert]

We also may want a section that specifically mentions that we encourage idiomatic use of language features, and that prescriptions that imply certain unavailable language features should be tolerated.

[justinabrahms]

This seems surprising to me. This single API will tell you "Is this enabled in the UI" and also "is the flag evaluating to true"? Why complecting these two concerns?

[InTheCloudDan]

Is there additional information around this? I'm having some trouble parsing the use case that isn't handled internally in the provider.

[dyladan]

I did not review the parser at all, but I looked pretty thoroughly at the spec. In general, I would characterize it as a bit too prescriptive and doesn't leave room for the idioms of different languages to be used in some places. This may lead to an API implemented in one language which feels like it doesn't really belong. Take all my suggestions as merely suggestions as I am an outsider here.

[moredip]

with these examples, it might be nice to provide an example that uses generics to avoid needing the type in the name, for example string myString = client.getValue<string>('string-flag', 'default');

[toddbaert]

This conditional requirement is really about using a type-aware function:

if:

The language type system differentiates between strings, numbers, booleans and structures.

then:

The client MUST provide methods for typed flag evaluation, including boolean, numeric, string, and structure.

Because some of our underlying providers have type safe methods (see the cloudbees provider) and so we can enforce type safety at runtime, I don't think we actually want a generic method in these cases. In the case of TS/JS in particular, the fact there's no type-safety at runtime causes a real problem. It leaves us in a situation where the only way to know which method to call on the underlying provider (.value() (string) vs .getNumber()(number), for instance) is to test the type of the detaultValue (but this could be passed as the wrong type in error). Actually, we may have similar problems in Java due to type erasure of generics.

See: https://github.com/open-feature/sdk-research/blob/main/packages/js-cloudbees-provider/src/lib/js-cloudbees-provider.ts

In summary, I think we need type-specific and type-safe methods like getBooleanValue() and therefor the kind of thing you suggest might be a bit redundant, and behave unpredictably, particularly in JS and languages where the generic type information is erased at runtime.

[moredip]

should this list include reason and error code - those are referenced in Requirement 1.14 and Requirement 1.16 lower down.

[toddbaert]

Again I've been ambiguous 😅 ... these are the parameters to the method, not the fields in the structure. I will rephrase to make that more clear. The structure is detailed in types.md, as well as below.

[toddbaert]

rephrased, let me know if that's clearer.

[moredip]

wouldn't they be called fields on the evaluation details structure, rather than parameters?

[toddbaert]

These are the parameters to the method, not the fields in the structure. I will rephrase to make that more clear.

[toddbaert]

rephrased, let me know if that's clearer.

[agentgonzo]

evaluation context is missing from this section

Suggested change

	### evaluation context
	Structured data, presented however is idiomatic in the implementation language, such as JSON or YAML.

[toddbaert]

I think we need to take some time to flesh out the Evaluation Context a bit better. Some flag systems have pre-defined fields, and we'll need to define some too. There's also the implicit language specific context-injection and propogation mechanisms that need to be mentioned (Go Context, JS async-local-storage, etc). I think the context probably requires it's own spec page. You'll see this PR actually has a stub md file for it: specification/evaluation-context/evaluation-context.md

[beeme1mr]

It looks like the major issues have been addressed now. I think it's time to merge it as is since nothing here is final. Any issues can be handled with a tickets and/or a PR.

[toddbaert]

Rebased, squashed, added co-authors.