-
Notifications
You must be signed in to change notification settings - Fork 120
Ability to add telemetry processors through DI #445
Ability to add telemetry processors through DI #445
Conversation
@karolz-ms, The agreement will cover your contributions to all Microsoft-managed open source projects. |
@@ -45,6 +49,15 @@ public void Configure(TelemetryConfiguration configuration) | |||
configuration.InstrumentationKey = this.applicationInsightsServiceOptions.InstrumentationKey; | |||
} | |||
|
|||
if (this.telemetryProcessorFactories.Count() > 0) |
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.
Use this.telemetryProcessorFactories.Any()
(it short-circuits the enumeration once it hits the first item)
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 point
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.
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.
@karolz-ms, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
@@ -45,6 +49,15 @@ public void Configure(TelemetryConfiguration configuration) | |||
configuration.InstrumentationKey = this.applicationInsightsServiceOptions.InstrumentationKey; | |||
} | |||
|
|||
if (this.telemetryProcessorFactories.Any()) |
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.
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.
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.
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.
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.
OK.
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.
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)
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.
@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?
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.
@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.
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.
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.
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.
This is a great start but:
- aren't we missing some default setup? Like what about connecting with AddTelemetryChannelAndProcessorsForFullFramework?
- 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.
|
Relevant to #344. |
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.