Telemetry Publishing via Activity & Activity Event

[rossgrambo]

Why this PR?

#412

We now have our own ActivitySource and add an ActivityEvent on it. The event is essentially the EvaluationEvent that was previously passed to the Publisher.

Why our own ActivitySource instead of using the parent Activity?

We could emit the ActivityEvent onto whatever parent Activity is running. Sadly this approach has a few drawbacks:

1. Listeners who want this event now have to listen to all activities.
2. The parent code may not have an activity
3. If a customer implements slow async logic (like reading targeting context from a db), it’s no longer grouped within FeatureManagement.

Visible Changes

1. Adding publishing uses a different method:

builder.Services.AddFeatureManagement()
    .WithTargeting<HttpContextTargetingContextAccessor>()
    .AddTelemetryPublisher<ApplicationInsightsTelemetryPublisher>();

Becomes

builder.Services.AddFeatureManagement()
    .WithTargeting<HttpContextTargetingContextAccessor>()
    .AddAppInsightsTelemetryPublisher();

2. Public types ApplicationInsightsTelemetryPublisher & ITelemetryPublisher have been removed entirely
3. An Activity and Activity Event are available to listen to. Here's the schemas at a glance:

Activity: {
  Name: "Microsoft.FeatureManagement",
  Version: "1.0.0"
}

ActivityEvent: {
  Tags: {
    "TargetingId", evaluationEvent.TargetingContext?.UserId,
    "FeatureName", evaluationEvent.FeatureDefinition.Name,
    "Variant", evaluationEvent.Variant?.Name,
    "Enabled", evaluationEvent.Enabled,
    "VariantAssignmentReason", evaluationEvent.VariantAssignmentReason,
    "Version", ActivitySource.Version,
    ... // (Any items added to TelemetryMetadata)
  }
}

[julealgon]

The value "Microsoft.FeatureManagement" is mentioned twice in different places. Consider extracting a proper constant for it that can be shared for type safety and refactor-friendliness.

[rossgrambo]

Good callout. The problem to me is that they're currently in different packages. One string is in the primary Microsoft.FeatureManagement while the other is in Microsoft.FeatureManagement.Telemetry.ApplicationInsights. I could expose it as a public or InternalsVisibleToAttribute to share between the packages but it didn't feel valuable enough.

[julealgon]

internal + InternalsVisibleTo would be the best in this case indeed to avoid overexposing it. I see it is a bit convoluted though still, probably not worth it as you said.

[julealgon]

Consider evaluating the DateTimeOffset.Now closer to the actual flag check and passing it as an argument so it reflects the actual moment a bit more accurately. By evaluating here, any added delay from the extra processing would end up influencing the recorded time.

