Repository navigation
Emit only canonical OpenTelemetry output in MTP 3 - #12316
Amaury Levé (Evangelink) wants to merge 3 commits into
Conversation
Remove duplicate legacy output and shared control state, retain canonical values and lifecycle behavior, and document the exact telemetry migration. Add focused attribute, metric, duration, and packaged-consumer assertions. Release validation and counterfactual execution remain pending the coordinated build slot. Relates to #12314. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🧪 Test quality review — PR #12316
Compared merge base c46cfa7394e7be10c4057d3ba8d1d3b698fe242e with head 006fa932f29de2841189f3e8c9a462409d8667e1. Evidence applies to this head only. This was a bounded static review; no code was executed.
Test-value evidence
| Contract | Test / decisive assertion | Counterfactual | Evidence status |
|---|---|---|---|
| Only canonical OTel attributes are emitted, and the legacy env switch is no longer read | OpenTelemetryResultHandlerTests.HandleTestResult_EmitsOnlyCanonicalAttributes, rows null, 0 and 1. It asserts exact tag-set equality, the duration histogram, the counter tags, and GetEnvironmentVariable never called |
Re-emitting any legacy tag (test.result, test.duration.ms, ...) adds a key and breaks the equality assertion |
Static prediction: likely detected. Not executed. |
| Legacy aliases removed from stdout, stderr and explanation | Existing truncation test asserts test.stdout, test.stderr and test.result.explanation are Times.Never |
Restoring the legacy twin tags fails the Times.Never verifications |
Static prediction: likely detected. |
| Removal of the total-completed and total-duration legacy metrics | No dedicated assertion found in the changed ranges | A surviving legacy metric would likely go unnoticed | Unverified. Possible gap, not a confirmed defect. |
| Acceptance, adapter, invoker and broker test edits | Small line-level edits in OpenTelemetryTests, MSTestTestNodeConverterTests, TestHostTestFrameworkInvokerTests, TestExecutionActivityBrokerTests and TestExecutionTests |
Not individually traced | Unassessed within budget. |
| Docs and changelog | None | None | Not applicable. |
No paired red/green execution records were accessible, so there is no executed evidence.
🧵 Parallel safety
STATUS: PARTIAL. The parallel-safety specialist was not run in this session, so coverage is incomplete and must not be read as clean. The new test builds its own mocks and a local environment mock, and reads no process-wide environment, so no new shared state was spotted in the graded test.
Per-test assessment
Pseudo-mutation evidence below consists of static predictions, not executed mutation results.
| Result | Quality | Test | Notes | How to improve |
|---|---|---|---|---|
| Pass | A (90–100) | new OpenTelemetryResultHandlerTests.HandleTestResult_EmitsOnlyCanonicalAttributes |
Exact tag-set equality plus histogram, counter and disposal checks cover the canonical contract. | None |
| Uncertain | — | mod OpenTelemetryTests (acceptance, lines 366-506 and others) |
Large range was not individually graded within budget. | Grade the changed methods individually. |
| Uncertain | — | mod MSTestTestNodeConverterTests, TestHostTestFrameworkInvokerTests, TestExecutionActivityBrokerTests and TestExecutionTests |
One-line edits that were not graded. | Grade the changed methods individually. |
No inline findings were published.
<sub>This advisory review is heuristic and non-blocking.
Re-run the combined review with /review-tests, or only the parallel-safety specialist with /parallel-audit.</sub>
🤖 Automated content by GitHub Copilot. Generated by the Test Quality Review on PR (on open / sync) workflow. · copilot · auto · ~28 AIC · ⌖ ~1 AIC · ⊞ 8.1K
There was a problem hiding this comment.
🟡 Changes recommended
The generated acceptance asset references nested snapshot types without qualification, causing CS0246 compilation errors and preventing both telemetry scenarios from running.
1 open issue
What changed in this PR
This PR makes Microsoft.Testing.Platform 3 emit only canonical OpenTelemetry output, addressing the telemetry portion of #12314 without broadly retiring older extension compatibility.
Changes:
- Removes legacy attributes, instruments, the retired environment switch, and its process-global state.
- Updates unit and package-consuming tests to check canonical names, result associations, duration units, and retained capture controls.
- Documents migration requirements and records intentional internal API removals.
Test-value evidence: For implementation 006fa932f29de2841189f3e8c9a462409d8667e1, final focused tests, packaging, and counterfactual execution remain pending. The reported 84-test pass predates the changes; no builds or tests were run during this review.
| File | Description |
|---|---|
test/UnitTests/MSTestAdapter.UnitTests/MSTestTestNodeConverterTests.cs |
Updates broker construction in adapter tests. |
test/UnitTests/Microsoft.Testing.Platform.UnitTests/TestFramework/TestExecutionTests.cs |
Updates the execution-test broker helper. |
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Telemetry/TestExecutionActivityBrokerTests.cs |
Removes obsolete broker options. |
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Telemetry/OpenTelemetryResultHandlerTests.cs |
Checks canonical-only output, units, and capture controls. |
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Requests/TestHostTestFrameworkInvokerTests.cs |
Updates broker construction in lifecycle tests. |
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/OpenTelemetryTests.cs |
Adds exported-value, identity, metric, and legacy-setting checks. |
src/Platform/Microsoft.Testing.Platform/Telemetry/TestingPlatformSemanticConventions.cs |
Removes legacy names and status conversion. |
src/Platform/Microsoft.Testing.Platform/Telemetry/TestExecutionActivityBroker.cs |
Removes unused options from activity initialization. |
src/Platform/Microsoft.Testing.Platform/Telemetry/PlatformOpenTelemetryOptions.cs |
Removes the legacy-emission option. |
src/Platform/Microsoft.Testing.Platform/Telemetry/OpenTelemetryResultHandler.Tags.cs |
Emits only canonical initial tags. |
src/Platform/Microsoft.Testing.Platform/Telemetry/OpenTelemetryResultHandler.ResultHandling.cs |
Removes duplicate result attributes and duration recording. |
src/Platform/Microsoft.Testing.Platform/Telemetry/OpenTelemetryResultHandler.Notifications.cs |
Removes legacy counter notifications. |
src/Platform/Microsoft.Testing.Platform/Telemetry/OpenTelemetryResultHandler.cs |
Removes legacy instrument creation and fields. |
src/Platform/Microsoft.Testing.Platform/Services/TestApplicationResult.cs |
Removes calls serving retired counters. |
src/Platform/Microsoft.Testing.Platform/Requests/TestHostTestFrameworkInvoker.cs |
Retains canonical session/request tags without legacy configuration. |
src/Platform/Microsoft.Testing.Platform/InternalAPI/InternalAPI.Unshipped.txt |
Records removed members and replacement signatures. |
src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs |
Removes global legacy-output initialization. |
src/Platform/Microsoft.Testing.Platform/Helpers/ExtensionHelper.cs |
Emits canonical extension identity without global state. |
src/Platform/Microsoft.Testing.Platform/Helpers/EnvironmentVariableConstants.cs |
Removes the retired environment-variable constant. |
src/Platform/Microsoft.Testing.Extensions.OpenTelemetry/PACKAGE.md |
Updates migration and configuration guidance. |
docs/decisions/canonical-telemetry.md |
Adds rationale, migration mapping, and validation plans. |
docs/Changelog-Platform.md |
Announces the observable telemetry breaking change. |
Review effort: Balanced
Retain the oldest-activity association check using test.case.result.status, and record the successful 109-case telemetry/lifecycle runs on .NET 10, .NET 11 and .NET Framework 4.6.2. The broader consumer selection will be rebuilt and rerun before completion. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
PR head changed after screening; this approval does not cover the current contribution.
There was a problem hiding this comment.
🧪 Test quality review — PR #12316
Compared merge base c46cfa7394e7be10c4057d3ba8d1d3b698fe242e with head 8d006ba6208f2d0b35a92d45824faa3d9286ec81. Evidence applies to this head, not every intermediate commit. This was a bounded static review; no code was executed.
Test-value evidence
| Contract | Test / decisive assertion | Counterfactual | Evidence status |
|---|---|---|---|
| Only canonical OTel span attributes and metric tags are emitted; the retired legacy env switch is never read | OpenTelemetryResultHandlerTests.HandleTestResult_EmitsOnlyCanonicalAttributes (rows null, 0, 1). It asserts exact tag-set equality, the 2.5 s histogram sample, the counter tags, and GetEnvironmentVariable is never called |
Re-emitting any legacy tag (test.result, test.duration.ms, test.stdout, ...) adds a key and breaks the equality assertion |
Static prediction: likely detected. Not executed. |
| Legacy aliases removed from output and explanation | Truncation test asserts test.stdout, test.stderr and test.result.explanation are Times.Never |
Restoring the legacy twins fails the Times.Never verifications |
Static prediction: likely detected. |
| Packaged canonical-only activities and metrics (units, suite/status points, exception ownership) | OpenTelemetryTests VerifyCanonicalTelemetry throws on any legacy tag, unexpected instrument set, wrong unit, or misassociated result |
Re-adding a legacy tag or instrument on the platform source would throw | Static prediction: likely detected. The PR states the packaged run is still pending. |
Removal of the tests.* counters (discovered, started, unknown) |
ExpectedInstruments in the acceptance check and the constructor instrument-contract test |
A surviving legacy instrument adds a key to the exported set | Static prediction: likely detected. |
| Constructor signature changes (broker, handler) | Edits to MSTestTestNodeConverterTests, TestHostTestFrameworkInvokerTests, TestExecutionActivityBrokerTests, TestExecutionTests |
Behavior-preserving; these are expected to pass before and after | Not applicable beyond compilation. |
Docs, changelog, InternalAPI.Unshipped.txt |
None | None | Not applicable. |
No paired red/green execution records were accessible. The PR description says the counterfactual runs are "Not executed yet", so there is no executed evidence. The PR's 327-pass unit run covers the previous source revision. The changes at this head are a service-test assertion and the evidence document.
🧵 Parallel safety
STATUS: PARTIAL. The parallel-safety specialist was not run in this session, so coverage is incomplete and must not be read as clean. The changed unit tests build local mocks and a local environment mock and read no process-wide state. The acceptance edits are in OpenTelemetryTests, which generates assets per test. Shared-asset and assembly-level concerns were not assessed. Run /parallel-audit for that.
Per-test assessment
Pseudo-mutation evidence below consists of static predictions, not executed mutation results.
| Result | Quality | Test | Notes | How to improve |
|---|---|---|---|---|
| Pass | A (90–100) | new OpenTelemetryResultHandlerTests.HandleTestResult_EmitsOnlyCanonicalAttributes |
Exact tag-set equality plus histogram, counter and disposal checks cover the canonical contract. | None |
| Uncertain | — | mod OpenTelemetryResultHandlerTests (existing cases, lines 34-61 and others) |
Edits are small and were not individually graded. | Grade each changed method individually. |
| Uncertain | — | mod OpenTelemetryTests (acceptance, lines 366-506 and others) |
Large range was not individually graded; the body-level checks look sound statically. | Grade the changed methods individually. |
| Uncertain | — | mod MSTestTestNodeConverterTests, TestHostTestFrameworkInvokerTests, TestApplicationResultTests, TestExecutionActivityBrokerTests and TestExecutionTests |
One-line constructor/call-site edits that were not graded. | Grade the changed methods individually. |
An existing review thread (Copilot code review) reports that the generated acceptance asset references nested snapshot types without qualification (CS0246). It was not re-verified here and is not duplicated inline. No new inline findings were published.
<sub>This advisory review is heuristic and non-blocking.
Re-run the combined review with /review-tests, or only the parallel-safety specialist with /parallel-audit.</sub>
🤖 Automated content by GitHub Copilot. Generated by the Test Quality Review on PR (on open / sync) workflow. · copilot · auto · ~38 AIC · ⌖ ~1 AIC · ⊞ 8.1K
There was a problem hiding this comment.
🔵 Needs a closer look
Shipping-consumer validation remains incomplete, with a reported generated-asset compile blocker and no inspected passing evidence for packed fallback/native consumers, the corrected wider-service rerun, or counterfactual execution.
Review effort: Balanced
Correct the generated snapshot model scope and preserve the same assertion source/binary across green, baseline-core and wrong-unit package variants. Record shipping provenance, all unit/provider/native results and executed red/green sensitivity without claiming external dashboard readiness or final-head CI. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Pull request
Summary
MTP 3 emits only canonical OpenTelemetry output. Remove old test-attribute aliases,
Extension.*,SessionUid/RequestTypetags,tests.*instruments, and the retiredTESTINGPLATFORM_OTEL_EMIT_LEGACY_ATTRIBUTEScontrol and its process-global state.Relates to #12314; this does not close the multi-group tracking Task.
This is an intentional observable telemetry break, not a blanket retirement of 2.x extension compatibility. Preserve canonical values, result associations, exception events, retry/activity/link topology, capture/truncation controls, cancellation/fault observation, and supported frameworks. Shipped internal API history remains intact; intentional removals are recorded in the unshipped baseline.
Migration highlights:
test.result=passed/failedbecomestest.case.result.status=pass/fail; divide oldtests.durationmillisecond samples by 1000 fortest.case.durationseconds. Spantest.case.duration_ms, timeout, and step durations remain milliseconds. Discovery/start/unknown-notification counters, source end lines, and extension descriptions have no exact canonical replacement. The linked map covers every name/value/unit and aggregation difference.Design and decision record
Maintained decisions, exact migration map, commands and evidence.
Keeping duplicate output or merely disabling it by default would retain the obsolete mode and cross-host global flag. Remove emission and unused plumbing instead, without changing canonical mapping/lifecycle. Application-owned
test.name/test.idattributes remain legal: absence checks filter to the platform's activity source.Demonstration
Nonvisual telemetry: the consumed surface is real OpenTelemetry SDK export batches. Six package-consuming cases verified two distinct identities, their associated pass/fail results, exception ownership, exact suite/status/count dimensions, 1.25 s / 0.75 s samples, millisecond span values, and no platform-produced legacy output. The same selection passed again after both isolated counterfactuals.
Each child intentionally contains one passing and one failing test and exits 2; passing outer tests require that exit and validate the captured exports. No external exporter endpoint, dashboard, live telemetry, or credential publication was used. External dashboard query/rendering migration remains a downstream rollout prerequisite, not a locally validated claim.
Validation and regression proof
Base:
c46cfa7394e7be10c4057d3ba8d1d3b698fe242e. Packed production source:8d006ba6208f2d0b35a92d45824faa3d9286ec81. Final head:10d19fa632dcb209a5fb693c1070abb77fbcf980; production files are byte-identical to that packed revision. Final changes correct generated test-model scope and maintain evidence. Pinned SDK:12.0.100-alpha.1.26556.115.Completed local results: shipping source pack and all content/metadata guards passed; 504 platform/result-consumer unit executions, 380 real-SDK provider/listener/wrapper executions, 6 shipping exporter cases, and 2 native MSTest packaged cases passed with no skips. Final exporter replay also passed 6/6. All adapter test-consumer TFMs compiled. Independent structured discovery, fresh TRX execution gates, and applicable JUnit/CTRF parsing verified execution.
Decisive commands, run from their worktree roots:
$testHostPathis the appropriate root'sartifacts\bin\Microsoft.Testing.Platform.Acceptance.IntegrationTests\Release\net12.0\Microsoft.Testing.Platform.Acceptance.IntegrationTests.exe. Generated external consumers use net11.0. Report/result mappings, exact unit/provider filters, owner builds, identities, source states, and retained setup failures are documented in the linked decision record.Constructor_CreatesOnlyCanonicalInstrumentsWithUnits,ExtensionTags_ContainOnlyCanonicalIdentity,HandleTestResult_EmitsOnlyCanonicalAttributespassed on net10/net11/net462; both shipping exporter methods passed unset/0/1 rows.HostApplicationBuilderProviders_ConsumeMtpDiagnosticsAndPreserveApplicationResourceandExternalFramework_PublicCanonicalExecutionApi_ProducesOneIsolatedSpanPerExecution: 6/6 passed and final replay 6/6 passed. Counts, suite/status points, durations, exception ownership and absence assertions execute inside the real SDK exporters.CompleteScope_DrainAndDisposeFailuresPreservePrimaryAndSecondaryExceptions, old-service fallback, retry association, output capture/truncation and the corrected oldest-activity consumer assertion.MSTestSdk_ApplicationOwnedOpenTelemetry_UsesOneCanonicalTestActivity: 2/2 .NET 10/11 rows passed. The fixture also builds its source-generation variant; only actual reflection executions are credited.Same-version packages used separate marked green/baseline/wrong-unit caches. Effective NuGet feeds/primary cache, resolved package/runtime assets, cached nupkg/DLL bytes, and all six children in each state were verified against that state's current package. The extra Visual Studio fallback folder was recorded, not modified. Green core nupkg SHA-256:
6B69635C0AA3D4AE96C5B5CE99CA45CBE94DD4E7B662CAFC15C7B126D03B3EF3; loaded net11 core:4712FA34954F619D68FEE700C351C74C470E45CA9C9047223EEF5C7CAAC84905. Native net10/net11 DLLs also match the shipping package.Gaps: final-head full CI is not inferred from local results; external dashboard readiness/rendering is unverified and unauthorized. Individual names were not each subjected to a separate mutation campaign; green asserts the complete output shape and baseline detects the legacy cohort at its first forbidden attribute. No task-owned build process remains; the mutation and original comparison package were restored. Named comparison/cache/result evidence is retained for reproducibility; shared/global caches were untouched.
Test-value evidence
SessionUidatCreateTestFrameworkSession(both modes, unset/1), 2 legacy-0 cases passed; outer exit 2.EF41412996AB094FD6217E9E2197257808157D35B25441DF1B5992F5E56E879D, loaded DLLDDEA8DBCF2983F6A07BCBA558D45A4D4E80B079A3D334F9722E1DFF69998BB39. This is an affected-core counterfactual, not a whole-repository baseline pack.TotalSecondstoTotalMillisecondsmutation.count=1, duration=1250, samples=1instead of 1.25 s. Other cases detected baseline legacy output.6291EAD4FBE0C602F78DD4B14F428EAFB79257FA9B4C38B4CE9AAD686E4E0130; mutated nupkg35260E72983A6794A395F324D024D6A5C6AC3A3F69B13E6A32C032E5A393BDD0; loaded DLL86C692BEE861EBA27A65B166DF7712568B7D242894ADC6423899AD3AD048FEDD.Fixed retained test patch SHA-256:
4FEB587EAE7E232157C4B9A1139AF5181806252806873C899B353BB066155DF1. Identical outer assertion DLL SHA-256:5A4937AEC817021541C9BDFE3E8353B0BFAF7026A2AEFE4D1597392B89022A69. Setup/compiler failures and zero/skipped tests were not counted as red proof.