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.04k stars 114 forks source link

Adds default http targeting context accessor #416

Closed rossgrambo closed 4 months ago

rossgrambo commented 6 months ago

Why this PR?

  1. Addresses issues a. https://github.com/microsoft/FeatureManagement-Dotnet/issues/415 b. https://github.com/microsoft/FeatureManagement-Dotnet/issues/399
  2. Also makes HttpContext.Items cache consistent- so all classes check the same location.

Next Steps:

  1. Remove unremarkable HttpTargetingContextAccessors from examples in favor of DefaultHttpTargetingContextAccessor.
  2. Consider a different cache pattern that doesn't rely on a shared string key.

Visible Changes

Developers will now have a new type available to them, DefaultHttpTargetingContextAccessor. Instead of defining their own, they can use this type when adding targeting via WithTargeting like:

builder.Services.AddFeatureManagement()
    .WithTargeting<DefaultHttpTargetingContextAccessor>()

The default accessor will extract the targeting info from HttpContext.User. Groups will be extracted from claims of type Role. UserId is taken from the Identity.Name field.

rossgrambo commented 6 months ago

If someone has an idea for a shared place for the TargetingContextLookup string, let me know. I couldn't decide on a clean spot. Decided to leave them as separate strings for now since we're hoping to adjust the caching to not need them anyway.

zhenlan commented 6 months ago

Just for ideas... you either make it a public property or use InternalsVisibleToAttribute between the two packages. Like you said, none of them are clean ways.

jimmyca15 commented 6 months ago

I know adding a scoped service will be an approach that we will want to propose to float the resolved targeting context along, but I also thing we should consider an alternative option -- exposing a public extension method on HttpContext from the Microsoft.FeatureManagement.AspNetCore package that gets the resolved targeting context.

namespace Microsoft.FeatureManagement.AspNetCore;

public static class HttpContextExtensions
{
    //
    // Gets the targeting context resolved by feature management during a request.
    public static TargetingContext GetTargetingContext(this HttpContext httpContext);
}
zhiyuanliang-ms commented 6 months ago

@jimmyca15 @rossgrambo Are we going to include this PR into the next preview release (preview3)?

rossgrambo commented 6 months ago

Would be nice- but I don't think it's a blocker.

zhiyuanliang-ms commented 6 months ago

Is there a plan to use this built-in targeting context accessor in our FeatureFlagDemo example? (include the change in this PR or create a new PR after this PR is merged?)

rossgrambo commented 6 months ago

Is there a plan to use this built-in targeting context accessor in our FeatureFlagDemo example? (include the change in this PR or create a new PR after this PR is merged?)

Yes, I'm planning on a separate PR to update examples.

jimmyca15 commented 6 months ago

There have been some developments as we have discussed the middleware and telemetry initializer across different PRs/issues. My expectation at this point is that:

As such, I don't expect any changes in the middleware/initializer as part of this PR.

rossgrambo commented 6 months ago

As such, I don't expect any changes in the middleware/initializer as part of this PR.

Agreed. This PR is on hold until the caching rework is completed.

rossgrambo commented 4 months ago

This PR has been dissolved into two: