-
Notifications
You must be signed in to change notification settings - Fork 115
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
Activity Based TargetingId Persistance #467
Conversation
…atureManagement.Telemetry.AspNetCore project
I don't see any files removed in the diff |
src/Microsoft.FeatureManagement.AspNetCore/TargetingHttpContextMiddleware.cs
Show resolved
Hide resolved
src/Microsoft.FeatureManagement.AspNetCore/TargetingHttpContextMiddleware.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.FeatureManagement.Telemetry.ApplicationInsights/TargetingTelemetryInitializer.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
// Telemetry.Properties is deprecated in favor of ISupportProperties | ||
if (telemetry is ISupportProperties telemetryWithSupportProperties) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a case that this wouldn't be true, perhaps if a user used an old version of app insights?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is no, that they will always be available. The deprecation message for telemetry.Properties says:
TelemetryContext.Properties is obsolete. Use GlobalProperties to set global level properties. For properties at item level, use ISupportProperties.Properties.
They were added in v2.21.0. .NET 6 and above should use at least v2.21.0, so I think we can assume this will be available. The versions before that are all deprecated (actually including v2.21.0 as well). v2.22.0 is the latest.
Updated to remove Microsoft.FeatureManagement.Telemetry.ApplicationInsights.AspNetCore |
Any other concerns? |
@@ -50,14 +53,15 @@ public async Task InvokeAsync(HttpContext context, ITargetingContextAccessor tar | |||
|
|||
if (targetingContext != null) | |||
{ | |||
context.Items[TargetingIdKey] = targetingContext.UserId; | |||
var activityFeature = httpContext.Features.Get<IHttpActivityFeature>(); | |||
activityFeature?.Activity?.AddBaggage(TargetingIdKey, targetingContext.UserId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null check on activity feature is a safeguard, but it's still unexpected right? I'm thinking we should log a warning if it's null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted to:
var activityFeature = httpContext.Features.Get<IHttpActivityFeature>();
if (activityFeature == null)
{
_logger.LogDebug("The IHttpActivityFeature from the IFeatureCollection was null");
}
else if (activityFeature.Activity == null)
{
_logger.LogDebug("The Activity on the IHttpActivityFeature was null");
} else
{
activityFeature.Activity.AddBaggage(TargetingIdKey, targetingContext.UserId);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a debug log. You don't consider it worth a warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was following suit on the other null check that causes the middleware to no op:
_logger.LogDebug("The targeting context accessor returned a null TargetingContext");
But I suppose these two are more unexpected than that one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I suppose these two are more unexpected than that one?
Yes, I would say that. And also I believe we didn't want to log warning in the middleware for not having a targeting context because the idea was to consider the middleware may be in use even if targeting is not enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR: #471
Why this PR?
Activity.Baggage
instead ofHttpContext.Items
to store Targeting information.Microsoft.FeatureManagement.ApplicationInsights.AspNetCore
packageVisible Changes
The
Microsoft.FeatureManagement.ApplicationInsights.AspNetCore
will no longer be a nuget package deployed. The initializer has moved toMicrosoft.FeatureManagement.ApplicationInsights
.Migrating
Developers using the initializer will need to change their using statement from
to
There are no changes to the line that adds the initializer or anywhere else. This line stays: