open-feature / ofep

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

Global providers and context #49

Closed toddbaert closed 1 year ago

toddbaert commented 1 year ago

Particularly with our upcoming support for client-side, I'm concerned about the global state (provider and global context) in our SDKs. We may want to provide some alternative APIs that don't rely on these global functions/state. They provide benefits, but also come with costs and risks.

Benefits:

Costs/Risks:

I'd like to explore solutions to this. One I've been considering is to simply add a setProvider method on the OpenFeature client. This would allow configuration of a provider simply for this client, not impacting the global API or other clients.

This is fairly straightforward in the server. In the case of the client, more might need to be considered. In our working client proposal, we don't support passing a context at evaluation time. Instead, the context is maintained on the global API object and changes to it have side-effects in providers (see here for details). If we'd like to provide alternatives to global state, we'd also need to allow for setting context on the client in client-side SDKs. To clearly scope the impact of changing the context in this case, I propose that we only allow the client context to be set only along with the client provider. The API might look like:

client.setProvider(provider, context); // pass optional provider and context

or perhaps when generating the client:

Client client = OpenFeature.getClient(provider, context); // pass optional provider and context. Here things are immutable as well.

This is just one idea... and perhaps my concerns are unfounded. I'm also not proposing removing any existing functionality, simply adding new non-global equivalents. Let me know your thoughts.

justinabrahms commented 1 year ago

I support adding client-side state and providers. I don't support removing global state which cascades down into the client.

As an example for a good use of global state, I want to include things like "Which environment is this deployed in?" or "Which server farm is this in?" as context in my calls.

toddbaert commented 1 year ago

I support adding client-side state and providers. I don't support removing global state which cascades down into the client.

As an example for a good use of global state, I want to include things like "Which environment is this deployed in?" or "Which server farm is this in?" as context in my calls.

OK. Ya, I think we should definitely keep the global stuff. I'm only advocating adding more functions. I think this is a good point though, I will add this to my initial comment.

moredip commented 1 year ago

Apologies, this is a bit of a stream of consciousness; I've been writing it for a while and want to hit Comment before I log off for the day!

As you know, I'm a proponent of getting away from the global state as much as possible - but agree with Justin that we should leave the option of adding some top-level global evaluation context that cascades down. My concern is more about other types of state, (and globaltons in general), rather than the evaluation context.

I'm focused on us making sure that the Application Author has the easiest possible way to get a flagging decision. Even asking them to create a client is, IMO, not great ergonomics. Asking them to also configure a provider would be really bad.

When I think about the ergonomics for Application Authors and Application Integrators, I believe we need:

a) an API that doesn't force an application author to think about the concept of a OF client unless it's relevant (essential complexity) to the situation. In the usual case, they should just be able to call getBooleanValue(...) and not have to figure out how to new up a client or otherwise get hold of one.

b) an API that doesn't force an application author to think about evaluation context unless it's relevant. In most situations that call to getBooleanValue should not require the caller to provide evaluation context, in either the static or dynamic paradigm. It should automatically use the "current" evaluation context - this would mean something different in the dynamic paradigm vs static paradigm, but in either case the application author shouldn't have to think about it unless they are in a situation where they are the ones that know some specific additional piece of eval context.

c) That part about using the "current" evaluation context also implies that we need some way for an application integrator to write code that adds server-side context for a limited scope (e.g. for the current request). I think the ergonomics for this API is less important than for the app author-facing API.

I suspect we can achieve the API described in (a) and (b) as a sort of sugar layer on top of an underlying API involving clients, scoped context, and so on, but of course that underlying API needs to be designed in such a way that this highly-ergonomic sugar API is possible.

My take is that In that underlying API we need some stateful object that tracks a specific evaluation context, and because of the way many OF providers work internally I think it makes sense for that stateful object to also manage a specific instance of an OF Provider - I think that would be a departure from how things work today.

image

I've not thought about hooks and how that set of state should work.

Another addition which I think we'd want to make in order to allow that sugar API would be to have some sort of ability to nest or scope clients, so that you could have a global client but then also spin off a per-evaluation scope that builds on top of that with some additional evaluation context. I might be wrong here though; there might be a better way to provide that sugar API that doesn't involve having to introduce the concept of nesting clients.

toddbaert commented 1 year ago

Relevant discussion in Java SDK.

toddbaert commented 1 year ago

Another thing we could do would be to register providers with an optional "name", and use that name to retrieve your indexed provider... this is still global state, but essentially you'd get the option to at least get clients associated with a particular provider, which might help with @thiyagu06 's case above.


