Skip to content

Enhancing blame data collector options #1682

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.

Sign up for GitHub

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

Merged
kaadhina merged 15 commits into from
Jul 27, 2018
Merged

Enhancing blame data collector options #1682

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
3279dd3
Merge remote-tracking branch 'refs/remotes/Microsoft/master'
kaadhina May 24, 2018
6120211
Merge remote-tracking branch 'refs/remotes/Microsoft/master'
kaadhina Jul 10, 2018
98dadc3
Proc dump changes
kaadhina Jul 10, 2018
283cabe
Merge branch 'master' into users/kavipriya/procdumpchanges
kaadhina Jul 10, 2018
08d56e1
PR comments
kaadhina Jul 11, 2018
e0254ac
Merge branch 'master' into users/kavipriya/procdumpchanges
smadala Jul 16, 2018
6e33e89
PR comments
kaadhina Jul 16, 2018
a2df30d
Fixing help text
kaadhina Jul 16, 2018
8570979
Help text
kaadhina Jul 16, 2018
15d72a1
Making read on binary reader thread safe
kaadhina Jul 18, 2018
5612d6a
Refactoring
kaadhina Jul 26, 2018
407442f
Renaming parameter
kaadhina Jul 27, 2018
81d6b30
Merge branch 'master' into users/kavipriya/procdumpchanges
abhishkk Jul 27, 2018
56c701f
Minor fixes
kaadhina Jul 27, 2018
186df0a
Fixing tests
kaadhina Jul 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 27 additions & 13 deletions src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ public class BlameCollector : DataCollector, ITestExecutionEnvironmentSpecifier
private int testStartCount;
private int testEndCount;
private bool processDumpEnabled;
Copy link
Contributor

@abhishkk abhishkk Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can rename this to processMiniDumpEnabled for readability

Copy link
Contributor Author

@kaadhina kaadhina Jul 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be true irrespective of minidump or full dump is enabled. Code logic wise this is more readable. We can discuss if needed.

Copy link
Contributor

@abhishkk abhishkk Jul 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this makes more sense. But you are also using processFullDumpEnabled bariable to know whether full dump is enabled or not. Rather than processFullDumpEnabled it shoule take string or enum type 'full' or 'mini'. Today we are implictly deciding that if processFullDumpEnabled is false but processEnabled is true, then that means mini dump is enabled. This should be explicit.

private bool collectDumpOnProcessExit;
Copy link
Contributor

@singhsarab singhsarab Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add usage docs in help for /blame command.

Copy link
Contributor Author

@kaadhina kaadhina Jul 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

private bool processFullDumpEnabled;
private string attachmentGuid;

