Add structured task logging design specification

[baronfel]

Context

PR #14606 prototypes structured task logging. This PR separates the design for review before or independently from the implementation.

The primary motivation is to reduce binary-log size by storing a reusable original format and separate values instead of repeatedly materialized display strings. The structure also preserves value meaning and enables importance-filtered messages to skip interpolation work.

Changes Made

Adds documentation/specs/proposed/structured-task-logging.md with:

• Task-author APIs for interpolated, dynamic, non-C#, and localized callers.
• Overload resolution, naming, formatting, null, alignment, culture, and materialization semantics.
• Message filtering and warning/error policy behavior.
• In-process events, node transport, binary-log versioning, deduplication, replay, and Change Wave compatibility.
• MediumRun performance and allocation evidence.
• Security, alternatives, rollout, and test plans.
• Open decisions for duplicate names, capture culture, diagnostic metadata placement, old readers, mixed nodes, and viewer rollout.

The specification also identifies a prototype blocker where the out-of-process TaskHost receive path can drop structured events.

Testing

• Verified both relative documentation links.
• Checked Markdown formatting with git diff --check.
• Confirmed the branch differs from origin/main by this documentation file only.

Notes

This PR changes documentation only. It does not change production code, tests, packages, SDK selection, or NuGet configuration.

Implementation prototype: #14606

[Copilot]

Pull request overview

Adds a proposed design specification for “structured task logging” to enable task log events to carry an invariant named template (OriginalFormat) plus ordered name/value pairs, with the goals of improving binary log deduplication opportunities and enabling importance-filtered logging to skip interpolation work while preserving existing Message-based logger behavior.

Changes:

• Introduces a full design spec covering task-author APIs (interpolated string handler + explicit structured APIs) and detailed naming/formatting/materialization semantics.
• Documents transport and compatibility design: in-proc event model, node transport expectations (including the noted prototype TaskHost receive gap), binary log record/versioning approach, and Change Wave gating plan.
• Captures measurement evidence, security/privacy considerations, alternatives, rollout, and a structured list of open questions for team review.

[github-actions]

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• patchdiff.githubusercontent.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "patchdiff.githubusercontent.com"

See Network Configuration for more information.

Generated by Expert Code Review (on open) for #14648 · sonnet46 · 71.2 AIC · ⌖ 8.2 AIC · ⊞ 5.2K

[github-actions]

Critical: Structured errors and warnings will be silently lost in old binary-log readers

The spec states: "The record does not contain an encoded classic event. Therefore, an old reader that skips the record also skips the complete event."

A lost structured error event means a failed build can appear to succeed on replay — a serious correctness regression. A lost structured warning breaks downstream tooling that relies on warning counts or warning-as-error policy in replayed logs.

Open question #7 (dual binary records) touches this, but the spec should resolve this before the implementation proceeds — it's the most consequential design decision in the entire document. Options:

a) Emit a classic fallback record alongside every structured record — guarantees old reader fidelity at the cost of some size overhead (partially defeating the size goal).
b) Require consumers to update to a minimum reader version before the feature ships — see rollout step 3 and open question #15.
c) Document that forward-compatible skip of an error/warning is a known limitation and specify a reader upgrade path.

Option (c) is the weakest choice given MSBuild's binary-log reliability guarantees. Please resolve this before merge.

[github-actions]

Performance overhead needs end-to-end evidence before shipping

The benchmarks show +137.7% time and +144 B allocations for the common case (enabled capture, two strings). The spec acknowledges this is prototype overhead and that "They do not yet prove the primary binary-log size goal."

This is a significant concern for a feature whose primary motivation is performance improvement:

• The per-event overhead is substantial on hot paths where thousands of events are emitted per build.
• The binary-log size reduction only materializes when templates are reused at high frequency — a pattern that may not be representative of typical task logging.
• The classic composite baseline ("Resolved {0}" style) already avoids the eager-string-creation problem; the comparison is slightly unfair to the new design.

The rollout plan step 6 ("Measure production binary logs before wider migration") is good, but the spec should state a pre-ship gate: if end-to-end logs for typical SDK builds do not show a net positive, the migration should halt.

Suggest adding an explicit acceptance criterion to the rollout plan.

[github-actions]

TaskHost gap needs a resolution path, not just a blocker label

The spec correctly identifies the TaskHost routing gap as "an implementation blocker, not a proposed behavior", but stops there. For a spec that's meant to guide implementation, this section should specify:

1. What the fix looks like: The receive switch in TaskHostTask needs cases for the three new event identifiers, routing each to the appropriate IBuildEngine.Log*Event call.
2. What tests are required: Specifically, a test that verifies a structured error emitted in an out-of-process task host fails the build (already mentioned in the testing plan at line ~865, good).
3. Whether this gap affects the Change Wave design: If the TaskHost routing is missing, should the Change Wave be disabled entirely for out-of-process task hosts as a fallback, rather than dropping events silently?

