open-feature / spec

OpenFeature specification
https://openfeature.dev
Apache License 2.0
666 stars 35 forks source link

add reasons, table #140

Closed toddbaert closed 2 years ago

toddbaert commented 2 years ago

~Many SDKs have implemented these reasons as enums or limited strings, they are not free-form. I also think we can be reasonably confident most evaluation reasons can be bucketed into one of these.~

~This PR:~

~If we think that reasons should be free-form, we should make changes in SDKs to support this. If you think a reason is missing, feel free to add it or make a case in a comment.~

:warning: UPDATE: I've updated this PR. I think the consensus is to have an extensible set pre-defined strings for Reasons. See the thread for more reasoning, but TLDR:

kinyoklion commented 2 years ago

I am not sure that I see strong benefit in enumerating them. It really depends on what the use case for the reason is seen to be at all.

If the reason is to help a developer understand why they got the value they got, so they can presumably debug it, then the more specific it can be the better. Different providers have different pipelines and can be more specific about the part of their pipelines involved in getting a result.

If we expect someone to be able to respond in some dynamic way to these conditions, then it would make sense for it to be enumerated.

toddbaert commented 2 years ago

@kinyoklion Agreed. I think the reason is most valuable in hooks that do telemetry/logging and for debugging purposes. I'd not expect people to write logic around particular reasons. This is why the spec was less prescriptive before. However, SDKs have implemented these are enums, so I was happy to button up the inconsistency.

Let's see what others say. This is a relatively minor issue but I'd like to get it remedied as we head toward a possible 1.0 of some SDKs.

thomaspoignant commented 2 years ago

I would suggest to make it more open. It is convenient to use a enum but it means that you have some mapping to do, and multiple cases will end up with the same reason.

I will suggest to have a string field instead.

agentgonzo commented 2 years ago

I think that for consistency it's nice to have this set of standard reasons. But I think it's important for this standard list to be extended by providers if necessary if there is another 'reason' that has not been thought of beforehand (it also allows for adding an new reason to the list without having to re-release the SDKs with a breaking change. So I agree that the reason should be implemented as a string rather than an enum.

However, I see value in providing these 'standard' reasons as a const defined in the openfeature SDKs. Then the provider SDKs can just use these constants (and also getting consistency across providers) rather than having to re-define them in for every provider (or worse, hard-coding the same string literal throughout the codebase).

toddbaert commented 2 years ago

I will update this PR. I think the consensus is to have an extensible set pre-defined strings for Reasons. See the thread for more reasoning, but TLDR:

toddbaert commented 2 years ago

@agentgonzo @thomaspoignant @kinyoklion I've updated the PR. Please re-review.