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

Ability to add telemetry processors through DI #445

merged 2 commits into from
May 16, 2017

Conversation

karolz-ms
Copy link
Contributor

This is necessary to complete exception snapshotting work for .NET Core and overall a good addition, filling a gap in programmatic initialization capabilities of AI SDK.

@msftclas
Copy link

@karolz-ms,
Thanks for your contribution. It looks like you are a Microsoft vendor. To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

The agreement will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

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

if (this.telemetryProcessorFactories.Count() > 0)
Copy link
Member

Choose a reason for hiding this comment

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

Use this.telemetryProcessorFactories.Any() (it short-circuits the enumeration once it hits the first item)

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 point

Copy link
Member

Choose a reason for hiding this comment

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

Actually, perhaps this if statement is unnecessary. If the imported telemetryProcessorFactories is empty, then the foreach does nothing, but I think we should still call .Build() for the case where users have manually added processors to the TelemetryProcessorChainBuilder. Someone needs to call .Build() and this might be the right place.

@msftclas
Copy link

@karolz-ms, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

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

Copy link
Member

@dnduffy dnduffy left a comment

Choose a reason for hiding this comment

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

This is a great start but:

  1. aren't we missing some default setup? Like what about connecting with AddTelemetryChannelAndProcessorsForFullFramework?
  2. Will we get multiple copies of some processors if Build is called more than once? This seemed to happen with the previous issue where UseApplicationInsights and AddApplicationInsightsTelemetry were not idempotent.

@karolz-ms
Copy link
Contributor Author

@dnduffy

  1. Not sure what default setup might be missing... For AddTlemetryChannelAndProcessorsForFullFramework, the TP factories that that method adds will be added to the same TelemetryProcessorChainBuilder, so the user factories and our predefined factories will be used simultaneously to build the final chain.

  2. Nope, as I explained above, each call to Build() will result in a brand-new chain. The old chain will be discarded, see https://github.com/Microsoft/ApplicationInsights-dotnet/blob/develop/src/Core/Managed/Shared/Extensibility/Implementation/TelemetryProcessorChainBuilder.cs#L71

@dnduffy
Copy link
Member

dnduffy commented May 16, 2017

Relevant to #344.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants