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

Async API for featuremanagement #85

Closed
andrewlock opened this issue Jun 7, 2019 · 4 comments
Closed

Async API for featuremanagement #85

andrewlock opened this issue Jun 7, 2019 · 4 comments
Assignees
Labels
enhancement New feature or request FeatureManagement

Comments

@andrewlock
Copy link

HI, I've been trying out the feature management package, and one of the design aspects that feels a little strange is that the API is entirely synchronous.

This totally makes sense from the consuming point of view, and when you consider you're driving it from IConfiguration, but it feels like it will severely limit what you can do in IFeatureFilter. Doing a database lookup for example becomes impossible without blocking. It may not be feasible to pull all that lookup data into configuartion, so this (very common I would imagine) scenario is broken.

The lack of an async API feels very much at odds with the general design trend for ASP.NET Core in general too, where async all the things seems to be the approach!

I'm assuming you decided against an async API specifically so that the features could be used in synchronous code-paths. I'm just wondering if there's a way to make async feature filters a thing!

@zhenlan
Copy link
Contributor

zhenlan commented Jul 9, 2019

Sounds reasonable to me. This needs IFeatureFilter to support async.

@jimmyca15, thoughts?

@jimmyca15
Copy link
Member

@andrewlock @zhenlan

This is the biggest debate right now with the Feature Management API. Currently the API is developed as a thin API surface over IConfiguration with the core scenario of enabling or disabling feature's based off whether they are configured true/false. Being synchronous allows feature management to be integrated into parts of frameworks that require synchronous code paths, and since IConfiguration is synchronous it all makes sense.

We recognize that this is a large limitation in feature filters because like you mentioned, calls to servers could not be made unless they are blocking and also pulling data into the current HTTP context or configuration might be excessive especially since it is possible that this feature data might not be used during a given request.

Introducing async would require a breaking change of the core API bool IFeatureManager.IsEnabled(string featureName) Otherwise there would be a notion of sync feature manager and async feature manager and then there would be sync feature filters, and async feature filters. Trying to figure out what should be called where would be an issue, and it would be confusing for those who depend on the package. So if we make the change it will be a big one.

At the moment we are evaluating whether we want to make this breaking change (it's basically the biggest one we could make).

Synchronous has some benefits

  • Thin fundamental API around IConfiguration
  • Integrate into any code-path

Asynchronous has some benefits as well that you mentioned

  • Ability to use async feature filters

If we make the move to async, some functionality that is possible with the current code will be lost, including some of the plugin points exposed into ASP.NET Core.

@andrewlock
Copy link
Author

Thanks for the input @jimmyca15 - totally understand the issues with going async here, and everything you've said makes sense. I definitely think you need to go all-in on sync or async, not support both - agree that would be confusing.

Regarding the functionality that would be lost, the main one I see is the FeatureRouteConstraint which is necessarily sync. There's async versions of featurefilters and feature tags so those are fine, and many other paths are already async. Whether that's a big loss - I'm not sure, but in my experience, route constraints aren't often used that much. That's n=1 though so 🤷‍♂

My initial thought was regarding storing feature flag values in the database:

  • That kind of goes against the aim of the library (a thin API around IConfiguration)
  • However, I expect it will be exactly what many people will want to do
  • If you're enabling per-user, then the API over IConfiguration won't really work - you would def have to use featurefilters, but then you have an issue as can't do sync.
  • A possible workaround is dedicated middleware that "pre-loads" the database feature-flags early in the pipeline.

That last point makes the sync API mostly workable I think, but it does feel a bit hacky - it feels like you're fighting the featuremanagement library. I'm really not sure what's best tbh!

@jimmyca15
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request FeatureManagement
Projects
None yet
Development

No branches or pull requests

3 participants