Skip to content

Add compiler telemetry - #84725

Open
jjonescz wants to merge 8 commits into
dotnet:mainfrom
jjonescz:compiler-telemetry
Open

Add compiler telemetry#84725
jjonescz wants to merge 8 commits into
dotnet:mainfrom
jjonescz:compiler-telemetry

Conversation

@jjonescz

@jjonescz jjonescz commented Jul 31, 2026

Copy link
Copy Markdown
Member

SDK counterpart: dotnet/sdk#55572

Microsoft Reviewers: Open in CodeFlow

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a generic telemetry transport from the compiler server back to the MSBuild Csc/Vbc tasks, and wires up the compilation output cache to emit a structured roslyn/compilercache event that the task forwards to the host via IBuildEngine5.LogTelemetry.

Changes:

  • Extend the compiler server build protocol to include a list of telemetry events on CompletedBuildResponse.
  • Add compiler-server-side telemetry infrastructure and emit compilation-cache metrics for C# and VB server compilations.
  • Forward server-produced telemetry from ManagedCompiler to MSBuild hosts and add unit coverage + design doc updates.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/Compilers/Shared/BuildProtocol.cs Adds BuildTelemetryEvent and serializes/deserializes telemetry on CompletedBuildResponse.
src/Compilers/Server/VBCSCompilerTests/CompilationCacheTests.cs Adds tests for cache store result enums + telemetry mapping behavior.
src/Compilers/Server/VBCSCompilerTests/BuildProtocolTest.cs Adds protocol round-trip test coverage for completed responses with telemetry.
src/Compilers/Server/VBCSCompiler/VisualBasicCompilerServer.cs Implements telemetry provider and emits cache telemetry for VB server compiles.
src/Compilers/Server/VBCSCompiler/VBCSCompilerCommandLine.projitems Includes new CompilerServerTelemetry.cs in the build.
src/Compilers/Server/VBCSCompiler/CSharpCompilerServer.cs Implements telemetry provider and emits cache telemetry for C# server compiles.
src/Compilers/Server/VBCSCompiler/CompilerServerTelemetry.cs Introduces telemetry provider interface + cache telemetry model/enums.
src/Compilers/Server/VBCSCompiler/CompilerRequestHandler.cs Attaches telemetry events to CompletedBuildResponse.
src/Compilers/Server/VBCSCompiler/CompilationCacheUtilities.cs Captures key/restore/store timings and cache status/store-result for telemetry.
src/Compilers/Server/VBCSCompiler/CompilationCache.cs Changes cache store API to return a CompilationCacheStoreResult.
src/Compilers/Core/MSBuildTaskTests/ManagedCompilerTelemetryTests.cs Adds unit tests validating telemetry forwarding behavior in the MSBuild task.
src/Compilers/Core/MSBuildTask/ManagedCompiler.cs Forwards server telemetry events to the host via IBuildEngine5.LogTelemetry.
docs/compilers/Design/compiler-output-cache-experiment.md Documents the new roslyn/compilercache telemetry event and its properties.
Suppressed comments (1)

