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

Telemetry Publishing via Activity & Activity Event #455

Merged
merged 15 commits into from
Jun 19, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions examples/EvaluationDataToApplicationInsights/Program.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
//
using Microsoft.FeatureManagement.Telemetry;
using Microsoft.FeatureManagement;
using EvaluationDataToApplicationInsights;
using Microsoft.ApplicationInsights.Extensibility;
Expand Down Expand Up @@ -31,7 +30,7 @@
// Wire up evaluation event emission
builder.Services.AddFeatureManagement()
.WithTargeting<HttpContextTargetingContextAccessor>()
.AddTelemetryPublisher<ApplicationInsightsTelemetryPublisher>();
.AddAppInsightsTelemetryPublisher();

//
// Default code from .NET template below
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
using Microsoft.ApplicationInsights;
using System.Diagnostics;

namespace Microsoft.FeatureManagement.Telemetry.ApplicationInsights
{
/// <summary>
/// Listens to <see cref="Activity"/> events from feature management and sends them to Application Insights.
/// </summary>
internal sealed class ApplicationInsightsEventPublisher : IDisposable
{
private readonly TelemetryClient _telemetryClient;
private readonly ActivityListener _activityListener;

/// <summary>
/// Initializes a new instance of the <see cref="ApplicationInsightsEventPublisher"/> class.
/// </summary>
/// <param name="telemetryClient">The Application Insights telemetry client.</param>
public ApplicationInsightsEventPublisher(TelemetryClient telemetryClient)
{
_telemetryClient = telemetryClient ?? throw new ArgumentNullException(nameof(telemetryClient));

_activityListener = new ActivityListener
{
ShouldListenTo = (activitySource) => activitySource.Name == "Microsoft.FeatureManagement",
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Sample = (ref ActivityCreationOptions<ActivityContext> options) => ActivitySamplingResult.AllData,
ActivityStopped = (activity) =>
{
ActivityEvent? evaluationEvent = activity.Events.FirstOrDefault((activityEvent) => activityEvent.Name == "feature_flag");

if (evaluationEvent != null && evaluationEvent.Value.Tags.Any())
rossgrambo marked this conversation as resolved.
Show resolved Hide resolved
{
HandleActivityEvent(evaluationEvent.Value);
rossgrambo marked this conversation as resolved.
Show resolved Hide resolved
}
}
};

ActivitySource.AddActivityListener(_activityListener);
}

/// <summary>
/// Disposes the resources used by the <see cref="ApplicationInsightsEventPublisher"/>.
/// </summary>
public void Dispose()
{
_activityListener.Dispose();
}

private void HandleActivityEvent(ActivityEvent activityEvent)
{
var properties = new Dictionary<string, string>();

foreach (var tag in activityEvent.Tags)
{
properties[tag.Key] = tag.Value?.ToString();
rossgrambo marked this conversation as resolved.
Show resolved Hide resolved
}

_telemetryClient.TrackEvent("FeatureEvaluation", properties);
rossgrambo marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
using Microsoft.ApplicationInsights;
using Microsoft.Extensions.Hosting;

namespace Microsoft.FeatureManagement.Telemetry.ApplicationInsights
{
/// <summary>
/// A hosted service used to construct and dispose the <see cref="ApplicationInsightsEventPublisher"/>
/// </summary>
internal sealed class ApplicationInsightsHostedService : IHostedService
{
private readonly TelemetryClient _telemetryClient;
private ApplicationInsightsEventPublisher appInsightsPublisher;

/// <summary>
/// Initializes a new instance of the <see cref="ApplicationInsightsHostedService"/> class.
/// </summary>
/// <param name="telemetryClient">The <see cref="TelemetryClient"/> instance used for telemetry.</param>
public ApplicationInsightsHostedService(TelemetryClient telemetryClient)
{
_telemetryClient = telemetryClient ?? throw new ArgumentNullException(nameof(telemetryClient));
}

/// <summary>
/// Constructs the <see cref="ApplicationInsightsEventPublisher"/> which will start listening for events.
/// </summary>
/// <param name="cancellationToken">The cancellation token.</param>
/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
public Task StartAsync(CancellationToken cancellationToken)
{
appInsightsPublisher = new ApplicationInsightsEventPublisher(_telemetryClient);
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 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.

Copy link
Contributor Author

@rossgrambo rossgrambo May 31, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@rossgrambo rossgrambo Jun 11, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


return Task.CompletedTask;
}

/// <summary>
/// Disposes of the <see cref="ApplicationInsightsEventPublisher"/>.
/// </summary>
/// <param name="cancellationToken">The cancellation token.</param>
/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
public Task StopAsync(CancellationToken cancellationToken)
{
appInsightsPublisher.Dispose();
appInsightsPublisher = null;

return Task.CompletedTask;
}
}
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
//
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.FeatureManagement.Telemetry.ApplicationInsights;

namespace Microsoft.FeatureManagement
{
/// <summary>
/// Extensions used to add feature management functionality.
/// </summary>
public static class FeatureManagementBuilderExtensions
{
/// <summary>
/// Adds the <see cref="ApplicationInsightsEventPublisher"/> using <see cref="ApplicationInsightsHostedService"/> to the feature management builder.
/// </summary>
/// <param name="builder">The feature management builder.</param>
/// <returns>The feature management builder.</returns>
public static IFeatureManagementBuilder AddAppInsightsTelemetryPublisher(this IFeatureManagementBuilder builder)
rossgrambo marked this conversation as resolved.
Show resolved Hide resolved
{
if (builder == null)
Copy link
Contributor

@zhiyuanliang-ms zhiyuanliang-ms Jun 12, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

{
throw new ArgumentNullException(nameof(builder));
}

if (builder.Services == null)
{
throw new ArgumentNullException(nameof(builder.Services));
}
rossgrambo marked this conversation as resolved.
Show resolved Hide resolved

if (!builder.Services.Any((ServiceDescriptor d) => d.ServiceType == typeof(IHostedService) && d.ImplementationType == typeof(ApplicationInsightsHostedService)))
{
builder.Services.Insert(0, ServiceDescriptor.Singleton<IHostedService, ApplicationInsightsHostedService>());
}
Comment on lines +34 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered using one of the TryAdd... registration methods instead? I believe it would take care for you of ensuring only a single registration exists for that specific type without the need to manually inspect registered services manually.

Suggested change
if (!builder.Services.Any((ServiceDescriptor d) => d.ServiceType == typeof(IHostedService) && d.ImplementationType == typeof(ApplicationInsightsHostedService)))
{
builder.Services.Insert(0, ServiceDescriptor.Singleton<IHostedService, ApplicationInsightsHostedService>());
}
builder.Services.TryAddSingleton<IHostedService, ApplicationInsightsHostedService>();

I'm assuming the order is not important here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Current implementation referes to the open telemetry implementation. here

But I am also wondering why open telemetry wants to ensure the hosted service to be the first one.

Copy link
Contributor Author

@rossgrambo rossgrambo May 31, 2024

Choose a reason for hiding this comment

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

I was also curious about this- but the reason is so Telemetry is setup before other IHostedServices. This doesn't guarantee it but it's better than never capturing the telemetry. Here's OpenTelemetry's reasoning

Copy link
Contributor

Choose a reason for hiding this comment

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

If this starts to become such a common concern, we should consider asking for a dedicated ordering API for hosted services in the future.

Forcing a position feels like a hack to me right now. I don't think the IServiceCollection API was made to be inspected and manipulated like this.


return builder;
Copy link
Contributor

Choose a reason for hiding this comment

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

If using the suggestion above, this can also be inlined:

Suggested change
return builder;
return builder.Services.TryAddSingleton<IHostedService, ApplicationInsightsHostedService>();;

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

<ItemGroup>
<PackageReference Include="Microsoft.ApplicationInsights" Version="2.22.0" />
<PackageReference Include="Microsoft.Extensions.Hosting.Abstractions" Version="8.0.0" />
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

</ItemGroup>

<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,32 +76,5 @@ public static IFeatureManagementBuilder WithVariantService<TService>(this IFeatu

return builder;
}

/// <summary>
/// Adds a telemetry publisher to the feature management system.
/// </summary>
/// <param name="builder">The <see cref="IFeatureManagementBuilder"/> used to customize feature management functionality.</param>
/// <returns>A <see cref="IFeatureManagementBuilder"/> that can be used to customize feature management functionality.</returns>
public static IFeatureManagementBuilder AddTelemetryPublisher<T>(this IFeatureManagementBuilder builder) where T : ITelemetryPublisher
{
builder.AddTelemetryPublisher(sp => ActivatorUtilities.CreateInstance(sp, typeof(T)) as ITelemetryPublisher);

return builder;
}

private static IFeatureManagementBuilder AddTelemetryPublisher(this IFeatureManagementBuilder builder, Func<IServiceProvider, ITelemetryPublisher> factory)
{
builder.Services.Configure<FeatureManagementOptions>(options =>
{
if (options.TelemetryPublisherFactories == null)
{
options.TelemetryPublisherFactories = new List<Func<IServiceProvider, ITelemetryPublisher>>();
}

options.TelemetryPublisherFactories.Add(factory);
});

return builder;
}
}
}
6 changes: 0 additions & 6 deletions src/Microsoft.FeatureManagement/FeatureManagementOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,5 @@ public class FeatureManagementOptions
/// The default value is true.
/// </summary>
public bool IgnoreMissingFeatures { get; set; } = true;

/// <summary>
/// Holds a collection of factories that can be used to create <see cref="ITelemetryPublisher"/> instances.
/// This avoids the need to add the publishers to the service collection.
/// </summary>
internal ICollection<Func<IServiceProvider, ITelemetryPublisher>> TelemetryPublisherFactories { get; set; }
}
}
Loading