Maybe grab it even before the various ISessionManager calls (I don't know exactly what that abstraction is responsible for so take with a grain of salt).

[rossgrambo]

That would be more accurate for when the actual check started, but I think we'd rather be as close as possible to the return of the final result. Reason being so metrics emitted at or after this timestamp can be expected to use the new result. If we did it at the beginning and their implementation of ISessionManager was slow, the timestamps would be less reliable to correlate.

[rossgrambo]

Although- I could use the end of the activity to get the timestamp I'm referring to, not sure if that would be as reliable though. But in that same line of thinking, the start of the activity could be the considered the start.

[zhiyuanliang-ms]

We refers Microsoft.Extensions.Logging instead of the abstraction package in Microsoft.FeatureManagement project.

[julealgon]

@zhiyuanliang-ms Microsoft.Extensions.Hosting.Abstractions is required here because of the hosted service usage. If the project can reference just the Abstractions package, that will generate less dependencies for consumers which is a good thing.

[zhiyuanliang-ms]

I mean, in Microsoft.FeatureManagement.csproj we referenceMicrosoft.Extensions.Logging instead of Microsoft.Extensions.Logging.Abstraction`. we can change it to abstraction as well.

[julealgon]

Ah gotcha. If possible, that would make sense indeed, for the same reason.

[rossgrambo]

I'll hold off on that change in this PR but agreed we should investigate.

[zhiyuanliang-ms]

Is there any reason why we do not follow the opentelemtry's implementation

This hosted service is aimed to ensure that the ApplicationInsightsEventPublisher will be instantiated. Besides, we can take advantage of the DI system to manage the lifecycle of it.

Instead of injecting TelemetryClient to the HostedService, IServiceProvider is injected and serviceProvider.GetService() is called.

AddAppInsightsTelemetryPublisher method should also register ApplicationInsightsEventPublisher as singleton.

[rossgrambo]

There's a few things to talk about here- one, is that linked OpenTelemetry implementation is actually setting up their ActivitySources, not ActivityListeners. Their call to setup the listeners is a part of .WithTracing()

WithTracing creates a few things, but it eventually constructs TracerProviderSdk which creates the listeners during construction and explicitly has a shutdown function and a dispose function to handle disposing the listener: https://github.com/open-telemetry/opentelemetry-dotnet/blob/c38cc273bcd03949897d1585e9bc083962cb5f22/src/OpenTelemetry/Trace/TracerProviderSdk.cs#L352

However, we are instead using the IHostedService to handle the "shutdown" (OnStop) and dispose options for our ActivityListeners. It appears I forgot this- because at some point after my design I ripped out IDisposable on the IHostedService- which means in its current state, this would not dispose correctly in a bad exit where OnStop isn't called. 😅

The main question is, who should dispose the publisher? The options to me are:

1. IHostedService. If so- it should dispose it appropriately on both shutdown and when sp disposes the IHostedService.
2. ServiceProvider. If so- the disposal will happen automatically which is clean. However- this means we also have an IHostedService that stays around in the background for no reason. Stopping the hosted service would do nothing and disposing it does nothing but remove it. There's also no clear way to undo the IHostedService start without doing a full disposal of the entire ServiceProvider.

So in short- it came down to preference. I ended up landing on 1, but there's been a bit of pushback about it, so I'll likely switch over to 2.

[julealgon]

If we just need some hook to create the instance, could an option 3 using a IStartupFilter implementation also work? That interface provides IServiceProvider, is executed in the beginning of the pipeline, and doesn't "linger" like the hosted service would.

Contrary to HostedService, it also executes before the application actually starts listening to anything, which might actually be a positive here.

[rossgrambo]

IStartupFilter would be a great fit here- the issue is that it's only available in ASP.NET Core. Currently this telemetry publishing is possible without any ASP.NET dependency.

[julealgon]

Ah you are absolutely right @rossgrambo , I honestly completely forgot about that important detail when suggesting it here 😅.

[julealgon]

@rossgrambo I assume you meant that it is limited to Microsoft.Extensions.Hosting.Abstractions v8+. This doesn't seem to be tied to the framework but to the library itself. Supporting it would be a matter of bumping the dependency on Microsoft.FeatureManagement.Telemetry.ApplicationInsights.

Regardless, I'm curious where you found that. The page for the interface says it is supported since v6:
https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.hosting.ihostedlifecycleservice?view=net-8.0#applies-to

Can you elaborate?

[rossgrambo]

Oh I may have been mislead by the article then. The article linked earlier mentions it a couple times as a "new thing in .NET 8". Even the title says it's new in .NET 8. When searching for it- I saw the same mention of .NET 8 in other articles too.

But I just implemented it and ran it successfully with .net 6 and net48. So I suppose it's available?

[rossgrambo]

Oh okay I found the issue. The StartingAsync and StoppingAsync methods aren't called unless the project is running >= .NET8.

I could branch the logic on which .NET we're in- but I think I'll just stick with the HostedService StartAsync for now- unless there's a good way around this.

[rossgrambo]

*Updated to use service provider here as @zhiyuanliang-ms suggested

[julealgon]

Oh okay I found the issue. The StartingAsync and StoppingAsync methods aren't called unless the project is running >= .NET8.

Ohhh that is very weird! Documentation on this interface should be updated to reflect this limitation I think!

Sorry for leading you down this path @rossgrambo , I had no idea.

[zhiyuanliang-ms]

Could you add these null check for the WithTargeting extension method? It is fine if we want to put it in a new PR.

[rossgrambo]

Let's do another PR- because I'm not even sure this null check makes sense. We're using an extension parameter, which I believe means this method couldn't even be called unless the instance existed.

[jimmyca15]

Thanks for the thorough review on this @julealgon !

[jimmyca15]

Why does this use snake casing instead of pascal like all the other fields?

[rossgrambo]

This is following OpenTelemetry's guide for the naming and I didn't feel strongly to deviate. It made more sense when we were emitting both OpenTelemetry and FeatureManagement fields, but I still don't feel we need to deviate.

[jimmyca15]

This would be the only place where any external naming convention is used, and the naming deviates from the casing of all of our tags. Seems that it should be FeatureFlag

[rossgrambo]

Makes sense. PR: #464