Skip to content

Conversation

@fahad19
Copy link

@fahad19 fahad19 commented Aug 31, 2023

This PR

Introduces a new OFEP as per a previous discussion on Slack.

Proposes a new model for features, breaking it down into 3 kinds of values all scoped under a single feature:

  • feature flag
  • variation
  • variables

Notes

This is being proposed to help open up the possibility to create OpenFeature providers for Featurevisor: https://featurevisor.com/

SDK docs showing examples of the proposed feature model:

Further guides for the concepts introduced:


I understand this proposal can be very different than your overall vision around feature flags, but I feel it's worth raising it to you all because it enables many other tools (both Open Source & SaaS) to create OpenFeature Providers then ❤️

Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @fahad19, great proposal! Most of what you proposed is already technically possible but may benefit from more user-friendly method names. For reference, the method names were originally inspired by this API comparison.

const featureKey = "my_feature";
const defaultState = false;

const isEnabled = client.isEnabled(featureKey, defaultState);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you see this as syntactic sugar for the existing getBooleanValue method, or would this represent an entirely new value?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we consider features to contain a single value only, then yes it's a syntatic sugar.

but if we wish to establish three separate kinds of values, like:

  • flag status (boolean)
  • variation (string)
  • variables (key/value pairs with different types of values)

then .isEnabled() ends up with a meaning of its own, which evaluates the flag's status as boolean.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it would be very difficult to have something meaningful here cross-vendor. This feels more like the "why" that would be associated with the reason or meta-data associated with a flag evaluation. (The detailed evaluation reason would be "on").

I don't think there is a consistent idea of what "on" would be, or if all vendors would have such a concept.

const featureKey = "my_experiment";
const defaultVariation = "control";

const variation = client.getVariation(featureKey, defaultVariation);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this differ from the getStringValue method?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the intent is different.

imagine having both variations and variables under the feature, which are of both string type.

in that case, how can we let the provider know what kind of value to compute?

const variableKey = "showSidebar";
const defaultValue = false;

const showSidebar = client.getVariableBoolean(featureKey, variableKey, defaultValue);
Copy link
Member

@beeme1mr beeme1mr Aug 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the concept, especially with the type enforcement at the variable level. We can do something similar, but your proposal may be a nicer dev experience.

Here's what's already possible.

const featureKey = "my_feature";
const defaultValue = { showSidebar: false };

const { showSidebar } = client.getObjectValue(featureKey, defaultValue);

In TypeScript, it would even be possible to register a hook that validates the object before returning it to the client.

import { z } from "zod";

const myFeatureSchema= z.object({
  showSidebar: z.boolean(),
});
type MyFeature = z.infer<typeof myFeatureSchema>;

const featureKey = "my_feature";
const defaultValue: MyFeature = { showSidebar: false };

class Validator {
	constructor(private readonly zodSchema) {}

	after(_, evaluationDetails) {
		this.zodSchema.parse(evaluationDetails.value);
	}
}

const { showSidebar } = client.getObjectValue<MyFeature >(featureKey, defaultValue, undefined, { hooks: [new Validator(myFeatureSchema)] });

NOTE: This was written from memory and has yet to be tested.

The second example is fairly complex but would offer runtime variable validation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

client.getObject() can work, but this will require the provider to compute all the variables together and return it as an object, which application can destruct from.

the proposed API in document suggests we evaluate individual variables scoped under each feature on-demand.

also when the method name specifically says getVariable*, it shows the intent and the kind of desired value as well. We want the "variable" to be evaluated with that specific key.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I had discussed this in another OFEP or spec comment, but I think that the idea of a "scoped" feature could be added and would probably be more widely applicable.

Variables for this system seem closer to what variations may generally be from other systems (multiple types, potentially objects), and you would use several "flags" to accomplish something similar in those systems (or one structured flag containing the relevant data).

Here they are together in that a flag contains a few more states that are exposed.

If you were to imagine this at a very basic level it would be something like.

"myFeature" -> Variation (with on/off as details of that evaluation). Maybe this means some extra work is done.

"myFeature:showSideBar" -> Variable.

Being strings is for illustrative purpose, but if there is true need, then some more specific way could be devoted to support the idea.

Currently I think this is divergent enough that it would create a lot of dead API surface for most providers. There may be a way to make it into something more uniform though. How do we adjust the API so that two providers are not completely divergent from an API usage perspective?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you were to imagine this at a very basic level it would be something like.

"myFeature" -> Variation (with on/off as details of that evaluation). Maybe this means some extra work is done.

"myFeature:showSideBar" -> Variable.

Being strings is for illustrative purpose, but if there is true need, then some more specific way could be devoted to support the idea.

This is an interesting idea.

Currently I think this is divergent enough that it would create a lot of dead API surface for most providers.

I haven't had time yet to really dig into this proposal, but I think as it stands, this is my biggest concern with it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the comments!

scoped feature would solve more than half of the challenges I am having creating a new provider.

Variables

It would also be more like naming convention based like you suggested, without requiring any further development from OpenFeature's side:

// regular
client.getBooleanValue("myFeatureKey");

// scoped
client.getBooleanValue("myFeatureKey:myVariableKey");

Variations

for the variations part, OpenFeature provider-based API usage could be using myFeatureKey:variation.

another challenge would be emitting an "activation" event when an Experiment has been exposed to end users. For that the Context API can be abused a bit just to make it work somehow (just thinking out loud):

