search

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

TelemetryInitializer, Middleware, and TargetingContext rework #427

Closed rossgrambo closed 7 months ago

rossgrambo commented 7 months ago

Why this PR?

Primarily a refactor. Also resolves https://github.com/microsoft/FeatureManagement-Dotnet/issues/424. Related to- but explicitly not resolving here since the middleware was moved out of the main AspNetCore package https://github.com/microsoft/FeatureManagement-Dotnet/issues/396.

ITargetingContextAccessor is async and could not used reached by ITelemetryInitializer. We currently get around this by having our middleware add TargetingContext to HttpContext so it can be read synchronously.

This PR adjusts the middleware to instead place TargetingId on the RequestTelemetry object instead. This object is specific to Application Insights and we were already adding TargetingId to it with our existing initializer. By adding this earlier, we allow the initializer to depend on this field rather than the key on HttpContext. It also allows us to simply use the "TargetingId" key, rather than having a custom one for HttpContext.

This also allows ITargetingContextAccessor to cache however it wants and we're not caching in multiple places.

In short: Middleware stores on RequestTelemetry instead of HttpContext. Initializer now simply moves fields from RequestTelemetry to other Telemetry (like custom events).

Visible Changes

HttpContext will no longer have the key "Microsoft.FeatureManagement.TargetingId" added to HttpContext.Items.

rossgrambo commented 7 months ago

Met with Zhenlan offline. My takeaway is:

Our current preview release is placing TargetingId on the RequestTelemetry. However Split is not reading RequestTelemetry today, so it's not currently helpful to experimentation to be on there.

Should we be setting TargetingId on RequestTelemetry at all?

If the answer is no- then this PR is not the correct way to solve this problem and we should use a cache. If the answer is yes- this PR is a valid way to persist the TargetingId to custom events without needing a cache.

Currently I'm on the fence if we should add it. It's not helpful to split yet- but it could be helpful in App Insights today. If we go with "no" and pursue a caching solution instead, would adding TargetingId to RequestTelemetry 6 months from now make the cache obsolete? If so, our caching solution better not be a breaking change to take away.

zhiyuanliang-ms commented 7 months ago

Should we be setting TargetingId on RequestTelemetry at all?

Comparing to the current way (putting it in httpcontext.items), I will vote for setting TargetingId on RequestTelemetry, since it is the recommanded way in App Insights docs.

zhenlan commented 7 months ago

I am bit on the fence regarding whether we should add TargetingId to RequestTelemetry because we don't have any out-of-box usage scenario for RequestTelemetry today, but I'm not opposed to it. It could be handy for customers if they want to correlate feature evaluation with request telemetry. Of course, it could be handy for customers if we add TargetingId to all other ASP.NET telemetry (e.g. performance counters). I almost feel we should give customers an option to opt-in/out.

Regardless of whether we add TargetingId to RequestTelemetry (and other ASP.NET telemetry), it doesn't feel right to use RequestTelemetry as a caching mechanism in the middleware. We already have an initializer, and we must add TargetingId to the custom events. Even if we want to add TargetingId to RequestTelemetry (and any other ASP.NET telemetry), we should add it in the initializer.

rossgrambo commented 7 months ago

I agree, customers should probably be able to opt out of it. And if that's something we might offer- we would still need the custom events to be populated with it. I think that's a good argument for decoupling these. Going to close this PR.

jimmyca15 commented 7 months ago

I agree, customers should probably be able to opt out of it. And if that's something we might offer- we would still need the custom events to be populated with it. I think that's a good argument for decoupling these. Going to close this PR.

This PR moves the middleware into the asp.net core project though. Will you be opening a new PR for that?

rossgrambo commented 7 months ago

Yes I will. Going to solidify the shape of the caching first- since it has implications on what projects the middleware depends on.

For example, with the changes in this PR- it made a lot of sense to place the middleware under App Insights. I want to make sure that's still the case after changes.