Skip to content
This repository has been archived by the owner on Jun 10, 2020. It is now read-only.

Ability to add telemetry processors through DI #445

Merged
merged 2 commits into from
May 16, 2017
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
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
namespace Microsoft.Extensions.DependencyInjection
{
using System;
using System.Linq;
using System.Collections.Generic;
using Microsoft.ApplicationInsights.AspNetCore.Extensions;
using Microsoft.ApplicationInsights.Channel;
Expand All @@ -21,6 +22,7 @@ internal class TelemetryConfigurationOptionsSetup : IConfigureOptions<TelemetryC
private readonly IEnumerable<ITelemetryInitializer> initializers;
private readonly IEnumerable<ITelemetryModule> modules;
private readonly ITelemetryChannel telemetryChannel;
private readonly IEnumerable<Func<ITelemetryProcessor, ITelemetryProcessor>> telemetryProcessorFactories;

/// <summary>
/// Initializes a new instance of the <see cref="T:TelemetryConfigurationOptionsSetup"/> class.
Expand All @@ -29,11 +31,13 @@ public TelemetryConfigurationOptionsSetup(
IServiceProvider serviceProvider,
IOptions<ApplicationInsightsServiceOptions> applicationInsightsServiceOptions,
IEnumerable<ITelemetryInitializer> initializers,
IEnumerable<ITelemetryModule> modules)
IEnumerable<ITelemetryModule> modules,
IEnumerable<Func<ITelemetryProcessor, ITelemetryProcessor>> telemetryProcessorFactories)
{
this.applicationInsightsServiceOptions = applicationInsightsServiceOptions.Value;
this.initializers = initializers;
this.modules = modules;
this.telemetryProcessorFactories = telemetryProcessorFactories;
this.telemetryChannel = serviceProvider.GetService<ITelemetryChannel>();
}

Expand All @@ -45,6 +49,15 @@ public void Configure(TelemetryConfiguration configuration)
configuration.InstrumentationKey = this.applicationInsightsServiceOptions.InstrumentationKey;
}

if (this.telemetryProcessorFactories.Any())
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I wonder if we should remove the if statement altogether and let it fall through to the (potentially empty) foreach followed by .Build(). In the case where users have manually added to the TelemetryProcessorChainBuilder, someone needs to call .Build() and this might be the most appropriate place.

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 think whoever adds manually to the TelemetryProcessorChainBuilder should also take care of calling Build(). We do, so we call. Calling Build() "just in case" is a little dubious. The original code did not do that and I did not want to change this.

Copy link
Member

Choose a reason for hiding this comment

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

OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, quick pulse telemetry module and telemetry processor need to know about each other. This link between them will not survive thru the re-Build(). So Initialize on module should be called after Build on the chain.

Can this be achieved in this case?

(real solution will be to change the design of quick pulse)

Copy link
Member

Choose a reason for hiding this comment

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

@karolz-ms and @pharring Would we have a "race" condition where someone could add a processor factory after we call Build()? Something like the issue with calling both UseApplicationInsights and AddApplicationInsightsTelemetry?

Copy link
Contributor Author

@karolz-ms karolz-ms May 16, 2017

Choose a reason for hiding this comment

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

@SergeyKanzhelev The code that this change is adding runs before quick pulse telemetry module and processor are added/initialized. So I believe we are fine here?

I agree that quick pulse implementation should probably be changed. This dependency between its telemetry module and telemetry processor is making things unnecessarily fragile. Do you want me to open an issue for this?

@dnduffy In general TelemetryProcessorChainBuilder.Use() is just adding a TP factory to the chain builder. The Build() call is what instantiates the chain. Each time Build() is called the chain is instantiated from scratch, or at least this is the current implementation. So it is fine to Use(TP factory), then call Build(), then Use(another TP factory), then call Build() again. That is--IF there are no dependencies like quick pulse has today. But that is a separate issue that I am not trying to address here.

Copy link
Member

Choose a reason for hiding this comment

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

I just realized what I was thinking of, it wasn't the Use/Build of the processor chain but the fact that internal helper methods were causing the re-adding of the extra processors etc.

{
foreach (Func<ITelemetryProcessor, ITelemetryProcessor> processorFactory in this.telemetryProcessorFactories)
{
configuration.TelemetryProcessorChainBuilder.Use(processorFactory);
}
configuration.TelemetryProcessorChainBuilder.Build();
}

this.AddTelemetryChannelAndProcessorsForFullFramework(configuration);

configuration.TelemetryChannel = this.telemetryChannel ?? configuration.TelemetryChannel;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,19 @@ public static void RegistersTelemetryConfigurationFactoryMethodThatPopulatesItWi
Assert.True(dependencyModule.ExcludeComponentCorrelationHttpHeadersOnDomains.Count > 0);
}

[Fact]
public static void RegistersTelemetryConfigurationFactoryMethodThatPopulatesItWithTelemetryProcessorFactoriesFromContainer()
{
var services = ApplicationInsightsExtensionsTests.GetServiceCollectionWithContextAccessor();
services.AddSingleton<Func<ITelemetryProcessor, ITelemetryProcessor>>((next) => new FakeTelemetryProcessor(next));

services.AddApplicationInsightsTelemetry(new ConfigurationBuilder().Build());

IServiceProvider serviceProvider = services.BuildServiceProvider();
var telemetryConfiguration = serviceProvider.GetTelemetryConfiguration();
Assert.Contains(telemetryConfiguration.TelemetryProcessors, (processor) => processor is FakeTelemetryProcessor);
}

#if NET451
[Fact]
public static void AddsAddaptiveSamplingServiceToTheConfigurationInFullFrameworkByDefault()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
namespace Microsoft.ApplicationInsights.AspNetCore.Tests
{
using System;
using Microsoft.ApplicationInsights.Channel;
using Microsoft.ApplicationInsights.Extensibility;

internal class FakeTelemetryProcessor : ITelemetryProcessor
{
private readonly ITelemetryProcessor next;

public FakeTelemetryProcessor(ITelemetryProcessor next)
{
if (next == null)
{
throw new ArgumentNullException(nameof(next));
}

this.next = next;
}

public void Process(ITelemetry item)
{
this.next.Process(item);
}
}
}