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 filters are combined with a logical OR #28

Open
RemyBoyer opened this issue Nov 29, 2019 · 16 comments
Open

Feature filters are combined with a logical OR #28

RemyBoyer opened this issue Nov 29, 2019 · 16 comments

Comments

@RemyBoyer
Copy link

Hello,

While experimenting with the sdk, we noticed that when evaluating the filters and the contextual filters, the FeatureManager internal loop will break and return true for the first filter that returns true.

We are planning to use heavily the sdk, but we feel that our needs are mainly steering towards a logical AND.

For example, we will want to enable a feature for one specific country (contextual filter), but only for a defined percentage of them (filter). This would look like this in the app configuration store :

"conditions": {
    "client_filters": [
        {
            "name": "CountryContextual",
            "parameters": {
                "Country": [
                    "FR",
                    "IT"
                ]
            }
        },
        {
            "name": "Microsoft.Percentage",
            "parameters": {
                "Value": "50"
            }
        }
    ]
}

We imagine we might be able to create "meta" filters that would contain a logical operator and a list of sub filters, which can also be recursive. This would solve pretty much any use case we could imagine, but it seems to be a little over-complicated.

We could also create our own implementation of IFeatureManager, but the required dependency IFeatureSettingsProvider is currently internal.

In our opinion, the most sensible default combination operator should be an AND.

What's your opinion on that? Do you think of any built-in or alternative way of reaching our goals?

@jimmyca15
Copy link
Member

Hello @RemyBoyer ,

Trying to introduce AND vs OR behavior in the evaluation of feature filters can be complex. It seems to me that a better approach to this issue would be to create the country filter to be percentage aware.

        {
            "name": "CountryContextual",
            "parameters": {
                "Countries": [
                    {
                      "Code": "FR",
                      "Percentage": 10
                    },
                    {
                      "Code": "IT",
                      "Percentage": 30
                    }
                ]
            }
        }

