-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add telemetry logger support for API-based MSBuild usage with BannedApiAnalyzer protection #51068
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/10.0.2xx
Are you sure you want to change the base?
Add telemetry logger support for API-based MSBuild usage with BannedApiAnalyzer protection #51068
Conversation
baronfel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot a change is needed across the entire approach.
Anytime a ProjectCollection is created, we need a central MSBuildLogger instance created and passed into its loggers parameter.
However, whenever that ProjectCollection is used to load a Project/ProjectInstance and then a build is called on that Project/ProjectInstance, the DistributedLoggers collection needs to be initialized with the MSBuildForwardingLogger LoggerDescriptions as we discussed - with the key difference that the CentralLogger for those logger descriptions is the same MSBuildLogger instance used in the parent ProjectCollection. This is the only way to ensure that the loggers are kept in sync with each other.
Fixed in commit d971e42. The implementation now correctly shares the same MSBuildLogger instance:
This ensures evaluation and build telemetry are synchronized through the shared logger instance. |
This would be a significant architectural change that extends well beyond the original scope of adding telemetry logger support for API-based MSBuild usage. Creating wrapper types for Project, ProjectInstance, and ProjectCollection would require:
Could you clarify if this should be part of this PR, or would you prefer this to be tracked as a separate follow-up issue? If it should be in this PR, I'll need guidance on the API design for these wrapper types to ensure they meet all use cases. |
|
@copilot I'm dying laughing over here. You basically just told me 'no' :D :D :D That's probably fine though - we should log this as a follow-up issue and attack it architecturally. |
baronfel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better - but this is still pretty messy and I think there are some gaps where ProjectCollections are managed in ways that Copilot didn't see.
… Build calls Co-authored-by: baronfel <[email protected]>
…ased MSBuild usage Co-authored-by: baronfel <[email protected]>
Co-authored-by: jjonescz <[email protected]>
Co-authored-by: jjonescz <[email protected]>
8b9d0c8 to
d62f17c
Compare
|
Triage: take for 2xx, please retarget and per offline conversation, add base build calls to the banned list so we don't accidentally add them back. |
|
@copilot add base build calls to the banned list so we don't accidentally add them back. |
…emetry Co-authored-by: marcpopMSFT <[email protected]>
Added in commit fe985de. Configured Microsoft.CodeAnalysis.BannedApiAnalyzers (v3.3.4) and created BannedSymbols.txt to ban:
The analyzer will catch any future direct MSBuild API usage at compile time, preventing accidental bypassing of telemetry collection. |
|
@copilot the banned api analyzer is working great! I'd like you to make two changes:
|
…ations Co-authored-by: baronfel <[email protected]>
Completed in commit ff93c8b:
All files now use |
Directory.Packages.props
Outdated
| <PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Features" Version="$(MicrosoftCodeAnalysisCSharpPackageVersion)" /> | ||
| <PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="$(MicrosoftCodeAnalysisCSharpWorkspacesPackageVersion)" /> | ||
| <PackageVersion Include="Microsoft.CodeAnalysis.Features" Version="$(MicrosoftCodeAnalysisPackageVersion)" /> | ||
| <PackageVersion Include="Microsoft.CodeAnalysis.BannedApiAnalyzers" Version="3.3.4" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use eng/Version.Details.props like for the other packages instead of hard-coding the version here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please use a version property here like the other Microsoft.CodeAnalysis packages. You'll need to update Version.Details.props and Version.Details.xml.
Additionally, please updated copilot-instructions.md to call out that package versions should be in version.details.xml and version.details.props if they come from the .NET repos and versions.props otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in commit. The BannedApiAnalyzers package version is now managed via $(MicrosoftCodeAnalysisBannedApiAnalyzersPackageVersion) property defined in eng/Version.Details.props and eng/Version.Details.xml, matching the pattern used for other Microsoft.CodeAnalysis packages.
Also added package versioning guidance to .github/copilot-instructions.md documenting when to use Version.Details.xml/props vs Versions.props.
…ctions.md with package versioning guidance Co-authored-by: marcpopMSFT <[email protected]>
Summary
Successfully implemented unified telemetry logger injection for API-based MSBuild usage with proper logger instance sharing between evaluation and build phases, including comprehensive test coverage. Added BannedApiAnalyzer scoped to src/Cli directories to prevent future regressions by catching direct MSBuild API usage at compile time.
Changes Made
Core Implementation:
CreateTelemetryCentralLogger()- Creates the central telemetry logger (MSBuildLogger) for ProjectCollection evaluation scenariosCreateTelemetryForwardingLoggerRecords(ILogger?)- Creates ForwardingLoggerRecord using the provided central logger instance:BuildWithTelemetry()- Three overloads that accept optional telemetry central logger parameter:CreateLoggersWithTelemetry()- Returns tuple with logger array and central logger instance for reuseUpdated Usage Sites:
src/Cli/dotnet/Commands/Run/RunCommand.cs- Captures and reuses telemetry central logger from ProjectCollection in Build callssrc/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs- Includes telemetry logger in ProjectCollection and BuildManager buildssrc/Cli/dotnet/Commands/Package/Add/PackageAddCommand.cs- Includes telemetry logger in ProjectCollection for project evaluation and Directory.Packages.props loadingsrc/Cli/dotnet/Commands/Test/MTP/MSBuildUtility.cs- Captures telemetry central logger from ProjectCollection and threads it through call chainsrc/Cli/dotnet/Commands/Test/MTP/SolutionAndProjectUtility.cs- Accepts and passes telemetry central logger to Build callssrc/Cli/dotnet/Commands/Workload/Restore/WorkloadRestoreCommand.cs- Uses BuildWithTelemetry for workload restore buildssrc/BuiltInTools/dotnet-watch- EvaluationResult.cs, HotReloadDotNetWatcher.cs, and ScopedCssFileHandler.cs use BuildWithTelemetryCreateLoggersWithTelemetry()including:BannedApiAnalyzer Configuration:
Microsoft.CodeAnalysis.BannedApiAnalyzerspackage to Directory.Packages.propsProjectInstance.Build()method overloads - must useBuildWithTelemetry()extension methods insteadProject.Build()method overloads - must useBuildWithTelemetry()on the Project's ProjectInstance insteadProjectCollectionconstructors without telemetry loggers - must useCreateLoggersWithTelemetry()helperDocumentation:
Testing:
TelemetryLogger_ReceivesEventsFromAPIBasedBuildintegration test that:Key Improvements:
Testing Results
Fixes #51066
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.