[Question] mutation of provider property in OpenFeatureAPI instance

[thiyagu06]

The openFeatureAPI instance implemented as singleton. We're allowing the client of OpenFeatureAPI to mutate the provider property. It results in undesirable effect on the flag evaluation when we need to use the multiple providers.

I created a sample repo to demonstrate the side effects here. https://github.com/thiyagu06/openfeature-demo

The demonstrated logic is expected to evaluate the feature flag using https://github.com/thiyagu06/openfeature-demo/blob/main/src/main/java/com/openfeature/demo/provider/InMemoryFeatureProvider.java, but it's not happening because of the mutation.

Also, When implementing the dispose functionality open-feature/ofep#46, for every mutation of provider property, the dispose functionality needs to be called. If the logic inside the dispose method is an costly operation, we might hit with performance penalty as well.

My opinion on this is, we shouldn't allow mutation on the singleton instance to avoid any side effects.

[toddbaert]

@thiyagu06

You're right, but this is on purpose see 1.1.1 and 1.1.2. One of the main reasons for this is that we hope that eventually, somewhat similar to OpenTelemetry, 3rd party libraries will target the OpenFeature API, and will be able to use whatever globally configured provider the consuming application has configured. Without requiring configuration in the 3rd party library, this is only possible with a global singleton.

However, I definitely see the problem you are describing. In fact, I recently opened up this issue to discuss it (please feel free to comment on it, I would love your input). One thing that you can do right now is use a sort of "super-provider" that encompasses both the sources you want to use... but I understand that might not be an ideal solution for you.

I really would like to resolve this somehow. Please continue to contribute your opinion as we consider this problem.

[toddbaert]

@thiyagu06 Could you take a look at this?

[thiyagu06]

I'm happy that we're going to support multiple providers as per open-feature/ofep#56. My other concerns are about mutable objects present in different places (openFeature.java, OpenFeatureClient.java) which might cause side effects. We can take inspiration from open telemetry and try to do the same here as well. https://opentelemetry.io/docs/instrumentation/java/manual/. Ideally I expect the API to look like below.

OpenFeature openFeature = OpenFeature.builder()
                .setProvider(new FlagdProvider())
                .setProvider("split", new SplitProvider())
                .setEvaluationContext(new ImmutableContext())
                .setHooks(new OtelHook(), new MaskPIIDataHook())
                .build();

OpenFeatureClientBuilder client = openFeature.getClient("split")
                .setEvaluationContext(evaluationContext)
                .setHooks(new EvaluationResultLoggingHook())
                .build();

var isBetaUser = client.getBooleanEvalutation('beta-user', false);

[justinabrahms]

Here is the associated code in otel which manages the "you may only setup the global once"

https://github.com/open-telemetry/opentelemetry-java/blob/d95bc83542d06de6e5b119aac1ec49b7ba19a42b/api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java#L104

The resulting otel interface that you can get back doesn't have mutation support.

If we were to change this, it would result in a backwards incompatible change.

[toddbaert]

This spec addition, and this implementation in the java-sdk may resolve this issue. Please take a look @thiyagu06 .

[beeme1mr]

This should be resolved in the 1.4.0 release that includes mapped clients.

#364

[Kavindu-Dodan]

I am closing this issue as #364 is merged and related features are released with that.