open-feature / spec

OpenFeature specification
https://openfeature.dev
Apache License 2.0
588 stars 35 forks source link

feat: adds allFlagMetadata to clients and provider interface #245

Closed maxveldink closed 3 months ago

maxveldink commented 4 months ago

This PR

Adds an optional allFlagMetadata accessor to clients and the provider interface for retrieving a list of flag keys available in the provider.

Related Issues

Discussion that predated this proposal.

federicobond commented 4 months ago

One thing that worries me a bit about this specification is that many vendors will have paginated APIs for retrieving lists of flags, and there is no guidance on how they should implement this method.

Should they consume the whole collection in N requests in order to ensure correctness?

lukas-reining commented 4 months ago

One thing that worries me a bit about this specification is that many vendors will have paginated APIs for retrieving lists of flags, and there is no guidance on how they should implement this method.

Should they consume the whole collection in N requests in order to ensure correctness?

Good point @federicobond. On the other hand a provider may just decide to not implementallFlags metadata if pagination is too much of an hurdle as cited in the following. A provider may just decide to fetch all pages or simply not implement getAllMetadata if either the information is not available or implementations is just not good e.g. if the endpoints are expensive or just hard to iterate.

Requirement 1.2.3 The client interface MUST define a allFlagMetadata member or accessor, returning a collection of flag metadata from the provider.

client.getAllFlagMetadata() // [{"key": "featureA", "type": "boolean"}, {"key": "featureB", "type": "string"}]

This may return an empty collection if the provider does not implement the allFlagMetadata member or accessor.

beeme1mr commented 4 months ago

Let's try and get this merged next week. If anyone has any concerns, please speak up so they can be addressed.

luizgribeiro commented 3 months ago

I have a few minor concerns about this.

This may return a language-idiomatic way of describing the absence of a value if the provider does not implement the `allFlagMetadata` member or accessor.

I get that in some languages it's straight forward, but in others it would be open to the provider to determine how to express that it does not implement this method. Eg: In Java there could be one provider that throws and another one that returns an empty Collection. Any idea on how to deal with this? There's also an issue with empty Collections imho. It would be ambiguous because user's apps wouldn't be able to determine if this is not implemented by the provider or there's no flag configured.

My first thought in this situation is to return a struct with flag's metadata and details.

Regarding the provider's backend being overwhelmed by this sort of request, there could be something like a flag metadata query for getAllFlagMetadata. If flags store metadata about itself (like their bounded context) the App could get exactly what it needs. It's probably not in the scope of this proposal though

maxveldink commented 3 months ago

Thanks @luizgribeiro ! @lukas-reining has a similar concern here, which I tried to alleviate by having the provider interface optionally implement the behavior. In contrast, the client interface always needs to implement the behavior, with the option of returning null if the all flag metadata behavior is unavailable, for any reason.

Is it worthwhile to clarify that the client should handle erroring states and prevent them from bubbling up if something goes wrong in the provider?

luizgribeiro commented 3 months ago

In contrast, the client interface always needs to implement the behavior, with the option of returning null if the all flag metadata behavior is unavailable, for any reason.

That's where I think there might be a way to express provider state clearly to the app. Using a struct/map it's possible to communicate what happened and the app can handle scenarios as needed. @federicobond's statement (here)gave us some details and scenarios in which it would be nice to differentiate the result.

So, using something like the following would be an alternative.

type AllFlagsMetadataReturn = {
  status: 'OK' | 'REQUEST_ERROR';
  data: Array<{
    flag: string;
    metadata: 'string' | 'bool' | 'int' | 'float'
  }>;
};
toddbaert commented 3 months ago

In contrast, the client interface always needs to implement the behavior, with the option of returning null if the all flag metadata behavior is unavailable, for any reason.

That's where I think there might be a way to express provider state clearly to the app. Using a struct/map it's possible to communicate what happened and the app can handle scenarios as needed. @federicobond's statement (here)gave us some details and scenarios in which it would be nice to differentiate the result.

So, using something like the following would be an alternative.

type AllFlagsMetadataReturn = {
  status: 'OK' | 'REQUEST_ERROR';
  data: Array<{
    flag: string;
    metadata: 'string' | 'bool' | 'int' | 'float'
  }>;
};

@luizgribeiro in the latest (unreleased) spec version, there's an independent provider state accessor that must be available, see 1.7.1 and proceeding points.

kinyoklion commented 3 months ago

Left minor nits, but largely ready to approve; I'm interested in getting some more feedback, though. @kinyoklion and @federicobond seem like they might have some reservations.

I can probably accept this specification, in that I cannot see the immediate harm that it causes, but I am still not confident in use cases.

maxveldink commented 3 months ago

@luizgribeiro, I see what you're saying about the return type. I worry that we'd be adding too much complexity to the provider method if we introduced a new type to model potential error situations from the provider. I think allowing the provider method to return null, for now, would give enough information to the client to know if the flags returned are empty versus unavailable.

I added one more callout on the provider spec to clarify that more.

maxveldink commented 3 months ago

