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

feat: adds allFlagMetadata to clients and provider interface #245

Closed
wants to merge 15 commits into from

Conversation

maxveldink
Copy link
Member

@maxveldink maxveldink commented Feb 14, 2024

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.

@maxveldink maxveldink force-pushed the maxveldink/get-all-flags branch 2 times, most recently from 784c4c8 to 828f143 Compare February 14, 2024 13:36
@maxveldink maxveldink requested a review from a team February 14, 2024 13:36
specification/sections/02-providers.md Outdated Show resolved Hide resolved
specification/types.md Show resolved Hide resolved
@maxveldink maxveldink marked this pull request as ready for review February 14, 2024 13:39
specification/types.md Outdated Show resolved Hide resolved
@maxveldink maxveldink changed the title feat: adds allFlagKeys to provider metadata feat: adds allFlagMetadata to provider metadata Feb 17, 2024
@maxveldink maxveldink force-pushed the maxveldink/get-all-flags branch from ac4b621 to 8a3efdb Compare February 17, 2024 02:39
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.

Thanks for getting this started and sorry for the delayed review. Please review my comments when you have a moment and let me know if you have any questions. Thanks!

specification/sections/02-providers.md Outdated Show resolved Hide resolved
specification/sections/02-providers.md Outdated Show resolved Hide resolved
specification.json Outdated Show resolved Hide resolved
@Kavindu-Dodan Kavindu-Dodan force-pushed the maxveldink/get-all-flags branch from 387fbec to fba1a6a Compare February 23, 2024 21:11
specification/types.md Outdated Show resolved Hide resolved
specification/types.md Show resolved Hide resolved
@maxveldink
Copy link
Member Author

@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
Copy link
Member Author

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!

```

This operation should not be confused with a bulk evaluation of all flags.
This is an informative operation available to clients for understanding which flag keys are currently active from a provider.

Choose a reason for hiding this comment

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

The method is planned to be named getAllFlagMetadata but the description mentions this returns "currently active" flags only. Is "currently active" defined further somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, worth clarifying this. My personal understanding is that it refers to the enabled feature flags in the provider

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching that! I know active is semantically significant for most vendors. I tried to clarify this by changing it to available, which can be interpreted different per provider.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dferber90 I hope it clarifies it. What do you think of this enhancement?

@jonathannorris
Copy link
Member

jonathannorris commented Mar 7, 2024

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
Copy link

dferber90 commented Mar 7, 2024

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
Copy link
Member

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
Copy link
Member

toddbaert commented Mar 7, 2024

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
Copy link
Member Author

maxveldink commented Mar 7, 2024

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
Copy link
Member Author

@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
Copy link
Member

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
Copy link
Contributor

weyert commented Mar 7, 2024

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

@kinyoklion
Copy link
Member

kinyoklion commented Mar 7, 2024

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
Copy link
Member

toddbaert commented Mar 7, 2024

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
Copy link
Member

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
Copy link
Member

toddbaert commented Mar 7, 2024

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
Copy link
Member Author

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
Copy link

dferber90 commented Mar 8, 2024

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.

@kinyoklion
Copy link
Member

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
Copy link
Member Author

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

@maxveldink maxveldink closed this Mar 11, 2024
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.