client.getStringValue("myFeatureKey:variation", "control", {
  // ... rest of context
  "$activate": true,
});

isEnabled

given we adopted the convention as mentioned above, the proposed .isEnabled() method can be used as:

client.getBooleanValue("myFeatureKey");

and it will not clash with any variables, because if variables were intended the user was expected to pass the key as myFeatureKey:someVariable instead.


Question

is this a convention that we wish to align on and document somewhere at OpenFeature level?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could bubble up events using hooks. A hook can be configured to run after each flag evaluation and even be included in the provider itself. It's also possible to pass "hook hints" at evaluation time. These can be used in a hook to customize the behavior even further.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A hook can be configured to run after each flag evaluation

this is what I am specifically trying to avoid. we should not emit an event at the time of evaluation.

the event should be emitted only when the application thinks the user has been exposed to that feature/experiment, manually.

Copy link
Member

@jonathannorris jonathannorris Sep 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey just adding my voice to this as we @ DevCycle have a somewhat similar hierarchy to what's described here, having Features as our top-level organizational object that contain multiple variables (flags) underneath: https://docs.devcycle.com/introduction/core-concepts/feature-hierarchy
https://docs.devcycle.com/sdk/server-side-sdks/node/node-openfeature
https://github.com/DevCycleHQ/js-sdks/blob/main/sdk/openfeature-nodejs-provider/src/DevCycleProvider.ts

We generally have learned that the vast majority of folks want to just interface with flags (variables) as the main interface to our SDKs. There are very few folks who need the information about the feature + variation data at run-time, mostly that data is just needed for reporting / eventing. That is certainly something we've struggled with ourselves with OpenFeature is how to get the variation + feature metadata attached to the resolution details.

I think a better approach here would be to figure out how to keep the core interface of the OpenFeature spec the same, where users are interfacing with different types of flags (variables), but exposing the feature + variation data as part of the Resolution Details / Detailed Evaluation data.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey just adding my voice to this as we @ DevCycle have a somewhat similar hierarchy to what's described here, having Features as our top-level organizational object that contain multiple variables (flags) underneath: https://docs.devcycle.com/introduction/core-concepts/feature-hierarchy https://docs.devcycle.com/sdk/server-side-sdks/node/node-openfeature https://github.com/DevCycleHQ/js-sdks/blob/main/sdk/openfeature-nodejs-provider/src/DevCycleProvider.ts

We generally have learned that the vast majority of folks want to just interface with flags (variables) as the main interface to our SDKs. There are very few folks who need the information about the feature + variation data at run-time, mostly that data is just needed for reporting / eventing. That is certainly something we've struggled with ourselves with OpenFeature is how to get the variation + feature metadata attached to the resolution details.

I think a better approach here would be to figure out how to keep the core interface of the OpenFeature spec the same, where users are interfacing with different types of flags (variables), but exposing the feature + variation data as part of the Resolution Details / Detailed Evaluation data.

This is an interesting perspective, and an appealing idea, to me at least.

@fahad19 do you think we could add well-defined properties to the detailed evaluation methods that would help? value would still maintain its current semantics (variable in DevCycle language as @jonathannorris mentions), but we could perhaps add additional properties that would satisfy your use-case, while remaining coherent with systems that don't have such concepts.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @toddbaert and @jonathannorris for all the inputs <3

I don't think the discussion here regarding activation will go anywhere, except for pushing it to the userland entirely.

I will close it with a summarized comment in a bit.

- key (`String`)
- Value (`Boolean`, `String`, `Number`, `Object`)

**Note**: It is worthwhile to expand the `Number` type into more explicit types like `Integer` and `Double`, especially when dealing with SDKs in strongly typed languages.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already supported in strongly typed languages.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove this line soon.

@beeme1mr beeme1mr requested review from a team September 1, 2023 15:49
@moredip
Copy link
Member

moredip commented Sep 6, 2023

my 2 cents is that if there's a way we can achieve this in "userland" (e.g. using "foo:bar" key names with semantics that the provider understands) then we should strongly prefer that over changing/extending the spec

@fahad19
Copy link
Author

fahad19 commented Sep 12, 2023

my 2 cents is that if there's a way we can achieve this in "userland" (e.g. using "foo:bar" key names with semantics that the provider understands) then we should strongly prefer that over changing/extending the spec

@moredip: you are very much right in suggesting if we can a solution in the userland for scoped variations/variables like using foo:bar as keys.

I like this suggestion. and likely creating a new Featurevisor provider for OpenFeature following this pattern.

my response would be the same if I were an OpenFeature maintainer.

the only remaining thing for me is to figure out the "activation" event to be triggered at will, which is discussed in above thread.

@fahad19
Copy link
Author

fahad19 commented Sep 18, 2023

summary:

  • for scoped variables, we can follow featureKey:variableKey pattern, keeping the OpenFeature API the same
  • for experiment activation (which are not always handled at the time of evaluation, but when application decides when it has been exposed to the user visually), we can delegate that to the userland (application) without touching OpenFeature SDKs in any form

really appreciate all the inputs <3

I would have defended the OpenFeature API the same way to not have any further tweaks in the API to accommodate Featurevisor or any other solutions.

will take the learnings from here and build the provider at some point.

thanks everyone! 🙌

@fahad19 fahad19 closed this Sep 18, 2023
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 this pull request may close these issues.

7 participants