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.06k stars 115 forks source link

Adjusts builder extension methods to be friendlier #487

Closed rossgrambo closed 3 months ago

rossgrambo commented 3 months ago

Preview now with app insights telemetry looks like this:

// Add App Insights
builder.Services.AddApplicationInsightsTelemetry(); // Shouldn't be done by us

builder.Services.AddHttpContextAccessor();

// Add FeatureManagement's TargetingTelemetryInitializer
builder.Services.AddSingleton<ITelemetryInitializer, TargetingTelemetryInitializer>();

// Add FeatureManagement
builder.Services.AddFeatureManagement()
    .WithTargeting() // Uses DefaultHttpTargetingAccessor, requires HttpContextAccessor
    .AddApplicationInsightsTelemetryPublisher();

var app = builder.Build();

//...

app.UseMiddleware<TargetingHttpContextMiddleware>();

Making a few opinionated changes- we can make it:

// Add App Insights
builder.Services.AddApplicationInsightsTelemetry(); // Shouldn't be done by us

builder.Services.AddFeatureManagement()
    .WithTargeting() // Uses DefaultHttpTargetingAccessor, adds HttpContextAccessor if it's not added already
    .AddApplicationInsightsTelemetry(); // Adds TargetingTelemetryInitializer as well

var app = builder.Build();

app.UseMiddleware<TargetingHttpContextMiddleware>();
rossgrambo commented 3 months ago

Telemetry Publisher vs Initializer relationship:

I think it's okay to add these together- because it's very unlikely someone will want the publisher and not want the initializer.

jimmyca15 commented 3 months ago

I see three improvements here

(1) Add httpcontext accessor in WithTargeting (2) Add telemetry initializer when adding telemetry publisher (3) Add syntax sugar helper for middleware

(1) Agree to have it. The default targeting context accessor doesn't make sense without http context accessor being present. (2) Agree to have it. The scenario for feature management telemetry is to emit targeting id if present. (3) Doesn't seem helpful enough to me at this point to have it.

rossgrambo commented 3 months ago

Adjusted name of adding App Insights and removed middleware helper. Adjusted PR description to reflect the latest.

.AddApplicationInsightsTelemetry~Publisher~(); ~app.UseFeatureManagement();~