feat: provider state check

[liran2000]

According to the spec:

Requirement 2.4.5
The provider SHOULD indicate an error if flag resolution is attempted before the provider is ready.
It's recommended to set an informative error code, such as PROVIDER_NOT_READY if evaluation in attempted before the provider is initialized.

Problem:
Provider state check is implemented per provider.
Solution:
Provider state check is implemented at core SDK.

Notes

• Hooks:
  May be considered as some "behavior change", as in case of provider not ready, error is thrown, and hooks are not called, which is similar behavior to other errors handling.
• This is raised by several developers when implementing a provider, and suggested here by @ivarconr.
• Some providers implementation may have to adjust implementation and test for this. If implementation is not adopted, this is not expected to break something.

@toddbaert @Kavindu-Dodan @thomaspoignant

[toddbaert]

I think I like this as an idea, and maybe we could add a MAY recommendation for it. I'd like other's feedback. 🤔

[toddbaert]

@liran2000 there are other provider states as well (ERROR) for instance, and STALE is added in v0.7.0.

I don't think STALE is a problem for this idea - but if we're going to automatically check this state, should we check for the ERROR state as well? That seems difficult because we don't know the nature of the error. It seems inconsistent for the SDK to handle the one and not the other. What do you think?

@Kavindu-Dodan @lukas-reining @beeme1mr what do you guys think about this as a concept?

[Kavindu-Dodan]

@liran2000 there are other provider states as well (ERROR) for instance, and STALE is added in v0.7.0.

I don't think STALE is a problem for this idea - but if we're going to automatically check this state, should we check for the ERROR state as well? That seems difficult because we don't know the nature of the error. It seems inconsistent for the SDK to handle the one and not the other. What do you think?

@Kavindu-Dodan @lukas-reining @beeme1mr what do you guys think about this as a concept?

The way I see the spec section Requirement 2.4.5 is that this must be enforced by the provider but not by the SDK itself. The reason is that I see SDK being a facilitator for events but not necessarily an enforcer 🤔 IMO this helps OF SDK implementation and maintenance efforts. I think this idea match with what @toddbaert said - It seems inconsistent for the SDK to handle the one and not the other

[toddbaert]

The way I see the spec section Requirement 2.4.5 is that this must be enforced by the provider but not by the SDK itself. The reason is that I see SDK being a facilitator for events but not necessarily an enforcer 🤔 IMO this helps OF SDK implementation and maintenance efforts. I think this idea match with what @toddbaert said - It seems inconsistent for the SDK to handle the one and not the other

That's definitely the way the spec is currently written. I wanted to suggest we think about whether the spec might be enhanced here. I think I am leaning towards no, for the reasons already mentioned - but I agree it's tedious for the provider to handle this on it's own.

We could add this as a behavior in the EventingProvider abstract class to make it easier, as a "middle ground" 🤔

[liran2000]

Error state is handled by going to GeneralError, but yes, ERROR and STALE can be added to the code in a possibly adjusted way.

I will try to emphasize again the need for this, you can share if something is not accurate.
Currently each provider has to implement something like the following code block:

if (!ProviderState.READY.equals(state)) {
	if (ProviderState.NOT_READY.equals(state)) {
		/*
		 should be handled by the SDK framework, ErrorCode.PROVIDER_NOT_READY and default value
		 should be returned when evaluated via the client.
		 */
		throw new ProviderNotReadyError("provider not initialized yet");
	}
	throw new GeneralError("unknown error, provider state: " + state);
}

This create 2 problems:

1. The code is copied and duplicated per provider implementation.
2. It is not trivial and clear whether this code should be implemented by throwing errors, or by returning evaluation with relevant state/status. This question raised by several developers, recent example from here, and I also encountered it where it was clear to me when implementing a provider.

This solution comes to solve these problems.

Possible down side of the solution is to have less control of flow on these states, so I understand the conflict here.

Do you possibly have some other ideas for these problems ?

[toddbaert]

I will try to emphasize again the need for this, you can share if something is not accurate. Currently each provider has to implement something like the following code block:

if (!ProviderState.READY.equals(state)) {
	if (ProviderState.NOT_READY.equals(state)) {
		/*
		 should be handled by the SDK framework, ErrorCode.PROVIDER_NOT_READY and default value
		 should be returned when evaluated via the client.
		 */
		throw new ProviderNotReadyError("provider not initialized yet");
	}
	throw new GeneralError("unknown error, provider state: " + state);
}

This create 2 problems:

1. The code is copied and duplicated per provider implementation.
2. It is not trivial and clear whether this code should be implemented by throwing errors, or by returning evaluation with relevant state/status. This question raised by several developers, recent example from here, and I also encountered it where it was clear to me when implementing a provider.

I totally agree this is not ideal, and there is a lot I like about this proposal. I would like a solution, but the solution can't introduce more problems, and I'm worried this one might (but I'm not sure).

Possible down side of the solution is to have less control of flow on these states, so I understand the conflict here.

Error state is handled by going to GeneralError, but yes, ERROR and STALE can be added to the code in a possibly adjusted way.

Yes. The problem I see is that we also need to handle ERROR state to be consistent - but if we do that, how can the provider give more details about the error? If the SDK just throws GeneralError we have less information from the provider than we do now. Maybe that's OK? The provider can log things, of course.

Basically, the more logic we move into the SDK, the less providers have to implement (good), but the less control we give providers (bad). Striking this balance is hard.

[toddbaert]

I'm closing this for now. I'm totally willing to revive this discussion, but I think we want to leave it out for the time being.