Openfeature.getInstance().setProvider("my-provider", new MyProvider())

Client client = Openfeature.getInstance().getClient("my-provider");
toddbaert commented 1 year ago

@moredip

a) an API that doesn't force an application author to think about the concept of a OF client unless it's relevant (essential complexity) to the situation. In the usual case, they should just be able to call getBooleanValue(...) and not have to figure out how to new up a client or otherwise get hold of one.

I think this is potentially a job better handled by the DI framework of the app in question, but it also might be handled with something like I mention above:

Client client = Openfeature.getInstance().getClient("my-provider");

In this case, a provider "my-provider" was configured centrally beforehand, and the app author who wants to do evaluation can simply identify the client they want... I'm very interested in what other options you have in mind!

b) an API that doesn't force an application author to think about evaluation context unless it's relevant. In most situations that call to getBooleanValue should not require the caller to provide evaluation context, in either the static or dynamic paradigm. It should automatically use the "current" evaluation context - this would mean something different in the dynamic paradigm vs static paradigm, but in either case the application author shouldn't have to think about it unless they are in a situation where they are the ones that know some specific additional piece of eval context.

c) That part about using the "current" evaluation context also implies that we need some way for an application integrator to write code that adds server-side context for a limited scope (e.g. for the current request). I think the ergonomics for this API is less important than for the app author-facing API.

Agreed. I think the proposed transaction propagation functionality accomplishes this. A configurator could register the propagator, and additionally some middleware/request filters that would populate the context "transparently" in the scope of the continuation or thread handling that request.

thisthat commented 1 year ago

I also favor having a global context that describes some immutable properties of the application that won't change during its runtime.

One thing that I want to challenge is

lack of flexibility (two teams contributing components to the same webapp would not be able to use different providers)

Do we want to support this? Your point @toddbaert

configuration can be leveraged by 3rd party libraries targeting the evaluation API (a provider set globally will be used by evaluations in a 3rd party library without requiring any explicit openfeature configuration in that library)

is very valuable because it allows controlling libraries' behavior at runtime via FF. I suggest solving this use case by providing a MultiProvider provider implementation that does multiplexing of evaluations to multiple providers. If we want to support using a specific provider for a given client name, we would require to change the SDK spec and allow propagating Client metadata to the provider.

For handling the multiple contexts, did we already look into OpenTelemetry Resource? This will support us with:

toddbaert commented 1 year ago

For handling the multiple contexts, did we already look into OpenTelemetry Resource?

@thisthat It's not so much about the context implementation, in my view.

The complication is, especially in the client, the question of multiple contexts is directly tied to the question of multiple providers. In many client-side providers, the underlying SDK maintains state that maps 1:1 with our context. These SDKs are essentially singletons which require an explicit action to mutate their version of context. Without multiple providers, I don't see how we can have multiple contexts. We'd have to find some way of reconciling multiple OpenFeature contexts to a single provider's underlying stored state (I'm not confident there is a consistent, reasonable way to do this.)

lack of flexibility (two teams contributing components to the same webapp would not be able to use different providers)

Do we want to support this? Your point @toddbaert

While 3rd party library support is a great thing to have, and I don't want to lose it, the ability to "mask" multiple flag sources behind a single API has been an explicit use-case for OpenFeature since it's inception. We know some of our large enterprise adopters are in exactly this position. That said, it can be accomplished with an aggregate provider... it's just not as nice, IMHO. I still think we should keep the global singleton, I just wonder (especially in the client) if we want some other solutions for multiple providers/contexts as well.

moredip commented 1 year ago

I think this is potentially a job better handled by the DI framework of the app in question

In a world where everyone is using DI frameworks I would agree. In the world where I spend most of my time (JS/TS) that's not the case, and I would love to meet the application author where they are. That said, I could be persuaded on us not wanting to support a "client-less" sugar version of the API within the OF spec itself, as long as we left the door open for some implementations of the SDK (e.g. the JS implementation) to provide it as a non-standard addition on top.

, but it also might be handled with something like I mention above:

Client client = Openfeature.getInstance().getClient("my-provider");

I think this is better than expecting the Application Author to know exactly how to configure the provider, but it's still boilerplate that you have to put at the top of every file (if you're not using a DI framework). I almost think it would be better to not provide this half-solution, and that would encourage Application Integrators to write their own little helper methods that would let an Application Author get access to a configured client.

I think the proposed transaction propagation functionality accomplishes this.

Yep agreed - I'm excited to get that moved forward!