Without routing guidance here, an implementer might add the identifiers to the serialization switch but miss the deserialization receive path, recreating the same gap.

[github-actions]

Interface nullability contract is underspecified

public interface IStructuredBuildEventArgs
{
    string? OriginalFormat { get; }
    IReadOnlyList<KeyValuePair<string, string?>>? StructuredValues { get; }  
}

Both members are nullable, but the spec doesn't define when null is valid. Open question #12 ("Should the interface permit null after normal construction, or only during deserialization?") should be resolved in this spec.

Specifically:

• A null OriginalFormat on a live structured event is semantically meaningless — the event can't be grouped or analyzed. Should this be non-null (with null only allowed during deserialization of a legacy event that implements the interface by default)?
• A null StructuredValues alongside a non-null OriginalFormat is an inconsistency. Should the contract guarantee StructuredValues is non-null whenever OriginalFormat is non-null?

This ambiguity will cause defensive null checks in every logger that consumes the interface, adding overhead that compounds across thousands of events per build.

[github-actions]

Mixed-version node compatibility is unresolved but critical

The spec states: "The current node protocol requires both endpoints to recognize the new event identifiers."

This means in a mixed-version build graph (e.g., VS 17.x invoking MSBuild 18.11 worker nodes, or a solution build spanning different project SDK versions), structured events cannot be safely forwarded. But the spec leaves the resolution to open question #9.

This is a blocking gap, not a minor detail. Possible resolutions to add to the spec:

1. New nodes downgrade to classic events when connecting to an older parent node — requires version negotiation at node connect time.
2. Change Wave 18.11 is disabled when any participating node is below 18.11 — simpler but very conservative.
3. New nodes always emit both structured and classic records in node protocol — higher bandwidth cost but safe.

Without a decision here, the feature cannot ship safely in VS-hosted build scenarios, where mixed node versions are common during the VS update transition window.

[github-actions]

LogWarning handler overloads are missing warningCode nullability contract

The simplified warning overload:

public void LogWarning(
    string warningCode,
    ref StructuredLogInterpolatedStringHandler message);

The spec says "Every structured warning and error overload requires a non-empty code" (stated at line ~451), but this is specified as throwing FormatException for malformed templates and ArgumentException for named-list mismatches. The spec doesn't state what exception is thrown for an empty/null warningCode, or whether it's validated eagerly or lazily.

Given that existing LogWarning overloads accept null code (to produce an uncoded warning), this new overload's stricter contract is a deliberate API design choice. The spec should:

1. Explicitly state the exception type for null/empty warningCode (suggest ArgumentException).
2. Explain the design decision to require a code (good rationale: structured warnings that feed into tooling need stable identifiers for suppression).
3. Confirm whether null is accepted and silently treated as an empty string, or rejected outright.

[rainersigwald]

In general I think I like this idea but I'm concerned about whether it's relevant. If we're already doing a functionally identical argument deduplication with the existing format strings + args, does this buy much? It's a better API for sure, but generally it's not super important and we can push pain (using old-school format strings instead of interpolation) on specific tasks that log a lot.

[rainersigwald]

Strong disagree here: the primary goal should be to reduce the overhead of logging structured messages. Binlog size reduction is desirable but secondary--if binlogs were 20% bigger but had less wall-clock-time overhead, I would take that trade.

[JanProvaznik]

I think binlog size is very desirable proxy outcome for big builds.

1. If we plan to collect many binlogs the sizes in storage add up
2. time to upload and download in pipelines (possibly multiple times) adds up

[rainersigwald]

Not sure I understand what this means.

[rainersigwald]

Suggested change

	The compiler selects an interpolated string handler after the task recompiles.
	The compiler selects an interpolated string handler after the task recompiles, with no code change required.

[rainersigwald]

Our old-school logging APIs also have TaskLoggingHelper.LogMessage(string message, params object[] messageArgs) where string message is format string with {1} holes. Can we do that transparently too?

[rainersigwald]

Suggested change

	Structured loggers can also read `IStructuredBuildEventArgs`.
	Updated loggers may also read `IStructuredBuildEventArgs`.

[rainersigwald]

Expand on this please?

[rainersigwald]

Is this a standard pattern?

[rainersigwald]

This requires compiler cooperation and C# 10+, right? So people writing .NET 4.7.2 tasks with default settings wouldn't get it?

[rainersigwald]

Suggested change

	The `params ReadOnlySpan<T>` overloads give C# callers an allocation-free argument container.
	The `params ReadOnlySpan<T>` overloads give C# callers on .NET an allocation-free argument container.

[baronfel]

Taking this back to draft because of some things I've learned about the existing lazyformattedbuildeventargs - namely that it already does the format-string + separate arguments pattern. It just doesn't handle the names of the arguments, which aren't strictly necessary but are nice for structured logging/querying.