open-feature / dotnet-sdk

.NET implementation of the OpenFeature SDK
https://openfeature.dev
Apache License 2.0
61 stars 17 forks source link

feat!: internally maintain provider status #276

Closed toddbaert closed 1 month ago

toddbaert commented 2 months ago

This PR implements a few things from spec 0.8.0:

Fixes: https://github.com/open-feature/dotnet-sdk/issues/250

kinyoklion commented 2 months ago

Maybe more related to the spec than to this specific implementation. But I did find it somewhat hard to handle the runtime status after initialization status when the SDK automatically implements ready.

I think it makes it easy for simple providers, and somewhat cumbersome for more complex providers. I would actually prefer to be able to opt out.

Basically an underlying SDK may already emit these events, but we have to connect and translate these events conditionally around initialization. So after initialization if a provider fails we want to emit that event, but if during initialization it fails we need to not emit that event from the provider because the SDK also will emit it.

Example of attempting to reconcile this in a python provider: https://github.com/launchdarkly/openfeature-python-server/blob/243a8dc515bcd16fe8c7d5a0913fbdfe6e073aa0/ld_openfeature/provider.py#L46

toddbaert commented 2 months ago

So after initialization if a provider fails we want to emit that event, but if during initialization it fails we need to not emit that event from the provider because the SDK also will emit it.

I can see the issue here. We have some similar logic in flagd since it also supports automatic reconnection. I think for the majority of providers though, such events around reconnection etc, are not supported; it does indeed seem to be something featured in the most rigorous implementations.

I'm open to some kind of opt-out for emitting events on READY, but I'm not sure if the ROI is there. :thinking: I suppose it would be fairly easy to implement. :thinking:

kinyoklion commented 2 months ago

So after initialization if a provider fails we want to emit that event, but if during initialization it fails we need to not emit that event from the provider because the SDK also will emit it.

I can see the issue here. We have some similar logic in flagd since it also supports automatic reconnection. I think for the majority of providers though, such events around reconnection etc, are not supported; it does indeed seem to be something featured in the most rigorous implementations.

I'm open to some kind of opt-out for emitting events on READY, but I'm not sure if the ROI is there. 🤔 I suppose it would be fairly easy to implement. 🤔

I think an opt-out would work.

I've not thought about it extensively, but another option may be to have automatic events be part of a base class that more complex providers could choose not to use instead of it being in the API. Though that path isn't the most sustainable unless you have a language that supports more "mixin" type classes. So maybe not.

Maybe also we can leave it as is and think if the situation can be improved. Overall I feel like getting initialization correct has become more complex, so maybe the abstraction needs adjusted some.

toddbaert commented 2 months ago

So after initialization if a provider fails we want to emit that event, but if during initialization it fails we need to not emit that event from the provider because the SDK also will emit it.

I can see the issue here. We have some similar logic in flagd since it also supports automatic reconnection. I think for the majority of providers though, such events around reconnection etc, are not supported; it does indeed seem to be something featured in the most rigorous implementations. I'm open to some kind of opt-out for emitting events on READY, but I'm not sure if the ROI is there. 🤔 I suppose it would be fairly easy to implement. 🤔

I think an opt-out would work.

I've not thought about it extensively, but another option may be to have automatic events be part of a base class that more complex providers could choose not to use instead of it being in the API. Though that path isn't the most sustainable unless you have a language that supports more "mixin" type classes. So maybe not.

Maybe also we can leave it as is and think if the situation can be improved. Overall I feel like getting initialization correct has become more complex, so maybe the abstraction needs adjusted some.

I thought a bit about an opt-out. I think we could add some fields/metadata to the provider to signal this and possibly other behaviors. Perhaps we could open an new issue to discuss this in the spec.

toddbaert commented 2 months ago

I had to rebase this and resolve conflicts on the recently merged concellationToken/Async chnage.