Add telemetry for which error codes are suppressed at restore time #6290
Conversation
There was a problem hiding this comment.
PR Overview
This pull request adds telemetry support for tracking suppressed warning codes during restore operations. It introduces new telemetry event reporting in the restore command, extends test coverage for collecting suppressed warnings, and updates the logging implementation to capture these warnings.
- Introduces telemetry event for suppressed warning codes in RestoreCommand.
- Adds new unit tests to verify correct telemetry recording and warning suppression.
- Updates RestoreCollectorLogger to enqueue suppressed warnings.
Reviewed Changes
| File | Description |
|---|---|
| test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests/RestoreCommandTests.cs | Adds a test to verify telemetry population when warnings are suppressed. |
| test/NuGet.Core.Tests/NuGet.Commands.Test/CollectorLoggerTests.cs | Adds tests ensuring correct tracking of suppressed warnings. |
| src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs | Updates telemetry population logic to include suppressed warning codes. |
| src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/RestoreCollectorLogger.cs | Modifies warning suppression logic and introduces a suppressed warnings queue. |
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
test/NuGet.Core.Tests/NuGet.Commands.Test/CollectorLoggerTests.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/RestoreCollectorLogger.cs
Outdated
Show resolved
Hide resolved
| @@ -615,6 +616,7 @@ private async Task<IEnumerable<RestoreTargetGraph>> GenerateRestoreGraphsAsync(T | |||
|
|
|||
| var errorCodes = ConcatAsString(new HashSet<NuGetLogCode>(logs.Where(l => l.Level == LogLevel.Error).Select(l => l.Code))); | |||
| var warningCodes = ConcatAsString(new HashSet<NuGetLogCode>(logs.Where(l => l.Level == LogLevel.Warning).Select(l => l.Code))); | |||
| var suppressedWarningCodes = ConcatAsString(new HashSet<NuGetLogCode>(_logger.SuppressedWarnings.Select(l => l.Code))); | |||
There was a problem hiding this comment.
General question for everyone: This allocates a string that may or may not be used depending on if the telemetry pipeline is available. Shouldn't all of those allocations only happen if telemetry is available?
There was a problem hiding this comment.
We do collect quite a bit of data for telemetry, so I think it might be worth figuring out if we can gate it on NuGetTelemetryService being non-null.
My vote would be create an issue for this, since there are ways we can improve things.
| @@ -615,6 +621,15 @@ private async Task<IEnumerable<RestoreTargetGraph>> GenerateRestoreGraphsAsync(T | |||
|
|
|||
| var errorCodes = ConcatAsString(new HashSet<NuGetLogCode>(logs.Where(l => l.Level == LogLevel.Error).Select(l => l.Code))); | |||
| var warningCodes = ConcatAsString(new HashSet<NuGetLogCode>(logs.Where(l => l.Level == LogLevel.Warning).Select(l => l.Code))); | |||
| var suppressedWarningCodes = ConcatAsString(new HashSet<string>(_logger.SuppressedWarnings.Select(l => | |||
| { | |||
| if (string.IsNullOrEmpty(l.LibraryId)) | |||
There was a problem hiding this comment.
Is this tracking whether it was suppressed at the package id level vs the project level?
If we were gonna get anything, I think that's more valuable.
tbh, I feel like I'd rather just get the suppressed warnings, but that's just me.
Keep in mind we need to parse this when we "cook" the data. I'd rather have an array.
Bug
Fixes: https://github.com/NuGet/Client.Engineering/issues/2384
Description
Modified RestoreCollectorLogger to keep track of which warnings were suppressed. This is then added to telemetry.
PR Checklist
Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.