open-feature / spec

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

Previously created clients should always use currently registered provider #96

Closed toddbaert closed 2 years ago

toddbaert commented 2 years ago

I was making a change to the Node SDK to remove the provider accessor from the API (https://github.com/open-feature/spec/issues/84, https://github.com/open-feature/spec/pull/93) and in moving things around, I briefly changed behavior in a pretty nefarious way. Basically, I created a situation wherein clients maintained the provider that was registered at the time of their creation:

OpenFeature.setProvider(providerX)
clientA = OpenFeatire.getClient();
Openfeature.setProvider(providerY)
clientB = OpenFeatire.getClient();

clientA.getBooleanValue() // this was still using providerX
clientB.getBooleanValue() // this was using providerY

I think this is not good :tm: . It means authors would have to "keep track" of the creation time of certain clients, and causes a lot of other surprising knock-on effects. I don't think it technically violates our singleton requirement (the API object is still a singleton) but it feels like it violates it practically...

Should we mention the expectation that all clients use the currently registered provider in the spec? Is such a requirement already implied? Can we just add some verbiage to the singleton requirement?

@justinabrahms @davejohnston @beeme1mr

justinabrahms commented 2 years ago

I was actually thinking of a world where we had multiple clients, one for each provider. This would allow me to have a config-backed provider and a service-backed one. I might also have a single aggregate provider, but would need to do something w/ FlagEvalOptions to make it work.

Would love to better understand the use-case you're seeing w/ updating the provider to be different and have it reflect in existing clients.

toddbaert commented 2 years ago

I think this discussion might merit a meeting, either during our standing one, or something dedicated.

toddbaert commented 2 years ago

@justinabrahms

Thinking about what you wanted more, I think one (possibly nice) way to do that would be to utilize "namespaced" providers...

Basically, we'd add an optional namespace param to the setProvider() call:

OpenFeature.setProvider(new MyGlobalProvider()) // general provider

OpenFeature.setProvider(new MySpecificProvider(), "admin-pages") // admin-pages represents one logical section of some app that wants to use a different feature-flag control plane than the rest of the app

then:

OpenFeature.getClient({  nameSpace: "admin-pages" }) // returns a client associated with MySpecificProvider

Does this make sense? I think this makes the behavior pretty clear, the implementation would be very straightforward (maintain a single map of providers with keys being the namespace), it doesn't complicate basic use cases, and I think it supports your goal.

cc @beeme1mr

justinabrahms commented 2 years ago

I think I like that. My preference is we leave this ticket open until post-alpha bc this doesn't seem necessary to hit that goal and I want a stable spec to shoot for. :)

Or maybe we tag an alpha spec that I can work towards.

toddbaert commented 2 years ago

I'm going to close this now, and we can revive it if it comes up again... I think it's a bit of an edge case.