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

Adds default http targeting context accessor #416

Closed

Conversation

rossgrambo
Copy link
Contributor

@rossgrambo rossgrambo commented Apr 3, 2024

Why this PR?

  1. Addresses issues
    a. Add Default Http Targeting Context Accessor #415
    b. The TargetingId should be included in the FeatureEvaluation event even if it's null/empty. #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
Copy link
Contributor Author

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

zhenlan commented Apr 4, 2024

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

jimmyca15 commented Apr 5, 2024

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

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

@rossgrambo
Copy link
Contributor Author

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

@@ -18,7 +18,7 @@ public class TargetingHttpContextMiddleware
private readonly RequestDelegate _next;
private readonly ILogger _logger;

private const string TargetingIdKey = $"Microsoft.FeatureManagement.TargetingId";
private const string TargetingContextLookup = "FeatureManagement.TargetingContext";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason why we removed Microsoft prefix here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I can recall... I'll add it back.

@zhiyuanliang-ms
Copy link
Contributor

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

rossgrambo commented Apr 16, 2024

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

jimmyca15 commented Apr 16, 2024

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:

  • The middleware moves to the same project as the initializer
    • If we stick to http context items cache, the key is project internal
  • The default targeting context accessor uses a cache key that is not accessible outside
    • private object _targetingContextKey = new object();

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

@rossgrambo
Copy link
Contributor Author

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.

/// <summary>
/// The key used to store and retrieve the <see cref="TargetingContext"/> from the <see cref="HttpContext"/> items.
/// </summary>
public const string TargetingContextLookup = $"Microsoft.FeatureManagement.TargetingContext";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's debatable who should own this cache key. It's odd to see the more generic initializer and middleware reference the cache key from a very specific targeting context accessor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed... I still am unable to come up with a satisfying place to put this. I suppose the owner could be the middleware- because the middleware's whole goal is to ensure that the TargetingContext is available at that key.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The targeting context accessor doesn't need to share a key.

private static object _cacheKey = new object(); should work fine.

Copy link
Contributor Author

@rossgrambo rossgrambo Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's 3 classes that interact with HttpContext.Items for targeting context. Initializer, Middleware, and the TargetingContextAccessor.

Initializer depends upon the Middleware. The Middleware depends upon a TargetingContextAccessor existing. The Middleware and Initializer are tightly coupled- so they definitely need to share a key. The TargetingContextAccessor doesn't have to share the same key, but it reduces our HttpContext.Items footprint if it does- and is a clear shared cache location.

So for Initializer and Middleware, we are going to have a shared key- it could be either an object ref or the string key "Microsoft.FeatureManagement.TargetingContext". I slightly prefer the string key since it's public (on the HttpContext.Items and as a public property). The key needs a home, and if it was just these two classes it would either be on the middleware or a shared constants class.

Now the DefaultTargetingContextAccessor comes in- which is our default implementation. It needs a cache key as well. It could either define its own key (as a string or object ref) or used a shared constants class.

I think the shared constants class makes things the most clear- as FM is defining where TargetingContext should live on HttpContext.Items. I ended up not liking adding the constants class because it introduces yet another public class, and I ended up going with the DefaultTargetingContextAccessor.

@rossgrambo
Copy link
Contributor Author

This PR has been dissolved into two:

@rossgrambo rossgrambo closed this Jun 20, 2024
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

Successfully merging this pull request may close these issues.

5 participants