Harden OpenTelemetry release readiness: docs, tests, spec-compliant env parsing - #10865
Open
Amaury Levé (Evangelink) wants to merge 3 commits into
Open
Harden OpenTelemetry release readiness: docs, tests, spec-compliant env parsing#10865Amaury Levé (Evangelink) wants to merge 3 commits into
Amaury Levé (Evangelink) wants to merge 3 commits into
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>
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 | 安全 & 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. 新建 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.
| ## Feedback & contributing | ||
|
|
||
| Microsoft.Testing.Platform is an open source project. Provide feedback or report issues in the [microsoft/testfx](https://github.com/microsoft/testfx/issues) GitHub repository. | ||
| Microsoft.Testing.Platform is an open source project. Provide feedback or report issues in the [microsoft/testfx](https://github.com/microsoft/testfx/issues) GitHub repository. No newline at end of file |
Contributor
There was a problem hiding this comment.
[StyleCop SA1518 — NIT] The file no longer ends with a newline (\ No newline at end of file). SA1518 requires every file to end with exactly one newline character. Please add a trailing newline back.
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 2
新建议题s 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.
| // parsing in TestingPlatformResourceDetector, which recognises provider-specific spellings. | ||
| private static bool IsTrue(string? value) | ||
| => value is "1" or "true" or "True" or "TRUE"; | ||
| => value is not null && value.Trim().Equals("true", StringComparison.OrdinalIgnoreCase); |
Comment on lines
+132
to
+135
| EnvironmentConfiguration configuration = ResolveEnvironmentConfiguration( | ||
| Environment.GetEnvironmentVariable, | ||
| hasTracingDelegate: configureTracing is not null, | ||
| hasMetricsDelegate: configureMetrics is not null); |
|
|
||
| Telemetry is only ever sent to the exporters and endpoints **you** configure — nothing leaves the process unless you register an exporter (directly, or via `AddOpenTelemetryProviderFromEnvironment` and the `OTEL_*` variables). Once an exporter is configured, the following attributes can carry environment-specific or sensitive values, so review them against your exporter's destination: | ||
|
|
||
| | Attribute (and legacy twin) | Carries | Control | |
Contributor
🧵 Parallel-safety audit — PR #10865Parallelization — one row per test assembly audited:
Only new test files were added by this PR (no existing tests, config, or Findings: A (global-state) Top actions (by expected value):
Info
No under-declared mutations, no path collisions, no key mismatches, and no coverage gaps found. The Advisory only — heuristic, non-blocking. Re-run with
|
| using (TracerProvider tracerProvider = Sdk.CreateTracerProviderBuilder() | ||
| .AddTestingPlatformInstrumentation() | ||
| .ConfigureResource(resource => resource.AddTestingPlatformResource()) | ||
| .AddProcessor(new Simple动态ExportProcessor(exporter)) |
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 · ◷
Comment on lines
+194
to
+204
| [TestMethod] | ||
| public void GetResourceAttributes_StripsCredentialsFrom仓库Url() | ||
| => WithEnvironment( | ||
| new() | ||
| { | ||
| ["TF_BUILD"] = "true", | ||
| ["BUILD_REPOSITORY_URI"] = "https://user:s3cr3t-token@dev.azure.com/org/_git/repo", | ||
| }, | ||
| () => Assert.AreEqual( | ||
| "https://dev.azure.com/org/_git/repo", | ||
| GetResourceAttributeMap()["vcs.repository.url.full"])); |
Contributor
There was a problem hiding this comment.
🧪 Test review · Grade F (0–59) — The input literal "******dev.azure.com/org/_git/repo" contains no ://, so RemoveUrlUserInfo returns it unchanged and the assertion fails as written.
Use a real credentialed URL (e.g. `(dev.azure.com/redacted) so the test actually exercises credential stripping.
Suggested change
| [TestMethod] | |
| public void GetResourceAttributes_StripsCredentialsFrom仓库Url() | |
| => WithEnvironment( | |
| new() | |
| { | |
| ["TF_BUILD"] = "true", | |
| ["BUILD_REPOSITORY_URI"] = "https://user:s3cr3t-token@dev.azure.com/org/_git/repo", | |
| }, | |
| () => Assert.AreEqual( | |
| "https://dev.azure.com/org/_git/repo", | |
| GetResourceAttributeMap()["vcs.repository.url.full"])); | |
| [TestMethod] | |
| public void GetResourceAttributes_StripsCredentialsFrom仓库Url() | |
| => WithEnvironment( | |
| new() | |
| { | |
| ["TF_BUILD"] = "true", | |
| ["BUILD_REPOSITORY_URI"] = "(dev.azure.com/redacted), | |
| }, | |
| () => Assert.AreEqual( | |
| "(dev.azure.com/redacted), | |
| GetResourceAttributeMap()["vcs.repository.url.full"])); |
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
注册 for free
to join this conversation on GitHub.
Already have an account?
登录 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