open-feature / ofep

A focal point for OpenFeature research, proposals and requests for comments
https://openfeature.dev
20 stars 15 forks source link

Invocation Context in Web SDK #33

Closed toddbaert closed 1 year ago

toddbaert commented 2 years ago

I've published an experimental version of the Node SDK that can be used in the web.

Experimentation and digging into some client SDKs has led me to conclude there's going to be potential performance footguns using the same interfaces we do on the server in the web client. Specifically, passing context to each flag evaluation on the client seems like it could potentially result in a lot of cache busting and server chatter in many SDKs. I'm leaning toward a relatively simple solution to this problem: don't allow context to be passed in flag evaluation calls in the evaluation API. Instead, context must be set on the client or globally. This seems reasonable in a web application, in which case the user (context) won't change for some duration (until logout, or some other action explicitly changing a user characteristic).

To put it plainly, instead of:

  getBooleanValue(flagKey: string, defaultValue: boolean, context?: EvaluationContext, options?: FlagEvaluationOptions): Promise<boolean>;

we'd have:

  getBooleanValue(flagKey: string, defaultValue: boolean, options?: FlagEvaluationOptions): Promise<boolean>;

The provider contract would not change - it would still receive the context object, it just will not have been merged with a context provided at invocation time.

We could also consider preventing before hooks from returning an altered context (allow them to return only void) for the same reason.

I'm particularly interested in hearing vendors thoughts on this one.

@davejohnston @InTheCloudDan @patricioe @dabeeeenster @agentgonzo @justinabrahms @beeme1mr

toddbaert commented 2 years ago

I should add, if we agree to go this route, we could add a condition in the spec that says for "single-user" applications, evaluation context need not be a parameter to flag evaluations.

justinabrahms commented 2 years ago

Why is there an issue with invocation-based context but not globally set context? Presumably the cache issues would be a problem any time someone passes any unique context, right?

My thinking is that cache busting seems like a provider implementation concern. If the provider has a local, on-device copy of the full rule-set, I don't see any concerns with clients passing invocation context. If they have it all on a remote service instead.. then normal web service concerns apply. They should document this for users and let them make their own decisions.

toddbaert commented 2 years ago

Why is there an issue with invocation-based context but not globally set context?

Globally set context represents static context. You could mutate it before every evaluation, but I think it would be an obvious misuse of the API.

Presumably the cache issues would be a problem any time someone passes any unique context, right?

Yes, that's the issue. Some vendor SDKs have explicit methods that must be called when the context changes on the client to bust this cache or call a server. Since the application author doesn't have access to this cache busting API, the provider must implement it somehow. This leaves the provider in a situation where they are doing something like hashing the context to see if it's changed, plus adding some kind of debounce or something like that.

Another thing we could do is potentially add some kind of cache busting API to client SDKs (perhaps we'd add a conditional requirement to the spec that would only apply to SDKs to be used for clients...).

My thinking is that cache busting seems like a provider implementation concern. If the provider has a local, on-device copy of the full rule-set, I don't see any concerns with clients passing invocation context. If they have it all on a remote service instead.. then normal web service concerns apply. They should document this for users and let them make their own decisions.

I think I see your point, and the current API just puts client side providers in a tough spot as it stands. We might need to do something like I mentioned above, but I'm really open to any solutions.

kinyoklion commented 2 years ago

For a provider which needs to have a context in order to get a flag configuration the current API doesn't provide a documentation only fix. At least not one which could have the same performance/latency characteristics as the vendor specific SDK.

The primary problem is decoupling the time a provider becomes aware of a context from the point of needing to evaluate a flag. If the provider doesn't have the context until the point that a flag is being evaluated, then that means the flag evaluation is going to have the worst case latency.

If the API could not include invocation specific contexts at all, then that would be a more ideal API for that kind of provider. API based fixes are going to require less technical support than the documentation ones.

justinabrahms commented 2 years ago

I'll concede that there may be performance issues w/ passing evaluation context in some vendor architectures. I don't think that is reason enough to change the interface. For groups like the one I mentioned above (all rules exist on client), they'll have a strictly less powerful system at hand.

The primary problem is decoupling the time a provider becomes aware of a context from the point of needing to evaluate a flag. If the provider doesn't have the context until the point that a flag is being evaluated, then that means the flag evaluation is going to have the worst case latency.

Would it be reasonable to have some sort of provider event when per-request eval context is created / changed? Sounds messy to implement, but interested in if that would solve the underlying concern.

I think I may be missing the heart of the concern here.

toddbaert commented 2 years ago

Would it be reasonable to have some sort of provider event when per-request eval context is created / changed? Sounds messy to implement, but interested in if that would solve the underlying concern.

This sounds like a possibility I was considering:

The SDKs that will have this issue require specific methods to be called for context changes to have an effect. If we simply exposed something similar in the evaluation API, I think it might solve the problem. Then users could call some kind of refreshContext() on the API, which would call the analogous method on the provider. For provides which don't have this paradigm, they simply wouldn't implement it.

I don't think an event being fired implicitly when context is changed (however we decide to define that) will help much here. The main issue is the implicated SDKs want context updates to be very "explicit" to avoid performance issues.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in next 60 days.

github-actions[bot] commented 1 year ago

This issue was closed automatically because there has not been any activity for 90 days. You can reopen the issue if you would like to continue to work on it.