Fix #3163: per-call TelemetryContext enrichment in Track*/StartOperation*#3181
Fix #3163: per-call TelemetryContext enrichment in Track*/StartOperation*#3181rajkumar-rangaraj wants to merge 4 commits into
Conversation
…ion* Constructing a second TelemetryClient against a TelemetryConfiguration that has already been built no longer throws InvalidOperationException. The non-DI TelemetryClient constructor no longer registers TelemetryContextActivityProcessor/TelemetryContextLogProcessor via PrependOpenTelemetryBuilderConfiguration. Instead, each TelemetryClient enriches its own telemetry with its per-instance TelemetryContext (typed fields + GlobalProperties) at the call site of Track*/StartOperation* using skip-if-present semantics so item-level values continue to take precedence. Precedence at the emission site is: item dict > GlobalProperties > typed Context. ApplicationInsightsHttpModule now shares a single TelemetryClient per AppDomain, created lazily on first OnBeginRequest, so Init() never triggers an OTel SDK Build (which can fail when no connection string is configured). Adds new tests: TelemetryClientMultiInstanceTests (6) and 2 HttpModule regression tests.
Adds a test that two TelemetryClients sharing a configuration each tag their StartOperation<T> Activity with their own per-instance Context (User.Id).
- CA1508 in ApplicationInsightsHttpModule: suppress around the inner null check of the double-check locking pattern (analyzer doesn't model the cross-thread write). - CS8632 in TelemetryContextActivityProcessorTests: drop the '?' nullable annotation on the cast since the test project isn't in a nullable-annotations context.
There was a problem hiding this comment.
Pull request overview
This PR fixes multi-TelemetryClient construction against an already-built TelemetryConfiguration (classic ASP.NET HttpModule + other non-DI scenarios) by removing per-constructor OpenTelemetry pipeline mutations and moving TelemetryClient.Context enrichment to the call sites of Track* / StartOperation*, ensuring per-client Context isolation.
Changes:
- Refactors
TelemetryClientto apply client context per call (properties +Activitytags) instead of registering global OTel processors in the non-DI ctor. - Updates classic ASP.NET
ApplicationInsightsHttpModuleto lazily create a single AppDomain-sharedTelemetryClienton first request. - Adds regression/unit tests for multi-instance construction and per-client context isolation; updates changelog.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| BASE/src/Microsoft.ApplicationInsights/TelemetryClient.cs | Removes non-DI pipeline mutation; adds per-call context enrichment helpers and threads client context through Track*/StartOperation paths. |
| BASE/src/Microsoft.ApplicationInsights/TelemetryClientExtensions.cs | Enriches freshly started activities with the calling client’s context. |
| WEB/Src/Web/Web/ApplicationInsightsHttpModule.cs | Switches to a lazily-created AppDomain-shared TelemetryClient on first request. |
| WEB/Src/Web/Web.Tests/ApplicationInsightsHttpModuleTests.cs | Adds regressions for #3163 + context isolation; resets new static shared client between tests. |
| BASE/Test/.../TelemetryClientMultiInstanceTests.cs | Adds multi-instance constructor + context isolation regressions across log/activity export paths. |
| CHANGELOG.md | Adds Unreleased entry describing #3163 fix and behavior change rationale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (sharedTelemetryClient == null) | ||
| { | ||
| lock (this.lockObject) | ||
| lock (StaticLockObject) | ||
| { | ||
| if (this.telemetryClient == null) | ||
| // Double-check pattern: another thread may have assigned | ||
| // sharedTelemetryClient between the outer null check and acquiring | ||
| // the lock. The analyzer doesn't model that across thread boundaries. | ||
| #pragma warning disable CA1508 // Avoid dead conditional code | ||
| if (sharedTelemetryClient == null) | ||
| #pragma warning restore CA1508 | ||
| { | ||
| this.telemetryClient = new TelemetryClient(this.telemetryConfiguration); | ||
| sharedTelemetryClient = new TelemetryClient(this.telemetryConfiguration); |
| if (overwrite) | ||
| { | ||
| properties[SemanticConventions.AttributeMicrosoftOperationName] = context.Operation.Name; | ||
| // Typed fields first; GlobalProperties applied last so they override typed values. | ||
| ApplyTypedFieldsToProperties(context, properties, overwrite: true); | ||
| MergeGlobalPropertiesToProperties(context, properties, overwrite: true); | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(context.Location?.Ip)) | ||
| else | ||
| { | ||
| properties[SemanticConventions.AttributeMicrosoftClientIp] = context.Location.Ip; | ||
| // Pre-existing entries win; among new keys GlobalProperties win over typed fields. | ||
| MergeGlobalPropertiesToProperties(context, properties, overwrite: false); | ||
| ApplyTypedFieldsToProperties(context, properties, overwrite: false); | ||
| } |
| if (overwrite) | ||
| { | ||
| activity.SetTag(SemanticConventions.AttributeEnduserId, context.User.AuthenticatedUserId); | ||
| // Typed fields first; GlobalProperties applied last so they override typed values. | ||
| ApplyTypedFieldsToActivity(context, activity, overwrite: true); | ||
| MergeGlobalPropertiesToActivity(context, activity, overwrite: true); | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(context.Operation?.Name)) | ||
| { | ||
| activity.SetTag(SemanticConventions.AttributeMicrosoftOperationName, context.Operation.Name); | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(context.Location?.Ip)) | ||
| else | ||
| { | ||
| activity.SetTag(SemanticConventions.AttributeMicrosoftClientIp, context.Location.Ip); | ||
| // Pre-existing tags win; among new keys GlobalProperties win over typed fields. | ||
| MergeGlobalPropertiesToActivity(context, activity, overwrite: false); | ||
| ApplyTypedFieldsToActivity(context, activity, overwrite: false); | ||
| } |
| private static IDictionary<string, string> BuildClientContextProperties(TelemetryContext context, IDictionary<string, string> properties) | ||
| { | ||
| var enriched = EnsureMutable(properties); | ||
| ApplyContextToProperties(context, enriched, overwrite: false); | ||
| return enriched; | ||
| } |
|
CreateDefault() doesn't crash anymore, however if one tries to create a telemetryclient from the telemetry config in global.asax, you get another crash that looks like this: Copilot suggesting below fix: |
Summary
Fixes #3163 — constructing a second
TelemetryClientagainst aTelemetryConfigurationthat has already been built (e.g. by the ASP.NET classicApplicationInsightsHttpModuleor by a priorTelemetryClientinstantiation) no longer throwsInvalidOperationException: "Configuration cannot be modified after it has been built."It also fixes the related correctness issue introduced in 3.1.0 (#3137) where the first
TelemetryClientsContextleaked to every subsequent client because it was registered globally as an OTel pipeline processor.Root cause
Prior to this PR, the non-DI
TelemetryClientconstructor calledconfiguration.PrependOpenTelemetryBuilderConfiguration(...)to registerTelemetryContextActivityProcessorandTelemetryContextLogProcessorevery time it ran. After the firstBuild()the configuration is sealed, so the second constructor call threw. Even when that issue was worked around, every subsequentTelemetryClientwould be silently shadowed by the first clientsContextbecause the processors were keyed to the first clientsTelemetryContextinstance.Approach (Path A — source-level per-call enrichment)
Move
TelemetryContextenrichment from a pipeline-wide processor to the call site of everyTrack*/StartOperation*method onTelemetryClient. This is the architecturally clean fix:TelemetryClientcarries its ownContext. At everyTrack*/StartOperation*invocation, the clientsContext(typed fields +GlobalProperties) is applied to the outgoing properties dict orActivitytags with skip-if-present semantics, so item-level values continue to take precedence.configuration.Build(). No pipeline mutation. Constructing N additionalTelemetryClientinstances against the same configuration is safe and cheap.TelemetryClientTests.UseApplicationInsightsTelemetryfor ASP.NET Core / Worker Service) is unchanged — it still registers pipeline-wideTelemetryContextActivityProcessor/TelemetryContextLogProcessorso OTel-native signals (ActivitySource,ILogger) emitted outside aTelemetryClient.Track*call continue to be enriched.Changes
BASE/src/Microsoft.ApplicationInsights/TelemetryClient.cs
PrependOpenTelemetryBuilderConfiguration; it just callsBuild()(idempotent, thread-safe).BuildClientContextProperties(TelemetryContext, IDictionary<string,string>)— returns a mutable, client-context-enriched properties dict (used byTrack*overloads that dont take an item-level telemetry object).EnrichActivityWithClientContext(TelemetryContext, Activity)— applies client context to an activity (used byStartOperation).ApplyContextToProperties/ApplyContextToActivityto take anoverwriteflag. Whenoverwrite=falsethey implement skip-if-present (preserving caller-set keys/tags).Track*methods and all 3StartOperationoverloads now thread the clientsContextthrough.BASE/src/Microsoft.ApplicationInsights/TelemetryClientExtensions.cs
StartOperationoverloads enrich the freshly-startedActivitywithtelemetryClient.ContextviaEnrichActivityWithClientContext.WEB/Src/Web/Web/ApplicationInsightsHttpModule.cs
static TelemetryClient sharedTelemetryClient(one host-blessed client per AppDomain).OnBeginRequestsoInit()no longer triggersBuild()(which can fail when no connection string is configured).Disposeis unchanged (the shared instance lives until the AppDomain unloads).Tests
BASE/Test/.../TelemetryClientMultiInstanceTests.cs(6 tests):SecondTelemetryClient_AgainstBuiltConfiguration_DoesNotThrow— direct InvalidOperationException from Microsoft.ApplicationInsights 3.1.0 Packages in ASP.Net 4.8 #3163 regression.ParallelTelemetryClientConstruction_DoesNotThrow— 32 parallel ctors.EachTelemetryClient_HasIsolatedContext— verifies per-instanceContextisolation.TrackTrace_FromDifferentClients_CarriesEachClientsContext— verifies log path enrichment is per-client.ItemLevelContext_TakesPrecedenceOverClientContext— verifies skip-if-present precedence.TrackDependency_FromDifferentClients_CarriesEachClientsContext— verifies activity path enrichment is per-client.WEB/Src/Web/Web.Tests/ApplicationInsightsHttpModuleTests.cs(2):Init_AllowsAdditionalTelemetryClientsToBeConstructedAfterInit— InvalidOperationException from Microsoft.ApplicationInsights 3.1.0 Packages in ASP.Net 4.8 #3163 regression in the HttpModule scenario.Init_AdditionalTelemetryClients_HaveIsolatedContext— per-clientContextisolation in the same scenario.Test results
Microsoft.ApplicationInsights.Tests, net8.0): 225/225 pass — including the 6 new tests.Diff stat
Backward compatibility
.publicApi/PublicAPI.Unshipped.txt).Contextis correctly scoped to that client. The only observable difference for non-DI users is that rawActivitySource.StartActivity(...)/ILogger.Log(...)calls (not routed throughTelemetryClient) no longer get auto-enriched with the firstTelemetryClientsContext. This was an undocumented side effect of the 3.1.0 design; restoring v3.0.0 behavior is arguably correct.CHANGELOG
Added an
Unreleasedentry. The placeholder/pull/0000URL will be replaced with the real PR number after this PR is opened.Closes #3163.