Repository navigation
Conversation
|
Azure Pipelines: Successfully started running 2 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Member
Author
|
Big sad, I based this off |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate review findings remain unresolved: CI-path coverage is missing, and live endpoint tests default to a shared resource.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Replaces the custom telemetry persistence/export pipeline with Azure Monitor OpenTelemetry Exporter 1.9.0 for managed and Native AOT CLI paths.
Changes:
- Removes obsolete custom telemetry implementation and tests.
- Configures exporter persistence and shutdown behavior.
- Updates dependencies, lifecycle coverage, and documentation.
File summaries
| File | Summary |
|---|---|
test/Microsoft.DotNet.Cli.Telemetry.Tests/RealEndpointTelemetryE2ETests.cs |
Adds live endpoint coverage. Moderate (3 votes): require explicit opt-in instead of the default connection string. |
test/Microsoft.DotNet.Cli.Telemetry.Tests/PersistentStorageTelemetryUploaderTests.cs |
Removes obsolete uploader tests. |
test/Microsoft.DotNet.Cli.Telemetry.Tests/Microsoft.DotNet.Cli.Telemetry.Tests.csproj |
Updates telemetry test dependencies. |
test/Microsoft.DotNet.Cli.Telemetry.Tests/HttpTelemetryUploadTransportTests.cs |
Removes obsolete HTTP transport tests. |
test/Microsoft.DotNet.Cli.Telemetry.Tests/FileSystemTelemetryBlobStorageTests.cs |
Removes obsolete storage tests. |
test/Microsoft.DotNet.Cli.Telemetry.Tests/BreezePartialContentTests.cs |
Removes obsolete Breeze tests. |
test/Microsoft.DotNet.Cli.Telemetry.Tests/AzureMonitorTelemetrySerializerTests.cs |
Removes obsolete serializer tests. |
test/Microsoft.DotNet.Cli.Telemetry.Tests/AzureMonitorLogSerializerTests.cs |
Removes obsolete log serializer tests. |
test/Microsoft.DotNet.Cli.Telemetry.Tests/AzureMonitorExporterDifferentialTests.cs |
Adds exporter differential coverage. |
test/Microsoft.DotNet.Cli.Telemetry.Tests/AzureMonitorConnectionStringTests.cs |
Removes obsolete connection-string tests. |
test/Microsoft.DotNet.Cli.Telemetry.Tests/AzureExporterTestSupport.cs |
Adds isolated Azure exporter test support. |
test/Microsoft.DotNet.Cli.Telemetry.Tests/AzureExporterLifecycleTests.cs |
Tests exporter persistence and shutdown behavior. |
test/dotnet.Tests/TelemetryTests/TelemetryClientTests.cs |
Updates CLI telemetry tests. |
test/dotnet.Tests/TelemetryTests/PersistentStorageTelemetryE2ETests.cs |
Tests managed and AOT telemetry persistence. |
test/dotnet-aot.Tests/dotnet-aot.Tests.csproj |
Removes obsolete telemetry references. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/README.md |
Updates telemetry component documentation. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/PersistentStorageTracerProviderBuilderExtensions.cs |
Removes custom tracer-provider extensions. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/PersistentStorageTraceExporter.cs |
Removes the custom trace exporter. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/PersistentStorageTelemetryOptions.cs |
Removes custom telemetry options. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/PersistentStorageLoggerOptionsExtensions.cs |
Removes custom logger extensions. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/PersistentStorageLogExporter.cs |
Removes the custom log exporter. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/Microsoft.DotNet.Cli.Telemetry.csproj |
Updates the telemetry project. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/Implementation/TelemetryUploadResult.cs |
Removes custom upload result types. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/Implementation/TelemetryResourceContextFactory.cs |
Removes custom resource-context creation. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/Implementation/TelemetryResourceContext.cs |
Removes custom resource context. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/Implementation/PersistentStorageTelemetryUploader.cs |
Removes the custom telemetry uploader. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/Implementation/ITelemetryUploadTransport.cs |
Removes the custom upload transport abstraction. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/Implementation/ITelemetryBlobStorage.cs |
Removes the custom blob-storage abstraction. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/Implementation/HttpTelemetryUploadTransport.cs |
Removes the custom HTTP transport. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/Implementation/FileSystemTelemetryBlobStorage.cs |
Removes the custom filesystem storage. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/Implementation/BreezeWriter.cs |
Removes the custom Breeze writer. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/Implementation/BreezeSchema.cs |
Removes the custom Breeze schema. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/Implementation/BreezePartialContent.cs |
Removes custom partial-content handling. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/Implementation/BreezeEnvelope.cs |
Removes the custom Breeze envelope. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/Implementation/AzureMonitorTelemetrySerializer.cs |
Removes the custom telemetry serializer. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/Implementation/AzureMonitorLogSerializer.cs |
Removes the custom log serializer. |
src/Cli/Microsoft.DotNet.Cli.Telemetry/Implementation/AzureMonitorConnectionString.cs |
Removes custom connection-string handling. |
src/Cli/dotnet/Telemetry/TelemetryClient.cs |
Configures Azure exporter lifecycle behavior. Moderate (1 vote): add real CI-path subprocess coverage. |
src/Cli/dotnet/dotnet.csproj |
Updates CLI telemetry dependencies. |
src/Cli/dotnet-aot/dotnet-aot.csproj |
Adds Azure exporter support for AOT. |
sdk.slnx |
Removes the deleted telemetry project. |
eng/Versions.props |
Updates Azure Monitor and OpenTelemetry versions. |
documentation/project-docs/telemetry.md |
Documents telemetry delivery and shutdown behavior. |
Review details
Suppressed comments (1)
src/Cli/dotnet/Telemetry/TelemetryClient.cs:135
- The CI-specific exporter switches added here are only reproduced in
ExporterSettingsScope; no test instantiates the productionTelemetryClientwith CI detection enabled. The CLI E2E test explicitly removes CI markers and the host-matrix tests disable Azure export, so a typo in these switch names/values or in the CIFlushProviderspath can pass all tests. Add an isolated subprocess test that exercises the real CLI CI path and verifies it waits for an upload and retains retryable data.
AppContext.SetSwitch("Azure.Monitor.OpenTelemetry.Exporter.DisablePersistOnShutdown", s_isCIEnvironment);
AppContext.SetData("Azure.Monitor.OpenTelemetry.Exporter.ShutdownDrainBudgetMilliseconds", 0); // background upload will occur next time - reduce exit latency. This only impacts storage persistThenDrain, so not CI.
AppContext.SetSwitch("Azure.Monitor.OpenTelemetry.Exporter.PersistOnForceFlush", !s_isCIEnvironment);
s_tracerProviderBuilder.AddAzureMonitorTraceExporter(options =>
- Files reviewed: 43/43 changed files
- Comments generated: 1
- Review effort level: Lite
nagilson
force-pushed
the
nagilson-eliminate-custom-telemetry-solution
branch
from
September 11, 2026 19:32
eaea5fa to
315812b
Compare
nagilson
requested review from
a team,
lbussell and
mthalman
as code owners
September 11, 2026 19:32
nagilson
marked this pull request as draft
September 14, 2026 22:47
|
Azure Pipelines: Successfully started running 2 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
1 task done
baronfel
approved these changes
Sep 21, 2026
Member
Author
|
The failing test is a dotnet test flake, so merging this. |
This was referenced Sep 21, 2026
This was referenced Sep 25, 2026
5 of 14 tasks
nagilson
added a commit
that referenced
this pull request
Oct 23, 2026
…elease/11.0.1xx-to-release/11.0.2xx Conflict resolutions: - sdk.slnx, dotnet.csproj: keep Microsoft.DotNet.Cli.InternalMicrosoft (2xx, #56458); drop Microsoft.DotNet.Cli.Telemetry (removed in 1xx by #56274, replaced by Azure Monitor exporter 1.10.0). - Program.cs: combine 1xx exit-code-aware shutdown drain with 2xx internal-Microsoft detection wait. - Versions.props: take 1xx runtime version features (#56629: 8.0.33, 9.0.22, 10.0.MinorPatch+14) and restore 11.0.2xx preview band branding to match dotnet/dotnet#9512 (VersionSDKMinor 2, patch 00, preview label, empty iteration) instead of the auto-merged 1xx rtm label. Fixes for files reset to release/11.0.2xx by the flow bot: - Add Microsoft.NET.ProjectData.Tasks 5.13.0-1.26508.107 dependency (needed by #56243 from 1xx). - Disable central transitive pinning in tool_projectdata (content-only harvest) so the 2xx ProjectData package's NuGet.Versioning floor does not conflict with the 2xx NuGet band. - Update testfx dependencies to 1xx versions (MSTest 4.5.0-preview.26477.3, MTP 2.5.0-preview.26477.3) needed by 1xx HotReload.Client tests (#56609). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Oct 26, 2026
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.
Notable decisions:
5swas my selected shutdown time for CI300msis the bare minimum for NAOT failures to make it with relative consistency tested from my machines - so for failures, it enacts a300msshutdown delay. I viewed those as more important to capture.600msis the bare minimum for Managed code failures to make it with relative consistency tested from my machines, however,300msis at the edge of UX acceptable thresholds. If [FEATURE REQ] Allow Custom SubDirectory in Azure OTel Exporter Azure/azure-sdk-for-net#62997 is implemented and the two can share a sub-directory then the AOT side can cover for the managed side. The managed side is just plain slower.POSTand that the instrumented packet is actually rather small, so we'd hope that the numbers somewhat translate..lockfile Lease Incorrectly Blocks Subsequent Executables open-telemetry/opentelemetry-dotnet-contrib#5210 runs across 3 minutes from one another actually don't get reported unless you invoke the SDK again within2d(the blob validity lifetime)dotnetcommands, the drain is set to never run for non-CI environments outside of failure contexts.50msin Azure's new implementation, which may mean short-lived commands such asdotnet --infomay still not provide sufficient time to drain telemetry. This is an accepted risk. SeeEagerDrainDelayMillisecondsin https://raw.githubusercontent.com/Azure/azure-sdk-for-net/Azure.Monitor.OpenTelemetry.Exporter_1.9.0/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Internals/TransmitFromStorageHandler.cs.Background
This PR introduced a new telemetry implementation via a custom open telemetry exporter. Reliable CLI telemetry via a persist-then-drain OpenTelemetry exporter by baronfel · Pull Request #55211 · dotnet/sdk
The reason we did this was recorded within the PR descirption as well as
OpenTelemetrywithAzure Monitor Exporterseems to not be designed forCLIApplications · Issue #55184 · dotnet/sdk.You should probably also read [FEATURE REQ] Better support CLI applications in OTel Exporter by enabling manual control over blob storage & POST flush · Issue #60838 · Azure/azure-sdk-for-net.
In short, Azure Monitor Exporter had limitations where it did not typically wait long enough or actually enforce that telemetry storage persisted on short lived applications.
The team however made improvements to address our concerns in [Monitor OpenTelemetry Exporter] Persist telemetry on shutdown for short-lived applications by rajkumar-rangaraj · Pull Request #61818 · Azure/azure-sdk-for-net. And then [Monitor Exporter] Make the shutdown drain budget configurable by rajkumar-rangaraj · Pull Request #62340 · Azure/azure-sdk-for-net.
We coordinated over Outlook and they were great in designing a thoughtful solution for us that met our needs together. For context, using
ForceFlushin CI would still not result in correct telemetry delivery but solving that is only possible at the OTel layer, not the exporter layer.Why
There are many reasons, but really we do not want to maintain a long term fork of their code. This is quite an expensive cost and also introduces several risks such as desynced code or CVE Coordination Disclosures.
Timeframe
The target of this PR is Sept 14th - we must get this out before GA for RC2 to ensure that we have quality data coming in to the .NET platform which informs key decisions. Sept 22nd is the wiggle room but we want validation before RC2 goes out.
Tests
Different shutdown forceFlush budgets
A 'retained blob' means it didn't get sent.
Showing whether waiting 3m10s helped resend failed blobs
AOT vs Managed Failure Telemetry
Without extra shutdown budget: