Suggest to have app.UseFeatureManagement

microsoft / FeatureManagement-Dotnet

Microsoft.FeatureManagement provides standardized APIs for enabling feature flags within applications. Utilize this library to secure a consistent experience when developing applications that use patterns such as beta access, rollout, dark deployments, and more.

MIT License

1.03k stars 111 forks source link

Suggest to have app.UseFeatureManagement #396

Open jimmyca15 opened 6 months ago

jimmyca15 commented 6 months ago

Expected

app.UseFeatureManagement();

Actual

app.UseMiddleware<TargetingHttpContextMiddleware>();

See here.

zhenlan commented 6 months ago

I like it. I was thinking about something similar too.

The TargetingHttpContextMiddleware is only useful for ASP.NET targeting telemetry. The name of UseFeatureManagement is quite generic. We may want to reserve that name for a more base library though.

jimmyca15 commented 6 months ago

The middleware is defined in our base ASP.NET Core library, given the extension depends on ASP.NET core, I don't think it could go into any more base of a library.

zhenlan commented 6 months ago

You are right about the base library, Jimmy.

As we discussed offline, since TargetingHttpContextMiddleware is only useful for 1. targeting scenarios 2. AppInsights telemetry. To be a good citizen, ideally, we can add middleware only when we detect those conditions. Then I have no concerns to wrap the middleware in the generic method UseFeatureManagement.

rossgrambo commented 6 months ago

I think this middleware can be obsoleted by a TargetingContextAccessor doing the same thing. I spoke a little with Jimmy about the idea. I plan to make a PR and we can discuss further on that. Happy to keep this issue open until that's decided on.

zhenlan commented 6 months ago

If I understand your proposal correctly, the problem with it is that the request that calls TrackEvent may never trigger a feature evaluation at all. The heart-vote button is an example. If I misunderstand your proposal, please elaborate.

On the other hand, I'm happy to ship an in-box HttpContext-based TargetingContextAccessor. Please open a separate issue with details of the proposal.

rossgrambo commented 5 months ago

Hey @zhenlan & @jimmyca15 , looks like we were considering to add ASP.NET stuff to the .AddFeatureManagement(). Do we still feel that way about the out-of-the-box HttpTargetingContextAccessor?

jimmyca15 commented 5 months ago

add ASP.NET stuff to the .AddFeatureManagement()

Adding anything ASP.NET related to the AddFeatureManagement method isn't doable since AddFeatureManagement is in the base feature management library.

jimmyca15 commented 5 months ago