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

Suggest to have app.UseFeatureManagement #396

Open
jimmyca15 opened this issue Mar 11, 2024 · 8 comments
Open

Suggest to have app.UseFeatureManagement #396

jimmyca15 opened this issue Mar 11, 2024 · 8 comments
Milestone

Comments

@jimmyca15
Copy link
Member

Expected

app.UseFeatureManagement();

Actual

app.UseMiddleware<TargetingHttpContextMiddleware>();

See here.

@zhenlan
Copy link
Member

zhenlan commented Mar 11, 2024

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

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

zhenlan commented Mar 13, 2024

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

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

zhenlan commented Mar 14, 2024

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.

@zhenlan zhenlan added this to the v4.0.0 milestone Mar 23, 2024
@rossgrambo
Copy link
Contributor

rossgrambo commented Mar 27, 2024

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

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

Related: #424

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

3 participants