-
Notifications
You must be signed in to change notification settings - Fork 114
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 4 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,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); | ||
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. Is there any reason why we do not follow the opentelemtry's implementation This hosted service is aimed to ensure that the Instead of injecting
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. There's a few things to talk about here- one, is that linked OpenTelemetry implementation is actually setting up their
However, we are instead using the The main question is, who should dispose the publisher? The options to me are:
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. 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 we just need some hook to create the instance, could an option 3 using a Contrary to HostedService, it also executes before the application actually starts listening to anything, which might actually be a positive 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.
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 you are absolutely right @rossgrambo , I honestly completely forgot about that important detail when suggesting it 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. @rossgrambo I assume you meant that it is limited to Regardless, I'm curious where you found that. The page for the interface says it is supported since v6: Can you elaborate? 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. 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? 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. Oh okay I found the issue. The I could branch the logic on which .NET we're in- but I think I'll just stick with 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. *Updated to use service provider here as @zhiyuanliang-ms suggested 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.
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) | ||||||||||||
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 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
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.