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 all 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>();
.AddApplicationInsightsTelemetryPublisher();

//
// 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.HasValue && evaluationEvent.Value.Tags.Any())
{
HandleFeatureFlagEvent(evaluationEvent.Value);
}
}
};

ActivitySource.AddActivityListener(_activityListener);
}

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

private void HandleFeatureFlagEvent(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,44 @@
using Microsoft.Extensions.DependencyInjection;
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 IServiceProvider _serviceProvider;

/// <summary>
/// Initializes a new instance of the <see cref="ApplicationInsightsHostedService"/> class.
/// </summary>
/// <param name="serviceProvider">The <see cref="IServiceProvider"/> to get the publisher from.</param>
public ApplicationInsightsHostedService(IServiceProvider serviceProvider)
{
_serviceProvider = serviceProvider ?? throw new ArgumentNullException(nameof(serviceProvider));
}

/// <summary>
/// Uses the service provider to construct a <see cref="ApplicationInsightsEventPublisher"/> which will start listening for activities.
/// </summary>
/// <param name="cancellationToken">The cancellation token.</param>
/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
public Task StartAsync(CancellationToken cancellationToken)
{
_serviceProvider.GetService<ApplicationInsightsEventPublisher>();

return Task.CompletedTask;
}

/// <summary>
/// Stops this hosted service.
/// </summary>
/// <param name="cancellationToken">The cancellation token.</param>
/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
public Task StopAsync(CancellationToken cancellationToken)
{
return Task.CompletedTask;
}
}
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// 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 AddApplicationInsightsTelemetryPublisher(this IFeatureManagementBuilder builder)
{
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 ArgumentException($"The provided builder's services must not be null.", nameof(builder));
}

builder.Services.AddSingleton<ApplicationInsightsEventPublisher>();

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