Instrumentation module for BuildCheck#9890
Instrumentation module for BuildCheck#9890maridematte wants to merge 7 commits intodotnet:exp/build-analyzersfrom
Conversation
|
Right now we only have time for |
f-alizada
left a comment
There was a problem hiding this comment.
Thank you for the PR, left some clarification + nit comments :)
src/Build/BuildCheck/Infrastructure/BuildCheckConnectorLogger.cs
Outdated
Show resolved
Hide resolved
src/Build/BuildCheck/Infrastructure/BuildCheckConnectorLogger.cs
Outdated
Show resolved
Hide resolved
f-alizada
left a comment
There was a problem hiding this comment.
Leaving minor comments, Thank you for the updates!
| // We do not want to send tracing stats from in-proc node | ||
| return; | ||
| } | ||
| string infraStatPrefix = "infrastructureStat_"; |
There was a problem hiding this comment.
The value "infrastructureStat_" used twice, please consider to share the duplicated value via const variables.
| new ParameterlessSwitchInfo( new string[] { "filelogger8", "fl8" }, ParameterlessSwitch.FileLogger8, null), | ||
| new ParameterlessSwitchInfo( new string[] { "filelogger9", "fl9" }, ParameterlessSwitch.FileLogger9, null), | ||
| new ParameterlessSwitchInfo( new string[] { "distributedfilelogger", "dfl" }, ParameterlessSwitch.DistributedFileLogger, null), | ||
| new ParameterlessSwitchInfo( new string[] { "analyzeStats", "as" }, ParameterlessSwitch.AnalyzeStats, null), |
There was a problem hiding this comment.
nit: There was a decision not to use the short alias: https://github.com/dotnet/msbuild/blob/25b5f75df908bf749b1283d76b37996c6a02bb67/src/MSBuild/CommandLineSwitches.cs
Just wondering if we want to apply the decision here as well? @JanKrivanek what do you think?
JanKrivanek
left a comment
There was a problem hiding this comment.
Looks good - I have couple commnets for consideration
| // Infrastructure time keepers, examples for now | ||
| internal TimeSpan analyzerAcquisitionTime; | ||
| internal long analyzerSetDataSourceTime; | ||
| internal long newProjectAnalyzersTime; |
There was a problem hiding this comment.
Should all of those be TimeSpans?
| } | ||
| } | ||
|
|
||
| loggingContext.LogCommentFromText(MessageImportance.High, $"BuildCheck run times{Environment.NewLine}"); |
There was a problem hiding this comment.
Btw. as discussed here: #9853 (comment), the stats might rather be part of analysis, without an opt-in.
The opt-in migh be used to just flip the importance of the messages (default should be MessageImportance.Low - so that it's not part of console/file logs under default verbosity; the current opt-in can promote that to MessageImportance.High to make it visible in console even with the default verbosity)
| loggingContext.LogCommentFromText(MessageImportance.High, analyzerData); | ||
| } | ||
|
|
||
| private string BuildStatsTable(string title, Dictionary<string, TimeSpan> rowData) |
There was a problem hiding this comment.
For easy viewing and working with data I'd recommend some existing textual table format - e.g. csv or md table
Fixes #9629
Context
We need some timers and performance indicators for BuildCheck.
--analyzerStatsoption so it will report the time the BuildCheck infrastructure and individual analyzers performed.