/// <summary>
Expand Down Expand Up @@ -97,7 +99,13 @@ public override void Initialize(

if (this.configurationElement != null)
{
this.processDumpEnabled = this.configurationElement[Constants.DumpModeKey] != null;
var collectDumpNode = this.configurationElement[Constants.DumpModeKey];
this.processDumpEnabled = collectDumpNode != null;
if (this.processDumpEnabled)
{
this.collectDumpOnProcessExit = string.Equals(collectDumpNode.Attributes[Constants.CollectDumpAlwaysKey]?.Value, "true", StringComparison.OrdinalIgnoreCase);
Copy link
Contributor

@singhsarab singhsarab Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's throw a warning for invalid argument. Bool should only be true/false

Copy link
Contributor

@singhsarab singhsarab Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add test for when it is explicitly set to false by user.

this.processFullDumpEnabled = string.Equals(collectDumpNode.Attributes[Constants.DumpTypeKey]?.Value, "full", StringComparison.OrdinalIgnoreCase);
Copy link
Contributor

@abhishkk abhishkk Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can move full keyword to const.

Copy link
Contributor

@abhishkk abhishkk Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mini should also be supported as value. Any invalid value should be thrown. We should mention a list of valid values in EnabledBlameProcessor and do the validation there itself. In future we can add any list value support in that as well. Soemthing like fullDumpWithThreadStack

Copy link
Contributor

@abhishkk abhishkk Jul 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case, collectDumpNode.Attributes[Constants.CollectDumpOnProcessExitKey] has value different than true OR collectDumpNode.Attributes[Constants.DumpTypeKey] has value different than full, we are enabling mini dump always (even when garbage values are present).

Copy link
Contributor Author

@kaadhina kaadhina Jul 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
}

this.attachmentGuid = Guid.NewGuid().ToString().Replace("-", string.Empty);
Expand Down Expand Up @@ -159,23 +167,29 @@ private void SessionEnded_Handler(object sender, SessionEndEventArgs args)

if (this.processDumpEnabled)
{
try
// If there was a test case crash or if we need to collect dump on process exit.
if (this.testStartCount > this.testEndCount || this.collectDumpOnProcessExit)
Copy link
Contributor

@abhishkk abhishkk Jul 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the understanding of collectDumpOnProcessExit is incorrect here.
Variable name collectDumpOnProcessUnexpectedExit makes more sense.

So today, when tests run is successful and there is no process unexpexted exit or crash, we are still creating the dump which doesn;t make much sense.

That's why we want a new feature called collectDumpOnProcessUnexpectedExit which if set, creates dump only and only when process exits/crashes unexpectedly.

In that case, this.testStartCount > this.testEndCount && this.collectDumpOnProcessExit will be the scenario for process unexpectde exit.

Copy link
Contributor Author

@kaadhina kaadhina Jul 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed this as discussed

{
var dumpFile = this.processDumpUtility.GetDumpFile();
if (!string.IsNullOrEmpty(dumpFile))
try
{
var fileTranferInformation = new FileTransferInformation(this.context.SessionDataCollectionContext, dumpFile, true);
this.dataCollectionSink.SendFileAsync(fileTranferInformation);
var dumpFile = this.processDumpUtility.GetDumpFile();
if (!string.IsNullOrEmpty(dumpFile))
{
var fileTranferInformation = new FileTransferInformation(this.context.SessionDataCollectionContext, dumpFile, true);
this.dataCollectionSink.SendFileAsync(fileTranferInformation);
}
else
{
EqtTrace.Warning("BlameCollector.SessionEnded_Handler: blame:CollectDump was enabled but dump file was not generated.");
Copy link
Contributor

@abhishkk abhishkk Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be logger warning as well as user was expecting a dump file but was not created.

Copy link
Contributor Author

@kaadhina kaadhina Jul 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

this.logger.LogWarning(args.Context, "BlameCollector.SessionEnded_Handler: blame:CollectDump was enabled but dump file was not generated.");
Copy link
Contributor

@abhishkk abhishkk Jul 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logger warning localizable.

}
}
else
catch (FileNotFoundException ex)
{
EqtTrace.Warning("BlameCollector.SessionEnded_Handler: blame:CollectDump was enabled but dump file was not generated.");
EqtTrace.Warning(ex.Message);
this.logger.LogWarning(args.Context, ex.Message);
Copy link
Contributor

@abhishkk abhishkk Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets put this in trace as well.

}
}
catch (FileNotFoundException ex)
{
this.logger.LogWarning(args.Context, ex.Message);
}
}

this.DeregisterEvents();
Expand All @@ -195,7 +209,7 @@ private void TestHostLaunched_Handler(object sender, TestHostLaunchedEventArgs a

try
{
this.processDumpUtility.StartProcessDump(args.TestHostProcessId, this.attachmentGuid, this.GetResultsDirectory());
this.processDumpUtility.StartProcessDump(args.TestHostProcessId, this.attachmentGuid, this.GetResultsDirectory(), this.processFullDumpEnabled);
}
catch (TestPlatformException e)
{
Expand Down
10 changes: 10 additions & 0 deletions src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Constants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,15 @@ internal static class Constants
/// Configuration key name for dump mode
/// </summary>
public const string DumpModeKey = "CollectDump";

/// <summary>
/// Configuration key name for collect dump always
/// </summary>
public const string CollectDumpAlwaysKey = "CollectDumpOnProcessExit";
Copy link
Contributor

@singhsarab singhsarab Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename this as well ?


/// <summary>
/// Configuration key name for dump type
/// </summary>
public const string DumpTypeKey = "DumpType";
}
}
5 changes: 4 additions & 1 deletion src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Interfaces/IProcessDumpUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ public interface IProcessDumpUtility
/// <param name="testResultsDirectory">
/// Path to TestResults directory
/// </param>
void StartProcessDump(int processId, string dumpFileGuid, string testResultsDirectory);
/// <param name="isFullDump">
/// Is full dump enabled
/// </param>
void StartProcessDump(int processId, string dumpFileGuid, string testResultsDirectory, bool isFullDump = false);
}
}
21 changes: 16 additions & 5 deletions src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,14 @@ public string GetDumpFile()
}

/// <inheritdoc/>
public void StartProcessDump(int processId, string dumpFileGuid, string testResultsDirectory)
public void StartProcessDump(int processId, string dumpFileGuid, string testResultsDirectory, bool isFullDump = false)
{
this.dumpFileName = $"{this.processHelper.GetProcessName(processId)}_{processId}_{dumpFileGuid}";
this.testResultsDirectory = testResultsDirectory;

this.procDumpProcess = this.processHelper.LaunchProcess(
this.GetProcDumpExecutable(),
ProcessDumpUtility.BuildProcDumpArgs(processId, this.dumpFileName),
ProcessDumpUtility.BuildProcDumpArgs(processId, this.dumpFileName, isFullDump),
testResultsDirectory,
null,
null,
Expand All @@ -94,13 +94,24 @@ public void StartProcessDump(int processId, string dumpFileGuid, string testResu
/// <param name="filename">
/// Filename for dump file
/// </param>
/// <param name="isFullDump">
/// Is full dump enabled
/// </param>
/// <returns>Arguments</returns>
private static string BuildProcDumpArgs(int processId, string filename)
private static string BuildProcDumpArgs(int processId, string filename, bool isFullDump = false)
{
// -accepteula: Auto accept end-user license agreement
// -t: Write a dump when the process terminates.
// This will create a minidump of the process with specified filename
return "-accepteula -t " + processId + " " + filename + ".dmp";
if (isFullDump)
{
// This will create a fulldump of the process with specified filename
return "-accepteula -t -ma " + processId + " " + filename + ".dmp";
Copy link
Contributor

@cltshivash cltshivash Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this args needs to change. Please check with @abhishkk .

Copy link
Contributor Author

@kaadhina kaadhina Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I'm doing this in next task. Current PR is just for the additional collector attributes

Copy link
Contributor

@abhishkk abhishkk Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When discussed args are added, we need to test it for all known crashes, for 32 bit and 64 bit process with 32 and 64 bit OS.

Copy link
Contributor Author

@kaadhina kaadhina Jul 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

Copy link
Contributor

@abhishkk abhishkk Jul 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than creating separate if else, we can create a stringbuilder and not add -ma in case its mini dump. This allows us to enahance this argument creation based on any new functionality.

Copy link
Contributor Author

@kaadhina kaadhina Jul 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deferred to next refactor in line

}
else
{
// This will create a minidump of the process with specified filename
return "-accepteula -t " + processId + " " + filename + ".dmp";
}
}

/// <summary>
Expand Down
5 changes: 5 additions & 0 deletions src/Microsoft.TestPlatform.ObjectModel/Constants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ public static class Constants
/// </summary>
public const string BlameCollectDumpKey = "CollectDump";

/// <summary>
/// Name of collect dump option for blame.
/// </summary>
public const string BlameCollectDumpOnlyOnAbortKey = "CollectDumpOnlyOnAbort";
Copy link
Contributor

@abhishkk abhishkk Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CollectDumpOnAbort seems more accurate.

Copy link
Contributor

@singhsarab singhsarab Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is it being used ?


/// <summary>
/// Name of data collection settings node in RunSettings.
/// </summary>
Expand Down
32 changes: 25 additions & 7 deletions src/vstest.console/Processors/EnableBlameArgumentProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,21 @@
namespace Microsoft.VisualStudio.TestPlatform.CommandLine.Processors
{
using System;
using System.Collections.Generic;
using System.Xml;

using CommandLineResources = Microsoft.VisualStudio.TestPlatform.CommandLine.Resources.Resources;
using Microsoft.VisualStudio.TestPlatform.CommandLine.Processors.Utilities;
using Microsoft.VisualStudio.TestPlatform.Common;
using Microsoft.VisualStudio.TestPlatform.Common.Interfaces;
using Microsoft.VisualStudio.TestPlatform.Common.Utilities;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Utilities;
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces;
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions;
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.Interfaces;
using Microsoft.VisualStudio.TestPlatform.Utilities;

using CommandLineResources = Microsoft.VisualStudio.TestPlatform.CommandLine.Resources.Resources;

internal class EnableBlameArgumentProcessor : IArgumentProcessor
{
/// <summary>
Expand All @@ -30,7 +33,7 @@ internal class EnableBlameArgumentProcessor : IArgumentProcessor
/// <summary>
/// Initializes a new instance of the <see cref="EnableBlameArgumentProcessor"/> class.
/// </summary>
public EnableBlameArgumentProcessor()
public EnableBlameArgumentProcessor()
{
}

Expand Down Expand Up @@ -133,7 +136,8 @@ public void Initialize(string argument)
{
bool isDumpEnabled = false;

if (!string.IsNullOrWhiteSpace(argument) && argument.Equals(Constants.BlameCollectDumpKey, StringComparison.OrdinalIgnoreCase))
var parseSucceeded = LoggerUtilities.TryParseLoggerArgument(argument, out string loggerIdentifier, out Dictionary<string, string> parameters);
Copy link
Contributor

@abhishkk abhishkk Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is blame argumetn. TryParseLoggerArgument should not be used to parse this. Not only its wrong from code per-se, its wrong from experience per-se because in case of any invalid arg, logger will throw its own error rather than of blame specific.

As underlying code of logger is generic, i have made this code common in this PR: #1681

You can use these changes once PR is checked in. I am trying to check it in by today or tomorrow.

Copy link
Contributor Author

@kaadhina kaadhina Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, please let me know once you push in

Copy link
Contributor

@abhishkk abhishkk Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it seems like now /blame:collect;DumpType=full is the way to collect full dump.
In platform terminology we use arg like /argName:argidentifier;key1=value1. This makes collect as identifier of blame arg which seem correct. @cltshivash @singhsarab @kaadhina Any suggestions here?

Copy link
Contributor

@abhishkk abhishkk Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to have validation here in case any invalid args, parameters passed. Check comment in UTs for detail.

Copy link
Contributor Author

@kaadhina kaadhina Jul 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

@abhishkk abhishkk Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update Blame usage text with these new key value pair info.

Copy link
Contributor Author

@kaadhina kaadhina Jul 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if (parseSucceeded && loggerIdentifier.Equals(Constants.BlameCollectDumpKey, StringComparison.OrdinalIgnoreCase))
{
if (this.environment.OperatingSystem == PlatformOperatingSystem.Windows &&
this.environment.Architecture != PlatformArchitecture.ARM64 &&
Expand Down Expand Up @@ -194,13 +198,12 @@ public void Initialize(string argument)

if (isDumpEnabled)
{
var dumpNode = XmlDocument.CreateElement(Constants.BlameCollectDumpKey);
outernode.AppendChild(dumpNode);
AddCollectDumpNode(parameters, XmlDocument, outernode);
Copy link
Contributor

@abhishkk abhishkk Jul 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if user gives CollectDump in runsettings and also gives /blame:collectDump;dumpType=full?

  1. Which one gets preference?
  2. Are we overriding the existing one?

}

foreach (var item in dataCollectionRunSettings.DataCollectorSettingsList)
{
if( item.FriendlyName.Equals(BlameFriendlyName))
if (item.FriendlyName.Equals(BlameFriendlyName))
{
item.Configuration = outernode;
}
Expand All @@ -209,6 +212,21 @@ public void Initialize(string argument)
runSettingsManager.UpdateRunSettingsNodeInnerXml(Constants.DataCollectionRunSettingsName, dataCollectionRunSettings.ToXml().InnerXml);
}

private static void AddCollectDumpNode(Dictionary<string, string> parameters, XmlDocument XmlDocument, XmlElement outernode)
{
var dumpNode = XmlDocument.CreateElement(Constants.BlameCollectDumpKey);
if (parameters != null && parameters.Count > 0)
{
foreach (KeyValuePair<string, string> entry in parameters)
{
var attribute = XmlDocument.CreateAttribute(entry.Key);
attribute.Value = entry.Value;
dumpNode.Attributes.Append(attribute);
}
}
outernode.AppendChild(dumpNode);
}

/// <summary>
/// Executes the argument processor.
/// </summary>
Expand Down
Loading