The Microsoft.FeatureManagement library is perfectly fine to handle a filter like this. You may find a disconnect when trying to set up a filter like this in the Azure App Configuration portal since you mentioned what it would look like in Azure App Configuration. When configuring feature filters in the Azure App Configuration portal, it is difficult to model arrays (see issues #27 and 147). It is even more difficult to model arrays of objects. This is something we are currently in the process of revamping.

Ignoring the current portal configuration issue that I mentioned, does that sound like an adequate solution?

@RemyBoyer
Copy link
Author

Hello @jimmyca15 ,

First, regarding the App Configuration portal, it won't be a problem for us. We're planning on using a custom portal with all our rules, and updating the App Configuration store data programmatically. We should be able to do whatever we want.

Regarding the filters though, we think that the approach you're suggesting will unfortunately not scale very well. Here's a list of the filters that we plan to implement, knowing that it might get bigger:

on/off
date/time range (from/to) (required for synchronized deployment)
percentage (we'll have to wonder whether we provide a "deterministic/repeatable" one instead of a random one)
business unit (multiple)
partner code (multiple)
application version (from/to)

Having one big filter that would combine all of these would, in my opinion, add unnecessary complexity. If we don't ship all these filters at once, we'll have to be careful about retro-compatibility, for example.
Whereas using a logical "AND", which seems to be more sensible, would simply mean that we can add the filters one by one, and rely on the framework to determine whether this filter is registered on the consuming asp.net core api (I noticed it already ignores filters that haven't been registered).

This logical "AND" won't be perfect and solve all our requirements, but we think it would be good enough as a first version for quite some time.

Have you considered changing this default behavior? I understand it's a breaking change, so it has to be carefully studied, but we'd like to know your opinion on this matter.

@jimmyca15
Copy link
Member

Changing the default behavior is unlikely. We can enable the scenario by adding a switch in the feature settings. We can add something just like the RequirementType we use with the FeatureGateAttribute.

"conditions": {
    "requirement_type": "all",
    "client_filters": [
        {
            "name": "CountryContextual",
            "parameters": {
                "Country": [
                    "FR",
                    "IT"
                ]
            }
        },
        {
            "name": "Microsoft.Percentage",
            "parameters": {
                "Value": "50"
            }
        }
    ]
}

@RemyBoyer
Copy link
Author

I think this approach would suit our needs! 👍

@leonids2005
Copy link

Any idea when this feature (AND filters) could be implemented?

@zhenlan
Copy link
Member

zhenlan commented Mar 9, 2020

I think it will be just a matter of time someone will ask how to do this:

Filter1 OR ( Filter2 AND Filter3)"

I am thinking this AND/OR operator should go with each filter.

@leonids2005
Copy link

But let's do it in Agile way.... sorry to remind this :)
First agile principle :
Our highest priority is to satisfy the customer through early and continuous delivery of valuable software.

just AND all filters is a really good way forward and is needed by number of clients already.,

@jimmyca15
Copy link
Member

@zhenlan I could see that being requested in the future as well, but for now I haven't heard any requests for it. The ask here to switch the default evaluation behavior from require anything to require everything seems a bit simpler. I don't think enabling the scenario via the requirement_type option will block any future work on the ability to require a more complex and/or combination.

The requirement_type option is meant to be a way to control the default and/or evaluation. I would expect if we enhanced the and/or logic to individual filters than it would override the requirement_type option.

@leonids2005
Copy link

@jimmyca15 - absolutely true. And I cannot believe requirement_type feature is difficult to implement :)

@zhenlan
Copy link
Member

zhenlan commented Mar 10, 2020

@leonids2005 I am all for agile development. The reason we are discussing this issue here is for the continuous delivery of value. We just have different opinions about the solution. Just doing customers asks may not always result in happy customers if we find we have to change it in the future. Let's focus on the solution.

@jimmyca15 this is a data schema change. Every change we make here, we will have to make them available in Azure portal, Auzre CLI, Java, import/export functionalities and so on so forth. It's not a neglectable effort. I am not necessarily against the idea of requirement_type, but I'd like to make sure this set a good path forward. I'd like we have a more concrete plan that can handle more complex filter combinations even if we decide to just do requirement_type as the first step.

Loop in @MSGaryWang to brainstorm ideas.

@jimmyca15
Copy link
Member

@zhenlan Doing logical operations like and/or in a declarative manner is an interesting problem to tackle, but here's one way it could be done.

Desired combination:
(FilterA && FilterB) || (FilterA && FilterC) || FilterD

The combination can be achieved by declaring filter groups. The filters are all evaluated, however the feature is only on if all of the filters in one of the filter groups are enabled.

"conditions": {
    "filter_groups": [
        [ "FilterA", "FilterB" ],
        [ "FilterA", "FilterC" ],
        [ "FilterD" ]
    ],
    "client_filters": [
        { "name": "FilterA" },
        { "name": "FilterB" },
        { "name": "FilterC" },
        { "name": "FilterD" },
    ]
}

@zhenlan
Copy link
Member

zhenlan commented Mar 10, 2020

I like the idea of filter_groups (or something similar), but how can I do this?

FilterA && (FilterB || FilterC)

@jimmyca15
Copy link
Member

jimmyca15 commented Mar 10, 2020

filter_groups: [
  [ "FilterA", "FilterB" ]
  [ "FilterA", "FilterC" ]
]

@Greybird
Copy link

Hi,

Hope you are doing fine in these troubled times!

Any news on the progress regarding this feature ? Would be great to have it, as it would remove some blockers in our use cases!

Regards,

@rsr-maersk
Copy link

Any updates on this? It is over 2 years old. And it is a valid use case

@jimmyca15
Copy link
Member

jimmyca15 commented Jun 1, 2022

Hello @rsr-maersk, yes I do have an update to share. We are planning on adding support for this in our upcoming 3.x package. The addition won't be in scope for the initial 3.0.0-preview release, but we plan on adding it shortly after in what will most likely be 3.0.0-preview2.

There are a few methods pitched in this thread. Right now the approach we are looking to implement is the RequirementType approach mentioned in this comment.

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

No branches or pull requests

6 participants