justinabrahms commented 1 year ago

I think the python logging framework (which I think cribbed heavily from java) got a lot of things like this right.

You can just log stuff using a root logger. logging.debug('message')

You can also have named loggers. lgr = logging.getLogger('myapp') which will allow you to do lgr.debug('other message'). From there, we could rely on language specific configuration mechanisms (including programatic) to either configure the root ~logger~ client, or the named ones.

This means libraries can use named loggers for their thing, and they could be configured to use some specific provider by the application author.

beeme1mr commented 1 year ago

Hey @justinabrahms, you please elaborate a bit more on how you think this pattern would work in OpenFeature? Would it work like @toddbaert described above?

justinabrahms commented 1 year ago

Not quite how Todd describes.

This is what Todd wrote:

Openfeature.getInstance().setProvider("my-provider", new MyProvider())

Client client = Openfeature.getInstance().getClient("my-provider");

Instead, I'd want this:

Library authors use a named client.

client = Openfeature.getClient('client-name-here')
client.getBooleanValue('my-key', false);

Application authors can either set global providers, or per-client providers.

Openfeature.setProvider(Providers.ROOT_PROVIDER, flagDProvider)
Openfeature.setProvider('client-name-here', envVarProvider)

It occurs to me as I write this that I may just be asking for "more global state".. but even in the version that Todd posted above, there's a global map for registration purposes. The question is 'should providers be configured at client usage?', to which I think I'd say no. They should be configured centrally.

moredip commented 1 year ago

@justinabrahms I love where you're going with the ergonomics described from the python logging thing. If we could get to the point that an app author could create a named client if they want, but also just do something like OpenFeature.getBooleanFlag('blah',false) in simple cases then I think we'd be in a good place.

Seems to me that having OpenFeature.getBooleanFlag just be sugar for OpenFeature.getClient('default').getBooleanFlag is a pretty reasonable approach 🤷

moredip commented 1 year ago

I also like @justinabrahms suggestion for how to configure different providers for different clients

Openfeature.setProvider(Providers.ROOT_PROVIDER, flagDProvider) Openfeature.setProvider('client-name-here', envVarProvider)

Seems simple and flexible

toddbaert commented 1 year ago

@justinabrahms is the main difference between what you describe here an what I described before with "named providers" that we would recommend library authors don't use the "root provider"?

I like that idea.

If we could get to the point that an app author could create a named client if they want, but also just do something like OpenFeature.getBooleanFlag('blah',false) in simple cases then I think we'd be in a good place.

@moredip

I'm not sure that the ergonomic gains here are huge. In my experience, many ORMs or libraries that require some network communication or I/O have concepts like a client, so I think it doesn't feel that unusual to people. More importantly, however, I worry about the technical implications of making evaluations global. That might imply more locks and more contention on fewer locks (see the locking in the go sdk and similar in dotnet). Multiple client instances mean multiple locks, which is better than a single global lock bottle-necking evaluations for an entire application. I imagine this sort of thing is why ORMs etc, usually leverage the concept of clients/pools etc, to have some simple abstraction to help with resource contention and to scope state reasonably.

Maybe my concerns are unfounded here. Interested in hearing from others on that point in particular.

I suspect @kinyoklion might have some valuable input on this one.

Regardless, it seems like there's some interest in the provider concept @justinabrahms described. Do you think we should create an OFEP? Or maybe just draft a spec PR and discuss there?

justinabrahms commented 1 year ago

@toddbaert My understanding of your version is you have people requesting providers by a string-y name? Maybe I'm misunderstanding.

This is what you wrote:

Openfeature.getInstance().setProvider("my-provider", new MyProvider())
Client client = Openfeature.getInstance().getClient("my-provider");

Which I think is equivalent to:

Openfeature.getInstance().setProvider("split-dot-io", new MyProvider())
Client client = Openfeature.getInstance().getClient("split-dot-io");

I don't think we want clients deciding which provider to use. Maybe that wasn't the intent?

I think it's worth making an ofep, if only to get some documentation about why the decision is being made.

toddbaert commented 1 year ago

Ah, yes, sorry, I missed the distinction. You're proposing that the we scope providers to particular clients by specifying a client-name when registering that provider, not by some provider identifier. Then the client name would be a key in a single global map, corresponding to a logical client/provider tuple. Correct?

I think I understand how that would be better for application authors, since it abstracts the provider configuration away from them. Thanks for clearing that up, assuming I understood it correctly.

justinabrahms commented 1 year ago

you got it. hopefully the ofep is clear. :)

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.