I believe I've addressed all feedback on this PR now. I'll wait until tomorrow morning and merge this if there is no other feedback!

jonathannorris commented 3 months ago

Hey @maxveldink, what's the motivation behind having the type in the data model?

I think we might have a better chance of constraining the usage of this to its intended use-case if the data model was simpler and more restrictive:

for example, instead of: { flags: [{"key": "featureA", "type": "boolean"}, {"key": "featureB", "type": "string"}] }

A simpler mapping of just flag key -> feature key would less likely be used beyond just as metadata:

{ "key_1": "featureA", "key_2": "featureB"}

dferber90 commented 3 months ago

The way we're able to show Feature Flags of any provider in the toolbar is that we query the various providers and map their API responses into this shape

interface FlagOptionType {
  value: any;
  label?: string;
}

interface FlagDefinitionType {
  options?: FlagOptionType[];
  /**
   * The URL where the feature flag can be managed.
   */
  origin?: string;
  description?: string;
}

type ProviderData = {
  definitions: Record<string, FlagDefinitionType>;
};

Being able to see the available options & description lets us build a rich UI for overwriting flags, where we can display the available flags, their descriptions, their options and the label of each option if present.

image
toddbaert commented 3 months ago

Hey @maxveldink, what's the motivation behind having the type in the data model?

I think we might have a better chance of constraining the usage of this to its intended use-case if the data model was simpler and more restrictive:

for example, instead of: { flags: [{"key": "featureA", "type": "boolean"}, {"key": "featureB", "type": "string"}] }

A simpler mapping of just flag key -> feature key would less likely be used beyond just as metadata:

{ "key_1": "featureA", "key_2": "featureB"}

My take is generally "less is more" but I'm not sure if anyone's envisioned use-case requires the type info. If not, I think we should drop it, but I'm wondering it if could be helpful. Some backends don't strictly type flags. cc @maxveldink @jonathannorris @dferber90

toddbaert commented 3 months ago

The way our Feature Flag toolbar integration works is that we query the various providers and map their API responses into this shape

interface FlagOptionType {
  value: any;
  label?: string;
}

interface FlagDefinitionType {
  options?: FlagOptionType[];
  /**
   * The URL where the feature flag can be managed.
   */
  origin?: string;
  description?: string;
}

type ProviderData = {
  definitions: Record<string, FlagDefinitionType>;;
};

Being able to see the available options & description lets us build a rich UI for overwriting flags, where we can display the available flags, their descriptions, their options and the label of each option if present.

image

I guess "label" would correspond to our key prop? description could be something supplied loosely in the metadata if I'm understanding correctly.

Is the proposed API sufficient then? Or is there anything we're missing? What is value? We don't evaluate these flags in our case so if it's the flag value I'm not sure we can satisfy that.

@dferber90

maxveldink commented 3 months ago

Hey @maxveldink, what's the motivation behind having the type in the data model?

I think we might have a better chance of constraining the usage of this to its intended use-case if the data model was simpler and more restrictive:

for example, instead of: { flags: [{"key": "featureA", "type": "boolean"}, {"key": "featureB", "type": "string"}] }

A simpler mapping of just flag key -> feature key would less likely be used beyond just as metadata:

{ "key_1": "featureA", "key_2": "featureB"}

I originally had a more straightforward collection like this, just really a list of keys. The need for a type comes from client evaluation. Callers need to know which client evaluation method to call for a given flag key, so we need some type information there to understand which client method to call.

@jonathannorris Is the current language too ambiguous for what should be in the type field? type is an optional property with a value of type string, indicating which client resolution method should be used to resolve the flag key.

maxveldink commented 3 months ago

@toddbaert @dferber90 Yep, description could be returned in the flag metadata type that we have defined. That can largely be left up to the provider/underyling vendor

jonathannorris commented 3 months ago

This PR

Adds an optional allFlagMetadata accessor to clients and the provider interface for retrieving a list of flag keys available in the provider.

  • Adds requirements for allFlagMetadata accessor
  • Defines Collection and Provider Metadata types in the Types page and adds key information to Flag Metadata.

Related Issues

Discussion that predated this proposal.

I'm weary about bringing this up and don't want to block this PR further, but it sounds like you would be better served by a bulk flag evaluation method. That is generally what our customers would use for this use case. I usually imagine "metadata" like this should primarily be used for analytics/tracking purposes, not really for having enough information to properly make a request for an individual flag evaluation.

weyert commented 3 months ago

Bulk evaluation costs money (e.g. 10x normal single flag evaluation) while fetching a list of flags available does not

kinyoklion commented 3 months ago

Bulk evaluation costs money (e.g. 10x normal single flag evaluation) while fetching a list of flags available does not

Currently the only way to provide this data for a LaunchDarkly provider would be via a bulk-evaluation, which is part of the reason I am dubious about use cases. For the LaunchDarkly case the primary cost will be in the time the evaluations takes. (Aside from more stateless ones like PHP, which will have a greater cost to execute bulk-evaluations.)

toddbaert commented 3 months ago

I think there's some value in, to use the language of the PR:

understanding which flag keys are currently available from a provider

