Bump Microsoft.ApplicationInsights to 3.1.2 and add local telemetry export - #9725
Bump Microsoft.ApplicationInsights to 3.1.2 and add local telemetry export#9725Evangelink wants to merge 6 commits into
Conversation
…xport Microsoft.ApplicationInsights 3.x is an OpenTelemetry shim whose TrackEvent overload no longer accepts a metrics dictionary. AppInsightTelemetryClient now folds numeric measurements into the event's properties (invariant-culture strings) so telemetry stays a single correlated, context-enriched customEvent and avoids the OpenTelemetry metric-name-syntax restriction. The obsolete explicit System.Diagnostics.DiagnosticSource pin is removed (a modern version now flows transitively via Azure.Monitor.OpenTelemetry.Exporter -> Azure.Core). Adds a network-free local exporter: setting TESTINGPLATFORM_TELEMETRY_LOCALEXPORTPATH writes collected events to a file as JSON Lines via LocalFileTelemetryClient, selected through the existing ITelemetryClientFactory seam, so telemetry can be verified locally after the bump. Fixes part of #7465 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…t change Numeric measurements now land in customDimensions (folded into event properties) rather than customMeasurements. Bumping the schema version stamped on every event lets telemetry dashboards branch old vs new query shapes on the boundary. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR upgrades Microsoft.ApplicationInsights from 2.23.0 to 3.1.2 in the Microsoft.Testing.Extensions.Telemetry MTP extension and adapts the code to the 3.x (OpenTelemetry-shim) API. Since 3.x removed the metrics parameter from TrackEvent, numeric measurements are now folded into the event's properties (i.e. customDimensions) as invariant-culture strings. It also introduces a network-free LocalFileTelemetryClient (selected via TESTINGPLATFORM_TELEMETRY_LOCALEXPORTPATH) that writes events to disk as JSON Lines for local verification, and removes the now-obsolete explicit System.Diagnostics.DiagnosticSource 6.0.0 pin.
Changes:
- Bump
Microsoft.ApplicationInsightsto3.1.2and drop the explicitSystem.Diagnostics.DiagnosticSourcepin (now flows transitively). - Adapt
AppInsightTelemetryClient.TrackEventto fold numeric metrics into event properties for the 3.x 2-arg overload. - Add opt-in
LocalFileTelemetryClient+ factory wiring via a new env var, with unit tests andPACKAGE.mddocs.
Show a summary per file
| File | Description |
|---|---|
Directory.Packages.props |
Bumps AppInsights to 3.1.2 and removes the DiagnosticSource pin/comment. |
.../Microsoft.Testing.Extensions.Telemetry.csproj |
Removes the explicit System.Diagnostics.DiagnosticSource package reference. |
.../AppInsightTelemetryClient.cs |
Folds numeric metrics into properties to fit the 3.x TrackEvent API. |
.../AppInsightTelemetryClientFactory.cs |
Adds optional local-export path; returns LocalFileTelemetryClient when set. |
.../AppInsightsProvider.cs |
Adds the TESTINGPLATFORM_TELEMETRY_LOCALEXPORTPATH env-var constant. |
.../AppInsightsTelemetryProviderExtensions.cs |
Reads the local-export env var and passes it to the factory. |
.../LocalFileTelemetryClient.cs |
New JSON-Lines local telemetry client with manual JSON escaping. |
.../PACKAGE.md |
Documents the new local-export verification option. |
.../LocalFileTelemetryClientTests.cs |
New unit tests for the local client and factory selection. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Medium
- Bump System.Threading.Tasks.Extensions 4.5.4 -> 4.6.3. Microsoft.ApplicationInsights 3.1.2 -> Azure.Monitor.OpenTelemetry.Exporter -> Azure.Core 1.54.0 requires >= 4.6.3, which made every .NETFramework project referencing MSTest.TestAdapter fail restore with NU1605 (Windows-only, because the Linux/macOS legs skip the net462 targets). - Declare the new/changed internal APIs (LocalFileTelemetryClient, AppInsightTelemetryClientFactory ctor, AppInsightsProvider.LocalExportPathEnvVar, TelemetryProperties.VersionValue = "21") in the InternalAPI.Unshipped.txt baselines introduced by #9752, which landed on main after this PR was opened. - Add the required UTF-8 BOM to LocalFileTelemetryClient.cs and LocalFileTelemetryClientTests.cs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e947c36-e52a-4ba8-8185-cd338ae24060
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (5)
src/Platform/Microsoft.Testing.Platform/Telemetry/TelemetryProperties.cs:17
VersionValuecomes from Microsoft.Testing.Platform, not from the telemetry exporter that determines where metrics land. The package dependency range permits an older 2.x telemetry extension to run with this newer Platform version, so such runs would report version 21 while still writingcustomMeasurements; the dashboard branch described by this PR would then read the wrong column. Tie the cutover marker to the telemetry client/extension that performs the conversion, or otherwise enforce package pairing so version 21 reliably identifies the new layout.
public const string VersionValue = "21";
src/Platform/Microsoft.Testing.Extensions.Telemetry/LocalFileTelemetryClient.cs:59
- The local format keeps measurements in a separate numeric
metricsobject, whereas the live 3.x path folds them into string-valued properties (AppInsightTelemetryClient.cs:43-49). Consequently this exporter cannot verify the schema-21/customDimensions representation advertised in PACKAGE.md and the PR description. Reuse one normalization step for both clients, then update the JSON contract and tests to reflect the actual exported shape.
builder.Append("},\"metrics\":{");
first = true;
foreach (KeyValuePair<string, double> metric in metrics)
src/Platform/Microsoft.Testing.Extensions.Telemetry/LocalFileTelemetryClient.cs:67
- This emits
NaN,Infinity, or-Infinityas bare tokens when a telemetry producer supplies a non-finite double, which makes the promised JSON Lines record invalid JSON. The interface accepts arbitrary doubles, so serialize non-finite values using an explicitly valid representation (or reject them before writing) and cover that boundary case.
AppendJsonKey(builder, metric.Key);
builder.Append(metric.Value.ToString("R", CultureInfo.InvariantCulture));
src/Platform/Microsoft.Testing.Extensions.Telemetry/LocalFileTelemetryClient.cs:75
- The single-consumer guarantee is only per provider process. The export-path environment variable is inherited by spawned test-host processes, so several providers can call
AppendAllTexton the same file concurrently; on Windows the competing append opens can throwIOException, and the provider then drops those events. Use an inter-process-safe append strategy, or write per-process files and merge them.
// TrackEvent is only ever invoked from the provider's single-consumer ingest loop, so no
// synchronization is needed here (mirroring AppInsightTelemetryClient).
File.AppendAllText(_filePath, builder.ToString() + Environment.NewLine);
src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlf:625
- This unrelated change replaces an existing French translation with the English source and resets it to
state="new", so French analyzer users regress to English. Revert this XLF hunk; the owning RESX source is unchanged, and repository localization guidance requires XLF updates to be generated rather than edited independently.
<target state="new">Methods marked with '[GlobalTestInitialize]' or '[GlobalTestCleanup]' should follow the following layout to be valid:
-it can't be declared on a generic class
-it should be 'public'
-it should be 'static'
-it should not be 'async void'
-it should not be a special method (finalizer, operator...).
-it should not be generic
-it should take one parameter of type 'TestContext'
-return type should be 'void', 'Task' or 'ValueTask'
The type declaring these methods should also respect the following rules:
-The type should be a class
-The class should be 'public'
-The class should be marked with '[TestClass]' (or a derived attribute)
-the class should not be generic.</target>
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Medium
|
Pushed a fix for the red pipeline (build 1529395 is now fully green). The branch was also merged with Root cause of
Two follow-on issues surfaced once restore succeeded:
No product behavior was changed beyond the dependency version bump. |
…ements) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e947c36-e52a-4ba8-8185-cd338ae24060
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
src/Platform/Microsoft.Testing.Extensions.Telemetry/LocalFileTelemetryClient.cs:60
- This local output preserves measurements in a separate numeric
metricsobject, while the migratedAppInsightTelemetryClientputs those same values intopropertiesas strings. Consequently the documented local verification path does not expose the schema that will actually reachcustomDimensions, which is the central migration behavior this PR needs to verify. Serialize the post-migration shape here as well and update the test expectation.
builder.Append("},\"metrics\":{");
first = true;
foreach (KeyValuePair<string, double> metric in metrics)
{
src/Platform/Microsoft.Testing.Extensions.Telemetry/LocalFileTelemetryClient.cs:75
- The single-consumer guarantee only applies within one provider process. This environment-supplied path can be shared by concurrently running testhost processes, and simultaneous
File.AppendAllTextcalls can fail with sharing violations (the provider catches the exception and drops the event). Use cross-process-safe serialization or define per-process output files and a merge strategy.
// TrackEvent is only ever invoked from the provider's single-consumer ingest loop, so no
// synchronization is needed here (mirroring AppInsightTelemetryClient).
File.AppendAllText(_filePath, builder.ToString() + Environment.NewLine);
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Medium
| // Bump this whenever the telemetry schema changes so dashboards can branch old vs new. | ||
| // 21: Microsoft.ApplicationInsights 3.x (OpenTelemetry shim) removed TrackEvent's metrics | ||
| // parameter, so numeric measurements are now folded into event properties | ||
| // (customDimensions) instead of customMeasurements. See #7465. | ||
| public const string VersionValue = "21"; |
There was a problem hiding this comment.
I traced this and the analysis holds. TelemetryManager (platform) builds TelemetryInformation from TelemetryProperties.VersionValue (TelemetryManager.cs:63), and AppInsightsProvider (extension) is what stamps that value as the telemetry version dimension (AppInsightsProvider.cs:182). Since the two ship as separately-resolvable NuGets, a newer platform paired with the 2.x telemetry extension - the combination TelemetryForwardCompatibilityTests explicitly pins at 2.0.0 and regression-tests - would emit telemetry version = 21 while its AppInsightTelemetryClient still routes numeric values into customMeasurements. A dashboard branching on >= 21 would then read absent customDimensions keys and silently yield nulls.
I am deliberately not flipping the constant here. The bump to "21" was a deliberate commit (faf88bf) made in response to an earlier review thread asking for exactly this signal, so reverting it on review feedback alone would trade one wrong signal for no signal and re-open that thread. The convergent fix is to let the component whose serialization actually changed own the value, but each shape is a schema decision with downstream dashboard consequences:
- Revert the platform constant to
"20"and have the 3.x extension stamp its own"21"atAppInsightsProvider.cs:182. Keeps a single dimension, but a future genuine platform-schema bump would be clobbered by the extension's literal. - Revert the platform constant to
"20"and add a separate extension-owned dimension describing where numeric values live. Keepstelemetry versionmeaning "platform schema" and is unambiguous, at the cost of a new dimension for dashboards to read.
Either is a handful of lines, but picking between them changes the Kusto contract documented in the PR description, so I would rather not choose unsupervised. Leaving this unresolved for a maintainer call - happy to implement whichever you prefer.
Note
Auto-replied by the GitHub Copilot app.
|
Merged latest The diff against |
🧪 Test quality grade — PR #9725
This advisory comment was generated automatically. Grades are heuristic
|
What
Bumps
Microsoft.ApplicationInsightsfrom2.23.0to3.1.2and adapts our telemetry extension to the 3.x API. Also adds a network-free local telemetry exporter so collected telemetry can be verified after the migration.Fixes part of #7465.
Why
AppInsights 2.x was pinned because 3.x initially dropped
netstandard2.0(see ApplicationInsights-dotnet#3091). That restriction is gone — 3.1.2 shipsnetstandard2.0,net462,net8.0,net9.0,net10.0. 3.x is an OpenTelemetry shim (backed by Azure Monitor Exporter), which brings a modern, maintainedSystem.Diagnostics.DiagnosticSourcetransitively.Changes
Directory.Packages.props:Microsoft.ApplicationInsights2.23.0→3.1.2. Removed the explicitSystem.Diagnostics.DiagnosticSource6.0.0 pin — its purpose (override AI 2.x's unmaintained transitive 5.0.0) is obsolete; a modern version now flows transitively viaAzure.Monitor.OpenTelemetry.Exporter→Azure.Core. Keeping the pin caused anNU1605downgrade error.AppInsightTelemetryClient: 3.x removed themetricsparameter fromTrackEvent. We now fold numeric measurements into the event's properties as invariant-culture strings and call the 2-arg overload. This keeps a single correlated, context-enrichedcustomEventand sidesteps the new OpenTelemetry metric-name syntax restriction (property keys have no such restriction, so no keys need renaming).LocalFileTelemetryClient, selected viaTESTINGPLATFORM_TELEMETRY_LOCALEXPORTPATHthrough the existingITelemetryClientFactoryseam. When set, events are written to a file as JSON Lines instead of being shipped to AppInsights — the "local exporter" for verification, no network traffic. Documented inPACKAGE.md.20→21(see below).In AI 2.x, numeric measurements were sent in
customMeasurements. In 3.x they are folded intocustomDimensions(as strings). Dashboard queries that read numeric values must be updated, e.g.runStarttimings,total ran/total passedcounts,mstest.setting.*_timeout,parallelization_workers, etc.To make the cutover queryable, the platform telemetry schema version (
TelemetryProperties.VersionValue, emitted on every event as thetelemetry versiondimension) is bumped20→21. Dashboards can branch on it:Testing
netstandard2.0/net8.0/net9.0).Microsoft.Testing.Extensions.UnitTestspass onnet9.0(612) andnet472(611), including 4 newLocalFileTelemetryClient/factory tests.AppInsightTelemetryClientconstructs, tracks, and flushes without throwing (throwaway test, removed).Open questions for reviewers
netstandard2.0/net462, but the actual push now goes through the heavierAzure.Monitor.OpenTelemetry.Exporter→Azure.Corestack. Local export bypasses the network, so it does not validate real ingestion. A live smoke test against a (test) AI resource on those hosts is still needed before we're confident — this is the VS-2022 concern from Microsoft.Testing.Extensions.Telemetry blocked from upgrading to latest Microsoft.ApplicationInsights #7465.Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com