Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add reasons, table #140

Merged
merged 1 commit into from
Sep 23, 2022
Merged

add reasons, table #140

merged 1 commit into from
Sep 23, 2022

Conversation

toddbaert
Copy link
Member

@toddbaert toddbaert commented Sep 22, 2022

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:

  • adds the reasons previously in a non-normative section into a normative section
  • adds a table explaining each reason

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.

⚠️ 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:

  • we don't expect developers to write logic around Reaons; they are for logging/debugging/telemetry
  • most reasons fall into these buckets, but complex providers may want more and in those cases fidelity is lost

@kinyoklion
Copy link
Member

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
Copy link
Member Author

toddbaert commented Sep 23, 2022

@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
Copy link
Member

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
Copy link
Member

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 toddbaert marked this pull request as draft September 23, 2022 14:05
@toddbaert
Copy link
Member Author

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:

  • we don't expect developers to write logic around Reaons; they are for logging/debugging/telemetry
  • most reasons fall into these buckets, but complex providers may want more and in those cases fidelity is lost

Signed-off-by: Todd Baert <toddbaert@gmail.com>
@toddbaert
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants