Harden OpenTelemetry release readiness: docs, tests, spec-compliant env parsing - #10865
Merged
Amaury Levé (Evangelink) merged 6 commits intoAug 31, 2026
Merged
Conversation
Address the remaining OpenTelemetry release-readiness work for the newly stable AddTestingPlatformResource and AddOpenTelemetryProviderFromEnvironment helpers. - Document exactly which OpenTelemetry fields can carry sensitive data (source/artifact file paths, stdout/stderr, exception message/stack trace) and how to control each, addressing issue #8411's documentation ask. - Extract the OTEL_* environment-variable decision of AddOpenTelemetryProviderFromEnvironment into a pure internal ResolveEnvironmentConfiguration seam so it is unit-testable without a live builder. No public API or behavior change. - Add direct tests for TestingPlatformResourceDetector (service name, host/OS/ runtime attributes, CI provider detection, credential stripping), for AddTestingPlatformResource, for the env-driven decision, and an observable end-to-end trace through the real OpenTelemetry SDK. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…D parsing Code-review follow-up on the OpenTelemetry release-hardening change. - PACKAGE.md: fix the "Data exported" table. Drop the incorrect claim that every string attribute is truncated (file, artifact and metadata values are emitted verbatim); correct the legacy metadata prefix to test.metadataProperty.*; list the legacy exception twins (test.result.exception.type/message/stacktrace) and the span status description; and note the resource attributes (host, OS, CI provenance) that ride on every span. - AddOpenTelemetryProviderFromEnvironment: make OTEL_SDK_DISABLED honor the OpenTelemetry boolean convention — only a case-insensitive "true" disables the SDK, so "1" and other spellings leave it enabled. CI-marker parsing in TestingPlatformResourceDetector is intentionally left untouched. - Tests: assert the spec-compliant OTEL_SDK_DISABLED behavior (case-insensitive "true" disables; "1"/"yes"/"false"/"" do not). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The sensitive-data section rewrite left PACKAGE.md without a final newline, tripping markdownlint MD047. Append exactly one trailing newline. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Expert Review — 22-Dimension Verdict
| # | Dimension | Verdict | Notes |
|---|---|---|---|
| 1 | Algorithmic Correctness | ✅ LGTM | IsTrue behavioral change (dropping "1") is intentional spec-compliance, well-documented in comments. ResolveEnvironmentConfiguration logic faithfully preserves existing decision tree. |
| 2 | Threading & Concurrency | ✅ LGTM | EnvironmentConfiguration is a readonly struct (immutable). End-to-end test exporter properly locks the shared _activities list. |
| 3 | Security & IPC Contract Safety | ✅ LGTM | No new file operations, deserialization, or command-line handling. The PACKAGE.md docs accurately describe credential stripping and sensitive-data controls. |
| 4 | Public API & Binary Compat | ✅ LGTM | No public API changes. New internal members (ResolveEnvironmentConfiguration, EnvironmentConfiguration) correctly declared in InternalAPI.Unshipped.txt. Struct uses get (not init). |
| 5 | Performance & Allocations | ✅ LGTM | The refactoring introduces one struct allocation per call (stack-allocated). No regressions. |
| 6 | Cross-TFM Compatibility | ✅ LGTM | StringComparison.OrdinalIgnoreCase and string.Trim() are available on all target TFMs (netstandard2.0+). |
| 7 | Resource & IDisposable Mgmt | ✅ LGTM | End-to-end test properly disposes TracerProvider, OpenTelemetryPlatformService, and OpenTelemetryResultHandler with using. |
| 8 | Defensive Coding | ✅ LGTM | Null guard on builder preserved. Func<string, string?> delegate cleanly abstracts env access. |
| 9 | Localization & Resources | N/A | No .resx or .xlf changes. |
| 10 | Test Isolation | ✅ LGTM | [DoNotParallelize] on both test classes. TestingPlatformResourceDetectorTests.WithEnvironment saves and restores every observed env var in a finally block. OpenTelemetryProviderExtensionsTests env-decision tests use a pure Func<string, string?> — no env mutation. |
| 11 | Assertion Quality | ✅ LGTM | Tests use MSTest assertions (matching BannedSymbols.txt which bans AwesomeAssertions). Assertions are specific (AreEqual, IsTrue, IsFalse, ContainsKey). |
| 12 | Flakiness Patterns | ✅ LGTM | No timing-dependent assertions. End-to-end test uses GUID-prefixed activity names to avoid ambient pollution. |
| 13 | Test Completeness | ✅ LGTM | Comprehensive coverage: SDK-disabled (case variations, non-true values), endpoint-only, exporter-only, none-override, comma-separated exporters, delegate-without-exporter, null-guard, CI providers (GitHub/Azure/GitLab/Jenkins), precedence, credential stripping, resource attributes, end-to-end trace. |
| 14 | Data-Driven Test Coverage | ✅ LGTM | [DataRow] used for OTEL_SDK_DISABLED case variations including edge cases (" true ", "1", "yes", "false", ""). |
| 15 | Code Structure | ✅ LGTM | Clean extraction into pure function + value type. No deep nesting. |
| 16 | Naming & Conventions | ✅ LGTM | Test method names are descriptive and follow Method_Condition_Outcome pattern. |
| 17 | Documentation Accuracy | ✅ LGTM | XML doc comments on all new internal members. PACKAGE.md table is accurate and detailed. Comment on IsTrue clearly explains the spec difference vs. CI-marker parsing. |
| 18 | Analyzer & Code Fix Quality | N/A | No analyzer changes. |
| 19 | IPC Wire Compatibility | N/A | No serialization/wire changes. |
| 20 | Build Infrastructure | N/A | No build/dependency changes. |
| 21 | Scope & PR Discipline | ✅ LGTM | Coherent scope: docs + testability refactor + spec fix + tests for the same feature area. References #8411. |
| 22 | PowerShell Scripting | N/A | No .ps1 changes. |
Findings
1 NIT — PACKAGE.md lost its trailing newline (SA1518 violation). See inline comment.
Overall this is a well-structured PR: the testability refactor is clean, the behavioral change (IsTrue spec compliance) is well-documented, the test coverage is thorough with proper isolation, and the sensitive-data documentation fills a genuine gap.
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 2
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 |
src/Platform/Microsoft.Testing.Extensions.OpenTelemetry/OpenTelemetryProviderExtensions.cs — Trim() makes " true " disable the SDK, but the OpenTelemetry Boolean specification allows only… |
|
src/Platform/Microsoft.Testing.Extensions.OpenTelemetry/OpenTelemetryProviderExtensions.cs — There is still no positive test of AddOpenTelemetryProviderFromEnvironment itself: the resolver… |
 |
src/Platform/Microsoft.Testing.Extensions.OpenTelemetry/PACKAGE.md — This sensitive-data inventory is incomplete. test.case.name, test.case.id/test.case.parent.id… |
What changed in this PR
Hardens the OpenTelemetry extension for release through documentation, test coverage, and environment parsing changes.
Changes:
- Documents potentially sensitive telemetry fields and controls.
- Extracts environment configuration resolution for testing.
- Adds resource detection and OpenTelemetry pipeline tests.
| File | Description |
|---|---|
PACKAGE.md |
Documents exported sensitive values. |
OpenTelemetryProviderExtensions.cs |
Refactors environment parsing and SDK-disable handling. |
InternalAPI.Unshipped.txt |
Tracks new internal APIs. |
OpenTelemetryProviderExtensionsTests.cs |
Tests configuration and tracing behavior. |
TestingPlatformResourceDetectorTests.cs |
Tests resource and CI attributes. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This comment has been minimized.
This comment has been minimized.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
🧪 Expert test review — PR #10865
This advisory comment was generated automatically. Grades are heuristic
|
Contributor
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 171.8 AIC · ⌖ 1.43 AIC · ⊞ 16.9K · ◷
Amaury Levé (Evangelink)
added
the
state/needs-review
Amaury Levé (Evangelink)
enabled auto-merge (squash)
August 31, 2026 07:24
Petr Pokorny (0101)
approved these changes
Aug 31, 2026
…uller docs Follow-up on review feedback for the OpenTelemetry release-hardening PR. - OTEL_SDK_DISABLED parsing no longer trims: only an exact case-insensitive "true" disables the SDK, per the OpenTelemetry boolean convention. A whitespace-padded " true " now correctly leaves the SDK enabled, and the test that asserted the old behavior was moved to the enabled-values test. - Add a positive test of the public AddOpenTelemetryProviderFromEnvironment that drives a real TestApplicationBuilder, triggers the registered factory via TelemetryManager.BuildOTelProvider, and asserts the provider is built and the caller delegate runs; plus a no-op assertion when the SDK is disabled. - PACKAGE.md: extend the exported-data inventory with test.case.name, test.case.id / test.case.parent.id (and legacy twins) and test.step.<id>.description, all exported verbatim. Build clean; 30/30 OpenTelemetry unit tests pass on net9.0 and net472; markdownlint reports 0 errors. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Contributor
🧪 Expert test review — PR #10865Scope: 2 new test files (25 test methods) covering
No new inline suggestions posted this run — the two B-grade gaps above are completeness nits (an extra assertion), not correctness bugs, and every previously-flagged actionable finding on this PR is already fixed in the current head. This advisory comment was generated automatically. Grades are heuristic and informational — they do not block merging. Re-run with
|
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
|
src/Platform/Microsoft.Testing.Extensions.OpenTelemetry/PACKAGE.md — This sensitive-data inventory is incomplete. test.case.name, test.case.id/test.case.parent.id… View resolved comment |
|
src/Platform/Microsoft.Testing.Extensions.OpenTelemetry/OpenTelemetryProviderExtensions.cs — There is still no positive test of AddOpenTelemetryProviderFromEnvironment itself: the resolver… View resolved comment |
|
src/Platform/Microsoft.Testing.Extensions.OpenTelemetry/OpenTelemetryProviderExtensions.cs — Trim() makes " true " disable the SDK, but the OpenTelemetry Boolean specification allows only… View resolved comment |
Suppressed comments (1)
src/Platform/Microsoft.Testing.Extensions.OpenTelemetry/OpenTelemetryProviderExtensions.cs:164
- The resolver tests do not execute this metrics callback, and the only public-path test passes a tracing delegate with both exporters set to
none. Consequently, replacingConfigureMetricsProviderorUseOtlpMetricshere with the tracing flags would leave the entire suite green. Add a metrics-only public-path test that verifies the platform meter/exporter is actually wired, not just that the resolver returns the expected booleans.
if (configuration.ConfigureMetricsProvider)
Two non-blocking test-review suggestions: - ResolveEnvironmentConfiguration_WithCommaSeparatedExporterList_StillEnablesOtlp now also asserts ConfigureTracingProvider, so a mutant flipping it independently of UseOtlpTracing is killed. - GetResourceAttributes_WhenGitHubAndAzureBothSet_PrefersGitHubActions now sets an Azure-only marker (BUILD_REPOSITORY_URI) and asserts vcs.repository.url.full is absent, proving the GitHub branch wins and the Azure branch never runs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/Platform/Microsoft.Testing.Extensions.OpenTelemetry/PACKAGE.md:75
- This row says the whole group is truncated, but
exception.type,error.type, and the legacy exception type are passed through verbatim; only the explanation, message, status description, and stack-trace values use_options.Truncate(OpenTelemetryResultHandler.ResultHandling.cs:49-89). Distinguish those behaviors so the sensitive-data guidance is accurate.
| `code.stacktrace`, `test.case.result.explanation` (`test.result.explanation`), the `exception` span event (`exception.type` / `exception.message` / `exception.stacktrace`), the span status description, and the legacy `test.result.exception.type` / `test.result.exception.message` / `test.result.exception.stacktrace` | Exception message and stack-trace text. | Truncated to `TESTINGPLATFORM_OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT`. Disable the legacy twins with `TESTINGPLATFORM_OTEL_EMIT_LEGACY_ATTRIBUTES=0`. |
src/Platform/Microsoft.Testing.Extensions.OpenTelemetry/OpenTelemetryProviderExtensions.cs:205
- The enabled metrics-delegate branch is not tested: every resolver test passes
hasMetricsDelegate: false, except disabled cases that return before this expression. RemovinghasMetricsDelegatehere would therefore go undetected, and the public-helper test likewise verifies only the tracing delegate. Add an enabled metrics-delegate case that expects metrics configuration without OTLP.
bool configureMetricsProvider = useOtlpMetrics || hasMetricsDelegate;
src/Platform/Microsoft.Testing.Extensions.OpenTelemetry/PACKAGE.md:77
- The PR promises an exact inventory, but this resource row names only a subset.
AddTestingPlatformResourcealso emitsservice.*,host.arch,os.type,process.*, andtest.assembly.name, while CI detection emits specific pipeline/task/repository keys. Enumerate these fields so users can assess all provenance exported by the stable helper.
| Resource `vcs.repository.url.full`, plus the other resource attributes (`host.name`, `os.description`, and in CI the `cicd.*` / `vcs.*` pipeline, branch and commit — see *Resource attributes* above) | The machine, OS and CI provenance attached to every span and metric point. | User-info credentials in the repository URL (`https://user:token@host/...`) are stripped before export. |
Per the parallel-safety audit: OpenTelemetryProviderExtensionsTests carried a class-level [DoNotParallelize], but only two methods touch process-global state - the one that mutates real environment variables and the end-to-end test that stands up a real TracerProvider on the shared platform ActivitySource. Move the attribute onto just those two methods so the nine pure ResolveEnvironmentConfiguration tests and the null-guard tests rejoin the parallel set. The env-reading AddTestingPlatformResource test can safely parallelize because every environment-variable *mutator* in this assembly (AddOpenTelemetryProviderFromEnvironment_..., AzureFoundryChatClientProviderTests, TestingPlatformResourceDetectorTests) is [DoNotParallelize] and runs in the serial tail, so it never overlaps the parallel reader. TestingPlatformResource- DetectorTests is intentionally kept on class-level [DoNotParallelize] rather than migrated to [ResourceLock(EnvironmentVariables)]: moving its mutations into the parallel set would require the now-parallel reader to take the same lock, a broader coupling not worth introducing here. Verified: build clean; full Microsoft.Testing.Extensions.UnitTests assembly green across three consecutive runs (1522 passed, 7 skipped); OTel tests pass on net9.0 and net472. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
🧵 Parallel-safety audit — PR #10865Parallelization — assembly audited:
Findings: A (global-state) This PR's only change is scoping
No unsafe call sites, near-misses, or coverage gaps found in the changed lines. This is a clean readiness/throughput improvement with no correctness risk introduced. Advisory only — heuristic, non-blocking. Re-run with
|
Contributor
🧪 Expert test review — PR #10865All 25 new test methods across the two new files (
This advisory comment was generated automatically. Grades are heuristic
|
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/Platform/Microsoft.Testing.Extensions.OpenTelemetry/PACKAGE.md:77
- This catch-all still does not disclose the exact resource fields exported, which is the documentation gap this PR closes.
AddServicealso emitsservice.name,service.version, and a generatedservice.instance.id, while the detector emitshost.arch,os.type,process.pid, runtime fields, andtest.assembly.name; these can expose application or execution-environment details but are absent here. Please enumerate the concrete resource attribute names and state that only URL user-info is stripped.
| Resource `vcs.repository.url.full`, plus the other resource attributes (`host.name`, `os.description`, and in CI the `cicd.*` / `vcs.*` pipeline, branch and commit — see *Resource attributes* above) | The machine, OS and CI provenance attached to every span and metric point. | User-info credentials in the repository URL (`https://user:token@host/...`) are stripped before export. |
test/UnitTests/Microsoft.Testing.Extensions.UnitTests/OpenTelemetryProviderExtensionsTests.cs:242
- The metrics-only branches remain untested. The endpoint test reaches metrics through the shared fallback, while every
hasMetricsDelegate: truecase returns early because the SDK is disabled; therefore regressions that ignoreOTEL_METRICS_EXPORTERorhasMetricsDelegatewould pass. Add symmetric metrics-exporter and metrics-delegate cases.
public void ResolveEnvironmentConfiguration_WithTracingDelegateButNoExporter_ConfiguresProviderWithoutOtlp()
{
EnvironmentConfiguration configuration = OpenTelemetryProviderExtensions.ResolveEnvironmentConfiguration(
Env([]),
hasTracingDelegate: true,
hasMetricsDelegate: false);
Assert.IsTrue(configuration.ConfigureTracingProvider);
Assert.IsFalse(configuration.UseOtlpTracing);
Assert.IsFalse(configuration.ConfigureMetricsProvider);
}
Amaury Levé (Evangelink)
deleted the
dev/amauryleve/otel-release-hardening
branch
August 31, 2026 10:00
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Release-hardening for the OpenTelemetry extension ahead of MSTest 4.4 / MTP 2.4, focused on the two turnkey helpers that were promoted from
[TPEXP]to stable API this cycle —AddTestingPlatformResourceandAddOpenTelemetryProviderFromEnvironment— plus the sensitive-data documentation gap raised in #8411.No breaking public API changes: the stable helpers stay stable, and the one behavioral change is a spec-compliance fix on how
OTEL_SDK_DISABLEDis parsed.Motivation
mainthe result pipeline already truncates large string attributes (TESTINGPLATFORM_OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT, 8 KB default), gates stdout/stderr behindTESTINGPLATFORM_OTEL_CAPTURE_TEST_OUTPUT, and strips credentials fromvcs.repository.url.full. The genuine remaining gap was accurate documentation of what is exported — nothing leaves the process without an exporter you configure, so documentation (not a default behavior change) is the right release posture.Changes
PACKAGE.md) — added a "Data exported and controlling sensitive values" table enumerating each attribute that can carry sensitive data (source/artifact file paths, stdout/stderr, exception message/stack trace,test.metadata.*, resource provenance) and the exact control for each. Truncation is described precisely: only captured output, result explanation and exception text are truncated; file/artifact/metadata values are exported verbatim.AddOpenTelemetryProviderFromEnvironment'sOTEL_*decision into a pure internalResolveEnvironmentConfigurationseam so it is unit-testable without a live builder. Behavior-preserving; new internal members declared inInternalAPI.Unshipped.txt.OTEL_SDK_DISABLEDspec compliance — the turnkey helper now honors the OpenTelemetry boolean convention: only a case-insensitive"true"disables the SDK, so"1"and other spellings leave it enabled. The CI-marker parsing inTestingPlatformResourceDetector(GitHub/Azure/GitLab/Jenkins) is intentionally left untouched.TestingPlatformResourceDetectorTests(service name, host/OS/arch/runtime attributes, CI provider detection + precedence, credential stripping) andOpenTelemetryProviderExtensionsTests(AddTestingPlatformResourceresource build, null guards, the full env-decision matrix, and an observable end-to-end trace through the real OpenTelemetry SDK asserting exported span tags + resource attributes).Testing
.\build.cmd -c Debug -projects Microsoft.Testing.Extensions.UnitTests.csproj— clean (0 warnings, 0 errors) across all TFMs.Closes #8411.
Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com