issues/1794: fix to work with application insights sdk v3 - #1798
issues/1794: fix to work with application insights sdk v3#1798lockerbill wants to merge 3 commits into
Conversation
System.MissingMethodException: Method not found: 'Void Microsoft.ApplicationInsights.Extensibility.TelemetryConfiguration.set_InstrumentationKey(System.String)'.
|
@lockerbill thanks for opening up the PR we will try to look into it ASAP. |
There was a problem hiding this comment.
Pull request overview
This PR addresses compatibility with Microsoft.ApplicationInsights SDK v3 by removing usage of the removed TelemetryConfiguration.InstrumentationKey setter and switching telemetry setup to use a connection string, plus updating tests to cover the new behavior.
Changes:
- Update
TelemetryClientFactoryto configure Application Insights viaTelemetryConfiguration.ConnectionString. - Add a unit test ensuring the factory sets the connection string as expected.
- Update test projects to reference
Microsoft.ApplicationInsightsv3.1.2 and remove obsolete instrumentation-key setters in test helpers.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sdk/PnP.Core/Services/Core/TelemetryClientFactory.cs | Switches telemetry configuration to use connection string instead of obsolete instrumentation key setter. |
| src/sdk/PnP.Core.Test/PnP.Core.Test.csproj | Adds an explicit App Insights v3 reference to reproduce/guard the compatibility scenario in tests. |
| src/sdk/PnP.Core.Test/Base/TelemetryTests.cs | Adds a test validating the telemetry factory configures ConnectionString. |
| src/sdk/PnP.Core.Test.Common/Services/Core/TestTelemetryManager.cs | Removes obsolete instrumentation-key assignment in the test telemetry manager. |
| src/sdk/PnP.Core.Test.Common/Services/Core/TestPnPContextFactory.cs | Removes obsolete instrumentation-key assignment previously used to redirect telemetry during local test runs. |
| .gitignore | Ignores IDE metadata under src/sdk/.idea. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (telemetryConfiguration == null) | ||
| { | ||
| telemetryConfiguration = TelemetryConfiguration.CreateDefault(); | ||
| #pragma warning disable CS0618 // Type or member is obsolete | ||
| telemetryConfiguration.InstrumentationKey = instrumentationKey; | ||
| #pragma warning restore CS0618 // Type or member is obsolete | ||
| telemetryConfiguration = new TelemetryConfiguration(); | ||
| telemetryConfiguration.ConnectionString = $"InstrumentationKey={instrumentationKey}"; | ||
| } |
There was a problem hiding this comment.
CreateDefault() changed semantics between majors. On AI 2.x it returns a new instance per call; on v3 it returns a process-wide
shared singleton (verified: ReferenceEquals(CreateDefault(), CreateDefault()) == true on 3.1.2, false on 2.23.0).
So on v3, telemetryConfiguration.ConnectionString = ... on a CreateDefault() instance would:
- overwrite the hosting application's shared telemetry configuration with the PnP instrumentation key, redirecting the host's
own telemetry into the PnP App Insights resource, and - throw InvalidOperationException: "Configuration cannot be modified after it has been built" as soon as anything else in the
process has already built a TelemetryClient from it. This is not hypothetical — with CreateDefault() in place,
SetupTests.ServicesWithOptions (which builds a factory with DisableTelemetry = false) freezes the shared config and
TelemetryClientFactoryConfiguresConnectionString then fails.
As for the concern itself: new TelemetryConfiguration() does not leave the pipeline unusable. On 2.23.0 it gets an
InMemoryChannel + PassThroughProcessor and TrackEvent works; on 3.1.2 the channel/processor concept doesn't exist at all (v3 is a
shim over Azure.Monitor.OpenTelemetry.Exporter) and ctor vs CreateDefault() produce identical state. The only real delta is that
v2's CreateDefault() adds OperationCorrelationTelemetryInitializer — not worth hijacking the host's config for.
A library needs its own isolated configuration, so I've kept new TelemetryConfiguration() and added a comment explaining why, to
stop this being "fixed" back.
| EventHub eventHub) : base(logger, sharePointRestClient, microsoftGraphClient, contextOptions, globalOptions, eventHub) | ||
| { | ||
| if (TelemetryManager != null && !TestCommonBase.RunningInGitHubWorkflow()) | ||
| { | ||
| // Send telemetry to the test Azure AppInsights instance | ||
| #pragma warning disable CS0618 // Type or member is obsolete | ||
| TelemetryManager.TelemetryClient.InstrumentationKey = "6073339d-9e70-4004-9ff7-1345316ade97"; | ||
| #pragma warning restore CS0618 // Type or member is obsolete | ||
| } | ||
| } |
There was a problem hiding this comment.
Right intent, but it can't be done at that location — I've restored it elsewhere.
Two blockers on v3: TelemetryClient.InstrumentationKey (and TelemetryContext.InstrumentationKey) are removed, so there's no
per-client override any more; and TelemetryConfiguration is frozen once a TelemetryClient is built from it. By the time the
TestPnPContextFactory ctor body runs, the base PnPContextFactory ctor has already called ConnectTelemetry(), which built both the
config and the client — so assigning ConnectionString there would throw.
The key therefore has to be set before the first TelemetryManager is constructed. TelemetryManager.InstrumentationKey is already
an internal static that the ctor feeds to TelemetryClientFactory, so I've added TestCommonBase.UseTestTelemetryInstance()
(guarded by RunningInGitHubWorkflow(), same as before) and call it from an [AssemblyInitialize] in each test assembly, which
MSTest guarantees runs before any test. Local runs go to 6073339d-… again; CI is unaffected (and has DisableTelemetry: true
anyway). Covered by a new TestRunsUseTheTestAppInsightsInstance test.
I did not restore the TestTelemetryManager line: that class overrides both LogInitRequest and LogServiceRequest and never calls
TrackEvent, so it never sent anything to App Insights regardless of key — it was dead code.
| FieldInfo configurationField = typeof(TelemetryClientFactory).GetField("telemetryConfiguration", BindingFlags.Static | BindingFlags.NonPublic); | ||
| FieldInfo clientField = typeof(TelemetryClientFactory).GetField("telemetryClient", BindingFlags.Static | BindingFlags.NonPublic); | ||
| TelemetryConfiguration originalConfiguration = (TelemetryConfiguration)configurationField.GetValue(null); | ||
| TelemetryClient originalClient = (TelemetryClient)clientField.GetValue(null); |
@Adam-it |
Hi @Adam-it , |
Fix issue #1794
System.MissingMethodException: Method not found: 'Void Microsoft.ApplicationInsights.Extensibility.TelemetryConfiguration.set_InstrumentationKey(System.String)'.