open-feature / spec

OpenFeature specification
https://openfeature.dev
Apache License 2.0
639 stars 34 forks source link

Add evaluation context spec #60

Closed toddbaert closed 2 years ago

toddbaert commented 2 years ago

First pass at evaluation context.

Lot's of questions here. I opted not to leverage something like the elastic common schema for now, I believe it might be overkill. I used it, as well as OIDCs standard claims only as inspiration.

justinabrahms commented 2 years ago

https://github.com/open-feature/java-sdk/pull/5 - javasdk pr around eval context. May be useful to look at for folks, especially around large number of setters/getters on it for attributes.

toddbaert commented 2 years ago

@agentgonzo:

A quote from you:

If a vendor requires a given field that is not specifed in this context, it can be part of the contract for that specific provider to provide that extra field.

It's "contracts" between application author and provider that scare me a bit. App author should be mostly able to completely ignorant of the provider in use, ideally. In a sense, contract between these parties reduces portability. If provider-X uses ip and provider-Y uses remote_addr then to switch between them you are re-writing some code. If we standardize these fields this is much less likely to happen, at least with "commonly used" fields. Maybe I'm overly concerned about this :shrug:

agentgonzo commented 2 years ago

I might be overthinking it. I got a bit confused by the concept of "MUST provide an optional field". You're right in that for common fields, we should have a consistent way of naming that concept (ip vs remote_addr as you say), so yeah, I do now see the reason for these fields. (However, I'm not convinced by the need for the ip field - but it's not one that CloudBees uses as a named field, so it may be something I am ignorant of).

In short, I retract my objection to these fields 👍

toddbaert commented 2 years ago

After much discussion, and input from various vendors, I think perhaps we should remove all pre-defined fields except targeting key. We will discuss this in the community meeting tomorrow, and hopefully get some consensus here.

Sorry for all the thrashing on this one. :grimacing:

toddbaert commented 2 years ago

I think we're in a place to merge this. It seems like based on the community meeting, we're on the same page now about not defining any default fields except targeting key. There's still some challenges with that one in particular, but they deserve an issue outside this PR: https://github.com/open-feature/spec/issues/82

I'll be squashing and merging this today unless I hear objections or concerns!