push improved MSBuildLogger to a separate project in case we want to spin it off #49751

baronfel · 2025-07-12T19:39:49Z

This is part of reducing the diff in #49556.

During that PR, I implemented a number of improvements to the simple Microsoft.Extensions.Logging.Abstractions.ILogger wrapper we had that ties that ILogger to MSBuild's ILogger interfaces.

The main feature is being able to use state/props from MEL.ILogger messages as inputs to MSbuild's various parameters on the LogX overloads - things like code, file, line, etc can now all be set by props. State is also persisted and attached to messages so ambient context is available in the binlog and other MSBuild Loggers even if not directly used in the Message.

Copilot

Pull Request Overview

This PR extracts an improved MSBuild logging component into a separate project to potentially spin it off as a standalone package. The main enhancement is the ability to use properties from Microsoft.Extensions.Logging.ILogger messages as inputs to MSBuild's logging parameters (code, file, line, etc.), while preserving state information for binlog and other MSBuild loggers.

Moves MSBuild logging functionality from Microsoft.NET.Build.Containers to a new Microsoft.Extensions.Logging.MSBuild project
Enhances the logger to support extracting MSBuild-specific parameters from logging state/scope
Updates namespace and visibility from internal to public for the extracted components

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`src/Microsoft.Extensions.Logging.MSBuild/Microsoft.Extensions.Logging.MSBuild.csproj`	New project file defining the extracted MSBuild logging library
`src/Microsoft.Extensions.Logging.MSBuild/MSBuildLoggerProvider.cs`	Updated provider with scope support and public visibility
`src/Microsoft.Extensions.Logging.MSBuild/MSBuildLogger.cs`	Enhanced logger with parameter extraction from state/scope
`src/Containers/Microsoft.NET.Build.Containers/Tasks/CreateNewImage.cs`	Updated namespace import to use new project
`src/Containers/Microsoft.NET.Build.Containers/Tasks/CreateImageIndex.cs`	Updated namespace import to use new project
`src/Containers/Microsoft.NET.Build.Containers/Microsoft.NET.Build.Containers.csproj`	Added project reference to new logging library
`src/Containers/Microsoft.NET.Build.Containers/Logging/MSBuildLogger.cs`	Removed old implementation

Comments suppressed due to low confidence (1)

Copilot · 2025-07-12T19:40:33Z

src/Microsoft.Extensions.Logging.MSBuild/MSBuildLogger.cs

+                            continue;
+                        default:
+                            var wrappedKey = "{" + kvp.Key + "}";
+                            if (originalFormat.Contains(wrappedKey))


Potential null reference exception when originalFormat is null. The variable is declared as 'null!' but can remain null if no {OriginalFormat} key is found, causing a null reference when calling Contains().

Suggested change

if (originalFormat.Contains(wrappedKey))

if (originalFormat != null && originalFormat.Contains(wrappedKey))

originalFormat is also null if "{OriginalFormat}" is a key in the list but the foreach (var kvp in stateItems) loop has not reached it yet. There is no documented requirement that "{OriginalFormat}" be in the first element of the list.

Copilot · 2025-07-12T19:40:34Z

src/Microsoft.Extensions.Logging.MSBuild/MSBuildLogger.cs

+                                continue;
+                            default:
+                                var wrappedKey = "{" + kvp.Key + "}";
+                                if (originalFormat.Contains(wrappedKey))


Potential null reference exception when originalFormat is null. This is the same issue as line 140 - originalFormat can be null when no {OriginalFormat} key is found in the state items.

Suggested change

if (originalFormat.Contains(wrappedKey))

if (originalFormat != null && originalFormat.Contains(wrappedKey))

src/Containers/Microsoft.NET.Build.Containers/Microsoft.NET.Build.Containers.csproj

…spin it off

src/Microsoft.Extensions.Logging.MSBuild/MSBuildLogger.cs

KalleOlaviNiemitalo · 2025-07-13T04:53:38Z

src/Microsoft.Extensions.Logging.MSBuild/MSBuildLogger.cs

