Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE] Evaluation Context behavior is suprising #939

Closed
jhollanderpax8 opened this issue May 16, 2024 · 5 comments · May be fixed by #942
Closed

[FEATURE] Evaluation Context behavior is suprising #939

jhollanderpax8 opened this issue May 16, 2024 · 5 comments · May be fixed by #942

Comments

@jhollanderpax8
Copy link

The typescript examples in the OpenFeature docs show this snippet:

// add a value to the context
await OpenFeature.setContext({ myUserData: 'myUserValue' });

// the context is used for all feature flag evaluations automatically.
const boolValue = await client.getBooleanValue('boolFlag', false);

Leading one to believe that a similar approach would work with the Java SDK, however this is not true and it's very surprising behavior if you are used to things like MDC context in Log4j.

One would expect this to work:

OpenFeatureAPI api = OpenFeatureAPI.getInstance();
Map<String, Value> transactionAttrs = new HashMap<>();
transactionAttrs.put("userId", new Value("userId"));
EvaluationContext transactionCtx = new ImmutableContext(transactionAttrs);
api.setTransactionContext(apiCtx);

api.client.getBooleanValue("my-flag", false)

but instead you must do

OpenFeatureAPI api = OpenFeatureAPI.getInstance();
Map<String, Value> transactionAttrs = new HashMap<>();
transactionAttrs.put("userId", new Value("userId"));
EvaluationContext transactionCtx = new ImmutableContext(transactionAttrs);
api.setTransactionContext(apiCtx);

api.client.getBooleanValue("my-flag", false, api.getEvaluationContext)

Which IMO is surprising and inconsistent behavior and makes context propagation less than useful.

@Kavindu-Dodan
Copy link
Contributor

@jhollanderpax8 I think this is related to #940. Like I said in my response 1 OpenFeature defines multiple context levels (ex:- API, transaction, client, evaluation). And each of them are merged prior to a flag evaluation.

If you intend to set an API level context, that is valid to all underlying entities derived from OpenFeature API (ex:- clients, hooks ), then you can use example below as a reference,

  OpenFeatureAPI api = OpenFeatureAPI.getInstance();
  api.setEvaluationContext(new ImmutableContext(Map.of("foo", new Value("Bar"))));

  final Client client = api.getClient();
  client.getBooleanDetails("booleanKey", false);

Here the API context foo:bar is available for the booleanKey flag evaluation and you don't have to use api.getEvaluationContext. I hope this helps to clear your doubt.

Footnotes

  1. https://github.com/open-feature/java-sdk/issues/940#issuecomment-2115745803

@jhollanderpax8
Copy link
Author

@Kavindu-Dodan thanks for the reply. In my testing this didn't seem to be true. I was uncertain if I should file it as a bug because it wasn't 100% clear from the docs that it should be. I will make sure I can reproduce it in a small test and then open a bug report if I can.

@Kavindu-Dodan
Copy link
Contributor

@jhollanderpax8 sure feel free to. Please also check the the SDK version. 1.8.0 is the latest release.

@toddbaert
Copy link
Member

toddbaert commented May 17, 2024

@jhollanderpax8 I think part of the issue here is we are missing some Javadoc. We also are missing a very specific test for setting global context (it's covered, but only as part of other tests).

I've opened a PR that adds missing javadocs, and also adds a test specifically asserting that global context is used in evaluations.

cc @Kavindu-Dodan

@jhollanderpax8
Copy link
Author

jhollanderpax8 commented May 24, 2024

I was able to confirm that it works as expected and the error was in some Kotlin extension function hackery I was doing. I still consider ImmutableContext to be awkward in light of ThreadLocalTransactionPropagator, but I am likely going to likely using Kotlin extension functions to make the API more ergonomic as the basic abstractions are solid if awkward to work with. Thanks the clarification and documentation updates/test cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants