-
Notifications
You must be signed in to change notification settings - Fork 118
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
Changes from 8 commits
31764aa
91aba75
8d7abcf
2580625
512721f
f654715
7cd5aa3
9e68841
9b6eb53
4250726
62bbd17
f669d09
dbd6738
cdf5ac3
4579aef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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", | ||
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,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 AddAppInsightsTelemetryPublisher(this IFeatureManagementBuilder builder) | ||||||||||||
rossgrambo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
{ | ||||||||||||
if (builder == null) | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add these null check for the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you considered using one of the
Suggested change
I'm assuming the order is not important here. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||
|
||||||||||||
return builder; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If using the suggestion above, this can also be inlined:
Suggested change
|
||||||||||||
} | ||||||||||||
} | ||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ | |
|
||
<ItemGroup> | ||
<PackageReference Include="Microsoft.ApplicationInsights" Version="2.22.0" /> | ||
<PackageReference Include="Microsoft.Extensions.Hosting.Abstractions" Version="8.0.0" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We refers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @zhiyuanliang-ms There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
|
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 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.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.
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 inMicrosoft.FeatureManagement.Telemetry.ApplicationInsights
. I could expose it as a public or InternalsVisibleToAttribute to share between the packages but it didn't feel valuable enough.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.
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.