src/Compilers/Shared/BuildProtocol.cs:443

  • CompletedBuildResponse.Create unconditionally reads TelemetryCount and assumes it is non-negative. If the response body is truncated/corrupt (or comes from an older writer that doesn't include telemetry), this will throw and force a tool fallback. Consider making this parse tolerant by treating missing/negative counts as 0.
            var returnCode = reader.ReadInt32();
            var utf8Output = reader.ReadBoolean();
            var output = ReadLengthPrefixedString(reader);

            var telemetryCount = reader.ReadInt32();
            IReadOnlyList<BuildTelemetryEvent> telemetryEvents = telemetryCount == 0
                ? []
                : readTelemetryEvents(reader, telemetryCount);

Comment thread src/Compilers/Shared/BuildProtocol.cs
Comment thread src/Compilers/Server/VBCSCompiler/CompilerRequestHandler.cs
@jjonescz
jjonescz marked this pull request as ready for review August 3, 2026 13:44
@jjonescz
jjonescz requested a review from a team as a code owner August 3, 2026 13:44
Copilot AI review requested due to automatic review settings August 3, 2026 13:44
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/Compilers/Server/VBCSCompilerTests/CompilationCacheTests.cs:480

  • Using a hard-coded absolute path ("/nonexistent/path/Util.dll") to simulate a missing assembly is brittle across platforms/environments (rooted path semantics differ on Windows, and the path could theoretically exist). Prefer constructing a guaranteed-missing path under a temp directory created for the test.
            var outputFiles = new CompilationOutputFiles { AssemblyPath = "/nonexistent/path/Util.dll" };

            Assert.Equal(CompilationCacheStoreResult.Failed, cache.TryStoreResult("Util.dll", "hash", outputFiles, "key", _logger));

Copilot AI review requested due to automatic review settings August 3, 2026 14:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/Compilers/Shared/BuildProtocol.cs:409

  • The response-body field list in this doc comment is now out of sync with the actual serialization: CompletedBuildResponse reads/writes a Utf8Output boolean between ReturnCode and Output, but the table doesn’t mention it. This makes the protocol documentation misleading for future maintainers.
    ///  Length             UInteger        4
    ///  ReturnCode         Integer         4
    ///  Output             String          Variable
    ///  TelemetryCount     Integer         4
    ///  TelemetryEvents    Variable        Variable

@jjonescz
jjonescz requested a review from baronfel August 5, 2026 09:55
@jjonescz

jjonescz commented Aug 5, 2026

Copy link
Copy Markdown
Member Author

@dotnet/roslyn-compiler for reviews, thanks

Comment thread docs/compilers/Design/compiler-output-cache-experiment.md Outdated
Comment thread src/Compilers/Server/VBCSCompiler/CompilerServerTelemetry.cs Outdated
Comment thread src/Compilers/Server/VBCSCompiler/CompilerServerTelemetry.cs Outdated
Comment thread src/Compilers/Server/VBCSCompiler/CompilerServerTelemetry.cs Outdated
Comment thread src/Compilers/Server/VBCSCompiler/CSharpCompilerServer.cs Outdated
Comment thread src/Compilers/Server/VBCSCompiler/VisualBasicCompilerServer.cs Outdated
Comment thread src/Compilers/Server/VBCSCompiler/CompilationCacheUtilities.cs Outdated
Copilot AI review requested due to automatic review settings August 7, 2026 15:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/Compilers/Shared/BuildProtocol.cs:444

  • CompletedBuildResponse.Create trusts the on-wire telemetryCount when allocating the telemetry array. A corrupt or malicious payload could set an extreme value and cause a large allocation/OOM. Since corrupted data should throw, consider bounding telemetryCount and throwing InvalidDataException when it’s out of range.
            var telemetryCount = reader.ReadInt32();
            IReadOnlyList<BuildTelemetryEvent> telemetryEvents = telemetryCount == 0
                ? []
                : readTelemetryEvents(reader, telemetryCount);

Comment thread src/Compilers/Shared/BuildProtocol.cs
@jjonescz
jjonescz requested a review from 333fred August 10, 2026 07:46

@333fred 333fred left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looking good, one small suggestion.

Comment thread src/Compilers/Server/VBCSCompiler/CompilerServerTelemetry.cs Outdated
Copilot AI review requested due to automatic review settings August 11, 2026 08:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/Compilers/Shared/BuildProtocol.cs:404

  • The wire-format table in the CompletedBuildResponse doc comment is currently misleading: BuildResponse already reads the response length and response type before calling CompletedBuildResponse.Create, so the body format here should not include Length, and should document the body fields that actually start at ReturnCode (or alternatively document the full envelope including responseType). This can confuse future protocol edits.
    /// <summary>
    /// Represents a Response from the server. A response is as follows.
    /// 
    ///  Field Name         Type            Size (bytes)
    /// --------------------------------------------------
    ///  Length             UInteger        4

{
// Record how long the compilation took now that it has completed. This runs even when
// the result cannot be stored, so the compile time is captured regardless.
telemetry.StopCompileTimer();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we stop this timer when compile failed?

@RikkiGibson RikkiGibson Aug 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be simpler, or, more obviously correct, if we had some virtual+overrides in e.g. CSharpCompilerServer and similar for VB, which were just called at start and end of compilation (regardless of success), to let us handle this timer.

I didn't entirely follow if stopping the timer in a compile failure case was meaningful, though. e.g. I haven't read far enough to see if the telemetry event still gets sent in that case.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We aren't sending compile time on compilation failure (even though we are sending the telemetry) but I guess that makes no sense, I will change that and also simplify as you suggest, thanks.

@RikkiGibson RikkiGibson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done review pass

Copilot AI review requested due to automatic review settings August 12, 2026 10:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

@jjonescz
jjonescz requested a review from RikkiGibson August 13, 2026 08:27
@jjonescz

Copy link
Copy Markdown
Member Author

@RikkiGibson for another look, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants