open-feature / spec

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

Optional Targeting Key Handling #82

Closed beeme1mr closed 2 years ago

beeme1mr commented 2 years ago

Problem

The spec defines an optional targeting key property in the evaluation context. This can be problematic for some providers that require a consistent value that represents a target (i.e. user). This is typically handled by requiring a targeting key to be provided during a flag evaluation. OpenFeature allows context to be merged and an application developer may choose to set the target key elsewhere in the application (i.e. auth middleware). That means OpenFeature can't guarantee that a targeting key is available when a flag is being evaluated.

Proposal

The issue can be mitigated by:

InTheCloudDan commented 2 years ago

For requiring a context with a key, if a user moves from a provider that does not require it, to a provider that does, there is a risk of returning a different evaluation even given no targeting rules as the default value will be returned.

If a user moves from a provider that requires a key to one that does not, that same risk does not occur as the key will just be ignored if I'm understanding correctly.

toddbaert commented 2 years ago

@InTheCloudDan your assertion is correct, as far as I can tell.

My feeling is that we don't want to require a targeting key for flag evaluation.

The only alternative I see if we want to avoid a hypothetical LD provider defaulting would be to either make the targeting key or the entire context required in the OpenFeature API, which I'd like to avoid, as it makes the most basic flag evaluation use-case (a simple, boolean, context-less evaluation) more complex.

beeme1mr commented 2 years ago

I'm all for requiring a targeting key but I'm not sure how we can do this in a clean way. The problem stems from context propagation and merging. If you're able to set the targeting key at various touchpoints (i.e. client, transaction, flag eval) how can we guarantee that a targeting key was defined?

toddbaert commented 2 years ago

If you're able to set the targeting key at various touchpoints (i.e. client, transaction, flag eval) how can we guarantee that a targeting key was defined?

I don't think there's a way to do so at compile time. I suspect it has to be at runtime, which is where we get back to providers who require it defaulting.

InTheCloudDan commented 2 years ago

It will be less than ideal from a support and interoperability perspective because it is not just switch from one provider to another at that point.

I understand the goal and associated headaches with context being set in multiple places, I'd like to understand how we can make it fail/warn as loudly as possible so users can be aware without a negative experience.

toddbaert commented 2 years ago

I'd like to understand how we can make it fail/warn as loudly as possible so users can be aware without a negative experience.

I think "for now" the answer is simply logging to stderr. Long term, we will want to think about a more well-defined, optional logging interface (see this issue).

I don't think this issue is fundamentally different than using a wrong flag key (maybe I mis-typed welcome-mesage when I meant welcome-message) - this would result in the same defaulting and resultant troubleshooting as missing a targeting key. As long as the provider throws an helpful error, and logs it, I think that's enough. The spec has some features for doing this kind of troubleshooting already. In this situation, I would expect the provider to set the reason indicating an error (as in https://github.com/open-feature/spec/blob/main/specification/flag-evaluation/flag-evaluation.md#requirement-117) and the error code to something like `"NO_TARGETTING_KEY" (as in https://github.com/open-feature/spec/blob/main/specification/flag-evaluation/flag-evaluation.md#requirement-115). I'd hope that would make this problem relatively easy to troubleshoot.

@beeme1mr @InTheCloudDan @patricioe

patricioe commented 2 years ago

I think that works for us so far. I can't think of another elegant way to support both worlds (providers that required a key vs not)