I can imagine situations where this could be useful for debugging, administrative UIs, etc; generally for tangential use-cases not typical flag evaluation as we normally consider it. I appreciate how some SDKs/providers simply don't support an equivalent right now.

I'm still not sure if @dferber90 's use case is satisfied if we don't return actual evaluated flag values though.

kinyoklion commented 3 months ago

I think there's some value in, to use the language of the PR:

understanding which flag keys are currently available from a provider

I can imagine situations where this could be useful for debugging, administrative UIs, etc; generally for tangential use-cases not typical flag evaluation as we normally consider it. I appreciate how some SDKs/providers simply don't support an equivalent right now.

I'm still not sure if @dferber90 's use case is satisfied if we don't return actual evaluated flag values though.

I do see use of that information from a debugging perspective, but I don't know about this specific way of exposing it.

If your application is evaluating flags, then hooks provide the means to know what flags are evaluated, and what values they evaluated to.

The resolution details should contain information about flags that are evaluated that don't exist.

Provider configuration change events, when they can be supported, also provide that list of flags, but potentially in an incremental way.

I don't like bulk-evaluation, but it does solve these use cases, and I suspect flag meta data will just be used to make less efficient versions of bulk-evaluation (which is worse than just bulk-evaluation).

toddbaert commented 3 months ago

OK... I hate to throw more into the mix here, but I really don't want us to merge something we're not sure about.

I have a alternate path I'd like to propose which perhaps satisfies the original use-case brought up by @maxveldink and others, and dodges the valid pitfalls @kinyoklion mentions...

What if instead of adding a method to the client, we put a very similar payload in the READY event payload (and also enhance the PROVIDER_CONFIGURATION_CHANGED payload to feature the same). This means that if your provider supports it, you could attach a READY handler that would get all the flags and their metadata.

This has the advantage of being isomorphic with the PROVIDER_CONFIGURATION_CHANGED events, improving consistency. Plus, it makes it more clear that this set may change - and may need to be updated with change events; this can be easily done by adding a similar handler to those. The provider would have to be READY anyway in order to provide meaningful data via the proposed client.allFlagMetadata, so I don't think we're constraining ourselves in any way here. While it's true that users could still implement a dirty "bulk evaluation" this way, I think it's less likely to happen this way. EventDetails are already "best effort" so adding more optional data here feels idiomatic as well.

maxveldink commented 3 months ago

The current proposal seems to invite misuse, and we haven't reached an entire agreement. I'm cool with closing this attempt (with gratitude for the great conversation) so we can focus the discussion on using some other affordances in OpenFeature, such as hooks and events.

The Ruby SDK has yet to implement hooks and events, so I'm not as versed in them from an implementation perspective. Can we open a new issue/discussion for @toddbaert 's proposal?

dferber90 commented 3 months ago

Seems our use case was misunderstood a bit. To us an API would be useful which answers the questions

Imagine a payload like

{
  "myFlag": {
    "description": "A feature flag",
    "origin": "https://example.com/myFlag",
    "options": [
      { "value": false, "label": "off" },
      { "value": true, "label": "on" }
    ]
  }
}

What is value? We don't evaluate these flags in our case so if it's the flag value I'm not sure we can satisfy that.

Value is not the value the flag resolved to, but rather what it could resolve to. We turn this list of possible values into a dropdown where customers can select one of the possible values to override a feature flag.


Loading this kind of information usually requires a different API key, which needs to be kept secret. I'm not familiar enough with OpenFeature yet to know whether this would be in scope of OpenFeature, or whether OpenFeature is purely concerned with the "resolving feature flags" part.

kinyoklion commented 3 months ago

Seems our use case was misunderstood a bit. To us an API would be useful which answers the questions

  • "Which flags does the provider offer, and what are their descriptions?"
  • "For each flag the provider offers, which values can it resolve to, and what are those called if they have a name (label)?"

Imagine a payload like

{
  "myFlag": {
    "description": "A feature flag",
    "origin": "https://example.com/myFlag",
    "options": [
      { "value": false, "label": "off" },
      { "value": true, "label": "on" }
    ]
  }
}

What is value? We don't evaluate these flags in our case so if it's the flag value I'm not sure we can satisfy that.

Value is not the value the flag resolved to, but rather what it could resolve to. We turn this list of possible values into a dropdown where customers can select one of the possible values to override a feature flag.

Loading this kind of information usually requires a different API key, which needs to be kept secret. I'm not familiar enough with OpenFeature yet to know whether this would be in scope of OpenFeature, or whether OpenFeature is purely concerned with the "resolving feature flags" part.

This seems like a much greater level of detail than a provider would have in most cases. (Especially for single context paradigm where the available options are often not communicated to the client at all.)

I don't know what the requirements would be across many vendors, but for LaunchDarkly I know that it would require a different type of credential to get this kind of information than the credentials by SDKs.

It also seems like a debug level of feature, not a production feature? Something that maybe would lend itself to having some configuration to allow usage that could be easily disabled in production?

maxveldink commented 3 months ago

As mentioned above, closing for now. Thanks all for the conversation!