+        {
+            // state will be a FormattedLogValues instance
+            // scope will be our dictionary thing we need to probe into
+            scopeProvider.ForEachScope((scope, state) =>


I wonder if anyone would try to use the spun-off MSBuildLogger with log buffering, whose implementation in the Microsoft.Extensions.Telemetry package does not support scopes. Perhaps it is possible to work around that by implementing ILogEnricher to pull the attributes from the logger scopes into the log entries themselves.

This is an interesting question - to be very honest with you I don't think this particular MSBuild/MEL bridge would even be used in a 'general' sense, only in the very specific scenario of "a Task wants a unified logging abstraction for code that runs during the Task". In the extreme, this logger could potentially be set up/configured by the Engine itself and made ambiently available as an ILogger to Task implementations. That's all hypothetical and not designed at the moment though.

src/Microsoft.Extensions.Logging.MSBuild/MSBuildLogger.cs

KalleOlaviNiemitalo · 2025-07-13T05:35:54Z

src/Microsoft.Extensions.Logging.MSBuild/MSBuildLogger.cs

+                            var wrappedKey = "{" + kvp.Key + "}";
+                            if (originalFormat.Contains(wrappedKey))
+                            {
+                                // If the key is part of the format string of the original format, we don't need to append it again.
+                                continue;
+                            }


If the message template includes formatting like {CreationDate:O}, this won't match.

Totally agree - I should possibly look at interpreting this via CompositeFormat.

The CompositeFormat.Parse method does not support named parameter placeholders like log message templates have. The CompositeFormat class does not have public members for examining the structure of the format string. I don't think CompositeFormat is useful here.

I was hoping it would be slightly more capable. In the past I've used MessageTemplates for this kind of thing, but there would be source-build concerns with using an external library. Might have to punt for v1 of this - at least if it stays in the SDK repo directly.

KalleOlaviNiemitalo · 2025-07-13T05:41:46Z

src/Microsoft.Extensions.Logging.MSBuild/MSBuildLogger.cs

+                            continue;
+                        default:
+                            var wrappedKey = "{" + kvp.Key + "}";
+                            if (originalFormat.Contains(wrappedKey))


originalFormat is also null if "{OriginalFormat}" is a key in the list but the foreach (var kvp in stateItems) loop has not reached it yet. There is no documented requirement that "{OriginalFormat}" be in the first element of the list.

src/Microsoft.Extensions.Logging.MSBuild/MSBuildLogger.cs

src/Microsoft.Extensions.Logging.MSBuild/MSBuildLoggerProvider.cs

KalleOlaviNiemitalo · 2025-07-13T06:14:46Z

How do you create the state objects that are passed to ILogger.Log<TState> and then analysed by this logger?

Methods like LoggerExtensions.LogInformation and LoggerMessage.Define parse the log message template to learn the names of attributes, so if you use those and want to give e.g. "File" to MSBuild, then you have to include a "{File}" placeholder in the template. But then the formatted log message also includes the file name, and MSBuild adds the file name again, so the user sees it twice.

Is source-generated logging different in this respect?

baronfel · 2025-07-13T14:13:38Z

How do you create the state objects that are passed to ILogger.Log and then analysed by this logger?

Methods like LoggerExtensions.LogInformation and LoggerMessage.Define parse the log message template to learn the names of attributes, so if you use those and want to give e.g. "File" to MSBuild, then you have to include a "{File}" placeholder in the template. But then the formatted log message also includes the file name, and MSBuild adds the file name again, so the user sees it twice.

Is source-generated logging different in this respect?

The state objects are created via ILogger.BeginScope<TState>(TState state) - here's a specific example from the linked PR that I'm extracting this from. Serilog has good docs here, but basically it seems to be up to the specific logger how to handle various kinds of state - single primitive structures like strings you could add as some kind of tag, but collections/maps of key/value pairs are a natural fit for adding to a property bag of some kind, etc.

KalleOlaviNiemitalo · 2025-07-13T14:29:22Z

The state objects are created via ILogger.BeginScope<TState>(TState state)

Oh so if you don't want to include an attribute in the formatted log message, then you place it in the state of the logger scope, not in the state of the log entry.

- here's a specific example from the linked PR that I'm extracting this from.

That one doesn't seem to have any of the attributes that MSBuildLogger would recognise.

baronfel · 2025-07-13T14:45:32Z

That one doesn't seem to have any of the attributes that MSBuildLogger would recognise.

Correct - I didn't really make use of the MSBuild-specific attributes in that PR. They just kind of emerged from the fact that I could now walk state/scope in a structured way and do something with the attributes I found.

We have this problem in the SDK codebase more generally when we log in our Tasks where we have to call special "LogXFromFormat" methods because those know how to extract specific kinds of data from the shapes of our localized strings - "wouldn't it be nice if that was done in a more 'structured' way" was basically the genesis of this entire PR.

KalleOlaviNiemitalo · 2025-07-13T14:56:21Z

src/Microsoft.Extensions.Logging.MSBuild/MSBuildLoggerProvider.cs

+        if (!_loggers.TryGetValue(categoryName, out var logger))
+        {
+            logger = new MSBuildLogger(categoryName, _loggingHelper, _scopeProvider);
+            lock (_loggers)
+            {
+                _loggers[categoryName] = logger;
+            }
+        }


This kind of locking is still not thread-safe for Dictionary<TKey, TValue>, as the lock does not prevent reading and writing in parallel. It would be safe for Hashtable, though.

I kinda wondered if I should just use Concurrentdictionary.GetOrAdd for this, but I've heard/seen horror stories from ConcurrentDictionary internal locking...

Another option here is, make MSBuildLoggerProvider.CreateLogger create a new instance of MSBuildLogger on each call and not add it to any collection. Then, SetExternalScopeProvider would not be able to notify the MSBuildLogger instances, so each MSBuildLogger would instead have to keep a reference to MSBuildLoggerProvider and read the scope provider from there whenever it's needed. This would cost one more indirection in each method of ILogger but the locking would no longer be needed. Unnecessary instances of MSBuildLogger might get created but the garbage collector would clean them up.

In normal use though, LoggerFactory.CreateLogger already does lock (_sync) around the ILoggerProvider.CreateLogger calls, so there extra lock in MSBuildLoggerProvider.CreateLogger won't have any contention; I don't think ConcurrentDictionary there would help performance.

Copilot AI review requested due to automatic review settings July 12, 2025 19:39

baronfel added Area-CLI Area-MSBuild labels Jul 12, 2025

baronfel requested a review from a team July 12, 2025 19:39

Copilot AI reviewed Jul 12, 2025

View reviewed changes

push improved MSBuildLogger to a separate project in case we want to …

395a0e5

…spin it off

baronfel force-pushed the superpowered-msbuild-logger branch from fae2e39 to 395a0e5 Compare July 12, 2025 19:42

KalleOlaviNiemitalo mentioned this pull request Jul 12, 2025

Integrate Microsoft.Extensions.Logging ILogger with MSBuild's logging abstractions dotnet/msbuild#10757

Open

teo-tsirpanis reviewed Jul 12, 2025

View reviewed changes

src/Microsoft.Extensions.Logging.MSBuild/MSBuildLogger.cs Outdated Show resolved Hide resolved

correct packaging to account for the new dependency

0febdbe

baronfel requested a review from a team as a code owner July 12, 2025 21:06

more nuanced IsEnabled check

e116342

This was referenced Jul 12, 2025

dotnet-watch tests failing #40006

Open

Template engine tests hitting known parallelism issue in GetVisualStudioInstances #44878

Open

Containers integration tests are flaky when the network fails to load the registry #49502

Open

KalleOlaviNiemitalo reviewed Jul 13, 2025

View reviewed changes

add support for warning/error helpLink

72276d3

baronfel added 2 commits July 13, 2025 09:39

more efficient state processing

f057254

make logger creation and tracking more threadsafe

8720d27

KalleOlaviNiemitalo reviewed Jul 13, 2025

View reviewed changes

lock around the entire dictionary read to prevent read/write out of sync

53678d6

	if (originalFormat.Contains(wrappedKey))
	if (originalFormat != null && originalFormat.Contains(wrappedKey))

push improved MSBuildLogger to a separate project in case we want to spin it off #49751

Are you sure you want to change the base?

push improved MSBuildLogger to a separate project in case we want to spin it off #49751

Conversation

baronfel commented Jul 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 12, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KalleOlaviNiemitalo commented Jul 13, 2025

Uh oh!

baronfel commented Jul 13, 2025

Uh oh!

KalleOlaviNiemitalo commented Jul 13, 2025

Uh oh!

baronfel commented Jul 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KalleOlaviNiemitalo Jul 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KalleOlaviNiemitalo Jul 13, 2025 •

edited

Loading