-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Implement Text-Based Performance Log #5861
Changes from 5 commits
1b960f4
fca5b32
85475c4
88a4d9b
e70ae92
05902ba
bdc4ce7
ad04c82
2fa8afb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,117 @@ | ||||||||
// Copyright (c) Microsoft. All rights reserved. | ||||||||
// Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||||||||
|
||||||||
using System; | ||||||||
using System.Collections; | ||||||||
using System.Collections.Generic; | ||||||||
using System.Diagnostics; | ||||||||
using System.Globalization; | ||||||||
using System.IO; | ||||||||
using System.Linq; | ||||||||
using System.Threading; | ||||||||
|
||||||||
using Microsoft.Build.CommandLine; | ||||||||
using Microsoft.Build.Framework; | ||||||||
using Microsoft.Build.Shared; | ||||||||
using Microsoft.Build.UnitTests.Shared; | ||||||||
using Xunit; | ||||||||
using Xunit.Abstractions; | ||||||||
using Shouldly; | ||||||||
using System.IO.Compression; | ||||||||
using System.Reflection; | ||||||||
|
||||||||
namespace Microsoft.Build.UnitTests | ||||||||
{ | ||||||||
public class PerfLogTests | ||||||||
{ | ||||||||
#if USE_MSBUILD_DLL_EXTN | ||||||||
private const string MSBuildExeName = "MSBuild.dll"; | ||||||||
#else | ||||||||
private const string MSBuildExeName = "MSBuild.exe"; | ||||||||
#endif | ||||||||
|
||||||||
private readonly ITestOutputHelper _output; | ||||||||
|
||||||||
public PerfLogTests(ITestOutputHelper output) | ||||||||
{ | ||||||||
_output = output; | ||||||||
} | ||||||||
|
||||||||
[Fact] | ||||||||
[Trait("Category", "mono-osx-failing")] // Disable on Mono OSX, since Mono doesn't implement EventSource. | ||||||||
public void TestPerfLogEnabledProducedLogFile() | ||||||||
{ | ||||||||
// Setup perf log. | ||||||||
string perfLogDir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N")); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think all this would be easier with a TestEnvironment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please point me to an example that I should follow? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this example, but I'm a little biased. You can also use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using the test environment should make this simpler (since it handles cleanup), and allow avoiding the RunnerUtilities changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this definitely cleaned things up. |
||||||||
Dictionary<string, string> environmentVariables = new Dictionary<string, string>(); | ||||||||
environmentVariables["DOTNET_PERFLOG_DIR"] = perfLogDir; | ||||||||
|
||||||||
// Setup project directory. | ||||||||
string projectDir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N")); | ||||||||
string projectPath = Path.Combine(projectDir, "my.proj"); | ||||||||
|
||||||||
try | ||||||||
{ | ||||||||
Directory.CreateDirectory(perfLogDir); | ||||||||
Directory.CreateDirectory(projectDir); | ||||||||
|
||||||||
string content = ObjectModelHelpers.CleanupFileContents("<Project ToolsVersion='msbuilddefaulttoolsversion' xmlns='msbuildnamespace'><Target Name='t'><Warning Text='[A=$(A)]'/></Target></Project>"); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||||||||
File.WriteAllText(projectPath, content); | ||||||||
|
||||||||
string msbuildParameters = "\"" + projectPath + "\""; | ||||||||
|
||||||||
bool successfulExit; | ||||||||
RunnerUtilities.ExecMSBuild(RunnerUtilities.PathToCurrentlyRunningMsBuildExe, msbuildParameters, out successfulExit, environmentVariables: environmentVariables); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tiny nit:
Suggested change
|
||||||||
successfulExit.ShouldBeTrue(); | ||||||||
|
||||||||
// Look for the file. | ||||||||
// NOTE: We don't explicitly look for one file because it's possible that more components will add files that will show up here. | ||||||||
// It's most important to ensure that at least one file shows up because any others that show up will be there because MSBuild properly | ||||||||
// enabled this functionality. | ||||||||
string[] files = Directory.GetFiles(perfLogDir, "perf-*.log"); | ||||||||
Assert.NotEmpty(files); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
And also convert the rest to Shouldly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think you should also check to make sure those files didn't exist before? I don't think GetTempPath returns a truly random path each time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the rest to Shoudly. Thanks for pointing this out. With regard to the directory, GetTempPath doesn't return a random path, but the test creates a random GUID directory to ensure that the directory is empty, and then creates it, which would result in a thrown exception if it already existed. |
||||||||
Assert.All(files, f => Assert.True(new FileInfo(f).Length > 0)); | ||||||||
} | ||||||||
finally | ||||||||
{ | ||||||||
Directory.Delete(perfLogDir, true); | ||||||||
Directory.Delete(projectDir, true); | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
[Fact] | ||||||||
public void TestPerfLogDirectoryDoesNotExist() | ||||||||
{ | ||||||||
// Setup perf log. | ||||||||
string perfLogDir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N")); | ||||||||
Dictionary<string, string> environmentVariables = new Dictionary<string, string>(); | ||||||||
environmentVariables["DOTNET_PERFLOG_DIR"] = perfLogDir; | ||||||||
|
||||||||
// Setup project directory. | ||||||||
string projectDir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N")); | ||||||||
string projectPath = Path.Combine(projectDir, "my.proj"); | ||||||||
|
||||||||
try | ||||||||
{ | ||||||||
Directory.CreateDirectory(projectDir); | ||||||||
|
||||||||
string content = ObjectModelHelpers.CleanupFileContents("<Project ToolsVersion='msbuilddefaulttoolsversion' xmlns='msbuildnamespace'><Target Name='t'><Warning Text='[A=$(A)]'/></Target></Project>"); | ||||||||
File.WriteAllText(projectPath, content); | ||||||||
|
||||||||
string msbuildParameters = "\"" + projectPath + "\""; | ||||||||
|
||||||||
Assert.False(Directory.Exists(perfLogDir)); | ||||||||
|
||||||||
bool successfulExit; | ||||||||
RunnerUtilities.ExecMSBuild(RunnerUtilities.PathToCurrentlyRunningMsBuildExe, msbuildParameters, out successfulExit, environmentVariables: environmentVariables); | ||||||||
successfulExit.ShouldBeTrue(); | ||||||||
|
||||||||
Assert.False(Directory.Exists(perfLogDir)); | ||||||||
} | ||||||||
finally | ||||||||
{ | ||||||||
Directory.Delete(projectDir, true); | ||||||||
} | ||||||||
} | ||||||||
} | ||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,161 @@ | ||
using System; | ||
rainersigwald marked this conversation as resolved.
Show resolved
Hide resolved
|
||
using System.Diagnostics; | ||
using System.Diagnostics.Tracing; | ||
using System.IO; | ||
using System.Text; | ||
using Microsoft.Build.Eventing; | ||
|
||
namespace Microsoft.Build.CommandLine | ||
{ | ||
internal sealed class PerformanceLogEventListener : EventListener | ||
{ | ||
internal struct ProviderConfiguration | ||
{ | ||
internal string Name { get; set; } | ||
internal EventKeywords Keywords { get; set; } | ||
internal EventLevel Level { get; set; } | ||
} | ||
|
||
private static ProviderConfiguration[] s_config = new ProviderConfiguration[] | ||
{ | ||
new ProviderConfiguration() | ||
{ | ||
Name = "Microsoft-Build", | ||
Keywords = MSBuildEventSource.Keywords.PerformanceLog, | ||
Level = EventLevel.Verbose | ||
} | ||
}; | ||
|
||
private const string PerfLogDirEnvVar = "DOTNET_PERFLOG_DIR"; | ||
private const char EventDelimiter = '\n'; | ||
private string _processIDStr; | ||
private StreamWriter _writer; | ||
|
||
[ThreadStatic] | ||
private static StringBuilder s_builder; | ||
|
||
internal static PerformanceLogEventListener Create() | ||
{ | ||
PerformanceLogEventListener eventListener = null; | ||
try | ||
{ | ||
// Initialization happens as a separate step and not in the constructor to ensure that | ||
// if an exception is thrown during init, we have the opportunity to dispose of the listener, | ||
// which will disable any EventSources that have been enabled. Any EventSources that existed before | ||
// this EventListener will be passed to OnEventSourceCreated before our constructor is called, so | ||
// we if we do this work in the constructor, and don't get an opportunity to call Dispose, the | ||
// EventSources will remain enabled even if there aren't any consuming EventListeners. | ||
|
||
// Check to see if we should enable the event listener. | ||
string logDirectory = Environment.GetEnvironmentVariable(PerfLogDirEnvVar); | ||
if (Directory.Exists(logDirectory)) | ||
{ | ||
eventListener = new PerformanceLogEventListener(); | ||
eventListener.Initialize(logDirectory); | ||
} | ||
} | ||
catch | ||
{ | ||
if (eventListener != null) | ||
{ | ||
eventListener.Dispose(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if (eventListener != null && eventListener threw an exception) shouldn't we return null or throw? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's a good catch. Thanks. |
||
} | ||
} | ||
|
||
return eventListener; | ||
} | ||
|
||
private PerformanceLogEventListener() | ||
{ | ||
} | ||
|
||
internal void Initialize(string logDirectory) | ||
{ | ||
_processIDStr = Process.GetCurrentProcess().Id.ToString(); | ||
|
||
// Use a GUID disambiguator to make sure that we have a unique file name. | ||
string logFilePath = Path.Combine(logDirectory, $"perf-{_processIDStr}-{Guid.NewGuid().ToString("N")}.log"); | ||
|
||
Stream outputStream = new FileStream( | ||
logFilePath, | ||
FileMode.Create, // Create or overwrite. | ||
FileAccess.Write, // Open for writing. | ||
FileShare.Read, // Allow others to read. | ||
4096, // Default buffer size. | ||
FileOptions.None); // No hints about how the file will be written. | ||
|
||
_writer = new StreamWriter(outputStream); | ||
} | ||
|
||
public override void Dispose() | ||
{ | ||
lock (this) | ||
{ | ||
if (_writer != null) | ||
{ | ||
_writer.Dispose(); | ||
_writer = null; | ||
} | ||
} | ||
|
||
base.Dispose(); | ||
} | ||
|
||
protected override void OnEventSourceCreated(EventSource eventSource) | ||
{ | ||
try | ||
{ | ||
// Enable the provider if it matches a requested configuration. | ||
foreach (ProviderConfiguration entry in s_config) | ||
{ | ||
if (entry.Name.Equals(eventSource.Name)) | ||
{ | ||
EnableEvents(eventSource, entry.Level, entry.Keywords); | ||
} | ||
} | ||
} | ||
catch | ||
{ | ||
// If we fail to enable, just skip it and continue. | ||
} | ||
|
||
base.OnEventSourceCreated(eventSource); | ||
} | ||
|
||
protected override void OnEventWritten(EventWrittenEventArgs eventData) | ||
{ | ||
try | ||
{ | ||
if (s_builder == null) | ||
{ | ||
s_builder = new StringBuilder(); | ||
} | ||
else | ||
{ | ||
s_builder.Clear(); | ||
} | ||
|
||
s_builder.Append($"[{DateTime.UtcNow.ToString("o")}] Event={eventData.EventSource.Name}/{eventData.EventName} ProcessID={_processIDStr} ThreadID={System.Threading.Thread.CurrentThread.ManagedThreadId}\t "); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's an AppendFormat method for StringBuilder. I remember having tested whether it was faster than this, but I don't remember the answer. I think .Append over and over was fastest but also ugliest. |
||
for (int i = 0; i < eventData.PayloadNames.Count; i++) | ||
{ | ||
s_builder.Append($"{eventData.PayloadNames[i]}=\"{eventData.Payload[i]}\" "); | ||
} | ||
|
||
lock (this) | ||
{ | ||
if (_writer != null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When is _writer null, and should we error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My goal is to make this log as inobtrusive as possible. So if something goes wrong, I want the log to fail silently and/or just drop data rather than error. Essentially, preserve correctness. |
||
{ | ||
_writer.Write(s_builder.ToString()); | ||
_writer.Write(EventDelimiter); | ||
} | ||
} | ||
} | ||
catch | ||
{ | ||
// If we fail to log an event, just skip it and continue. | ||
} | ||
|
||
base.OnEventWritten(eventData); | ||
} | ||
} | ||
} |
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.
How did you choose which events to add this to?
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.
What I chose is based on two criteria:
If you think there are other events that should be included, I'm definitely open to that - just being careful with the perf overhead.
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.
Maybe RAROverallStart/Stop, BuildStart/Stop, and BuildProjectStart/Stop? I'd put those higher than LoadDocument, Parse, or MSBuildExe.
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.
Done. Thanks for the feedback on this.
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.
For command line scenarios I suspect Build and MSBuildExe will be fairly redundant: all MSBuild.exe does is start a build. But let's leave it in for v1.