Enhancing blame data collector options #1682

kaadhina · 2018-07-10T08:56:08Z

Description

Adding two more options in blame collector
DumpType: To be able to optionally get full dump (The default is mini dump)
AlwaysCollectDump: To be able to optionally generate dump even if all test cases complete (The default behaviour has been changed to generate dump only if any test cases has crashed)

Eg usage:
/blame:collectDump;DumpType=full;AlwaysCollectDump=true

karanjitsingh · 2018-07-10T09:33:27Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs

@@ -159,23 +167,26 @@ private void SessionEnded_Handler(object sender, SessionEndEventArgs args)

            if (this.processDumpEnabled)
            {
-                try
+                if (this.testStartCount > this.testEndCount || this.alwaysCollectProcessDump)


What if process throws before test cases start?

We have an option to collect dump no matter the number of test cases. This check is just the default behaviour.

cltshivash · 2018-07-11T08:37:24Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs

+            if (isFullDump)
+            {
+                // This will create a fulldump of the process with specified filename
+                return "-accepteula -t -ma " + processId + " " + filename + ".dmp";


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

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

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.

abhishkk · 2018-07-11T08:50:30Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs

@@ -30,6 +30,8 @@ public class BlameCollector : DataCollector, ITestExecutionEnvironmentSpecifier
        private int testStartCount;
        private int testEndCount;
        private bool processDumpEnabled;


we can rename this to processMiniDumpEnabled for readability

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

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.

abhishkk · 2018-07-11T08:54:31Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs

@@ -30,6 +30,8 @@ public class BlameCollector : DataCollector, ITestExecutionEnvironmentSpecifier
        private int testStartCount;
        private int testEndCount;
        private bool processDumpEnabled;
+        private bool alwaysCollectProcessDump;


If alwaysCollectProcessDump is false but collect dump is present, then we are creating dump on process crash. As we are creating dump, alwaysCollectProcessDump name is not incorrect. We can have name like collectDumpOnProcessAbort (or any other better name)

abhishkk · 2018-07-11T08:56:06Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Constants.cs

+        /// <summary>
+        /// Configuration key name for coolect dump always
+        /// </summary>
+        public const string CollectDumpAlwaysKey = "AlwaysCollectDump";


Same comment. CollectDumpOnProcessAbort/CollectDumpOnProcessCrash/CollectDumpOnProcessExit is more accurate.

abhishkk · 2018-07-11T08:57:41Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs

+                if (this.processDumpEnabled)
+                {
+                    this.alwaysCollectProcessDump = string.Equals(collectDumpNode.Attributes[Constants.CollectDumpAlwaysKey]?.Value, "true", StringComparison.OrdinalIgnoreCase);
+                    this.processFullDumpEnabled = string.Equals(collectDumpNode.Attributes[Constants.DumpTypeKey]?.Value, "full", StringComparison.OrdinalIgnoreCase);


We can move full keyword to const.

abhishkk · 2018-07-11T08:59:06Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Constants.cs

@@ -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 coolect dump always


abhishkk · 2018-07-11T10:08:09Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs

+                if (this.processDumpEnabled)
+                {
+                    this.alwaysCollectProcessDump = string.Equals(collectDumpNode.Attributes[Constants.CollectDumpAlwaysKey]?.Value, "true", StringComparison.OrdinalIgnoreCase);
+                    this.processFullDumpEnabled = string.Equals(collectDumpNode.Attributes[Constants.DumpTypeKey]?.Value, "full", StringComparison.OrdinalIgnoreCase);


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

abhishkk · 2018-07-11T10:14:40Z

src/vstest.console/Processors/EnableBlameArgumentProcessor.cs

@@ -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);


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?

abhishkk · 2018-07-11T10:17:55Z

src/vstest.console/Processors/EnableBlameArgumentProcessor.cs

@@ -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);


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

abhishkk · 2018-07-11T10:19:13Z

src/vstest.console/Processors/EnableBlameArgumentProcessor.cs

@@ -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);


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

abhishkk · 2018-07-11T10:19:37Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs

@@ -30,6 +30,8 @@ public class BlameCollector : DataCollector, ITestExecutionEnvironmentSpecifier
        private int testStartCount;
        private int testEndCount;
        private bool processDumpEnabled;
+        private bool alwaysCollectProcessDump;


Can we add a RFC for this as this is experience change?

singhsarab · 2018-07-11T13:04:24Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Constants.cs

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


Should we rename this as well ?

singhsarab · 2018-07-12T05:57:43Z

src/Microsoft.TestPlatform.ObjectModel/Constants.cs

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


Where is it being used ?

singhsarab · 2018-07-12T06:15:04Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs

+                this.processDumpEnabled = collectDumpNode != null;
+                if (this.processDumpEnabled)
+                {
+                    this.collectDumpOnProcessExit = string.Equals(collectDumpNode.Attributes[Constants.CollectDumpAlwaysKey]?.Value, "true", StringComparison.OrdinalIgnoreCase);


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

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

singhsarab · 2018-07-12T06:17:12Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs

@@ -30,6 +30,8 @@ public class BlameCollector : DataCollector, ITestExecutionEnvironmentSpecifier
        private int testStartCount;
        private int testEndCount;
        private bool processDumpEnabled;
+        private bool collectDumpOnProcessExit;


Add usage docs in help for /blame command.

abhishkk · 2018-07-25T06:49:47Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs

@@ -30,6 +30,8 @@ public class BlameCollector : DataCollector, ITestExecutionEnvironmentSpecifier
        private int testStartCount;
        private int testEndCount;
        private bool processDumpEnabled;


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.

abhishkk · 2018-07-25T06:55:25Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs

+                if (this.processDumpEnabled)
+                {
+                    this.collectDumpOnProcessExit = string.Equals(collectDumpNode.Attributes[Constants.CollectDumpOnProcessExitKey]?.Value, "true", StringComparison.OrdinalIgnoreCase);
+                    this.processFullDumpEnabled = string.Equals(collectDumpNode.Attributes[Constants.DumpTypeKey]?.Value, "full", StringComparison.OrdinalIgnoreCase);


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).

abhishkk · 2018-07-25T06:59:43Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs

@@ -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)


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.

renamed this as discussed

abhishkk · 2018-07-25T07:04:33Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcessDumpUtility.cs

+            if (isFullDump)
+            {
+                // This will create a fulldump of the process with specified filename
+                return "-accepteula -t -ma " + processId + " " + filename + ".dmp";


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.

deferred to next refactor in line

abhishkk · 2018-07-25T08:57:11Z

src/vstest.console/Processors/EnableBlameArgumentProcessor.cs

@@ -146,6 +151,10 @@ public void Initialize(string argument)
                    Output.Warning(false, CommandLineResources.BlameCollectDumpNotSupportedForPlatform);
                }
            }
+            else
+            {
+                Output.Warning(false, CommandLineResources.BlameIncorrectParameters);


RFC for the PR

abhishkk · 2018-07-25T08:59:51Z

src/vstest.console/Processors/EnableBlameArgumentProcessor.cs

@@ -194,13 +203,12 @@ public void Initialize(string argument)

            if (isDumpEnabled)
            {
-                var dumpNode = XmlDocument.CreateElement(Constants.BlameCollectDumpKey);
-                outernode.AppendChild(dumpNode);
+                AddCollectDumpNode(parameters, XmlDocument, outernode);


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

Which one gets preference?

Are we overriding the existing one?

abhishkk · 2018-07-25T09:03:17Z

src/vstest.console/Processors/EnableBlameArgumentProcessor.cs

+                    if (string.Equals(entry.Key, Constants.BlameCollectDumpOnProcessExitKey, StringComparison.OrdinalIgnoreCase))
+                    {
+                        attributeKey = Constants.BlameCollectDumpOnProcessExitKey;
+                        if(string.Equals(entry.Value, "true", StringComparison.OrdinalIgnoreCase) || string.Equals(entry.Value, "false", StringComparison.OrdinalIgnoreCase))


lets move value checks to blamecollector.

abhishkk · 2018-07-25T09:06:37Z

src/vstest.console/Processors/EnableBlameArgumentProcessor.cs

@@ -146,6 +151,10 @@ public void Initialize(string argument)
                    Output.Warning(false, CommandLineResources.BlameCollectDumpNotSupportedForPlatform);
                }
            }
+            else


in case of parsing error, lets throw the error.

abhishkk · 2018-07-25T09:08:24Z

test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameCollectorTests.cs

-                        null,
-                        null);
-                });
+            {


All kind of garbage value checks unit tests will come here.

abhishkk · 2018-07-25T09:10:45Z

test/vstest.console.UnitTests/Processors/EnableBlameArgumentProcessorTests.cs

@@ -106,6 +106,86 @@ public void InitializeShouldWarnIfPlatformNotSupportedForCollectDumpOption()
            }
        }

+        [TestMethod]


parsing related unit tests will come here.

abhishkk · 2018-07-27T09:09:58Z

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs

+                        else
+                        {
+                            EqtTrace.Warning("BlameCollector.SessionEnded_Handler: blame:CollectDump was enabled but dump file was not generated.");
+                            this.logger.LogWarning(args.Context, "BlameCollector.SessionEnded_Handler: blame:CollectDump was enabled but dump file was not generated.");


Logger warning localizable.

abhishkk · 2018-07-27T09:19:34Z

test/vstest.console.UnitTests/Processors/EnableBlameArgumentProcessorTests.cs

+
+        [TestMethod]
+        [ExpectedException(typeof(CommandLineException))]
+        public void InitializeShouldWarnIfInvalidParameterFormatIsSpecifiedForCollectDumpOption()


Nit: ShouldThrowError

kaadhina added 3 commits May 24, 2018 16:07

Merge remote-tracking branch 'refs/remotes/Microsoft/master'

3279dd3

Merge remote-tracking branch 'refs/remotes/Microsoft/master'

6120211

Proc dump changes

98dadc3

kaadhina requested review from cltshivash, singhsarab and abhishkk July 10, 2018 08:56

Merge branch 'master' into users/kavipriya/procdumpchanges

283cabe

singhsarab requested a review from karanjitsingh July 10, 2018 09:14

karanjitsingh reviewed Jul 10, 2018

View reviewed changes

cltshivash reviewed Jul 11, 2018

View reviewed changes

abhishkk reviewed Jul 11, 2018

View reviewed changes

PR comments

08d56e1

singhsarab reviewed Jul 12, 2018

View reviewed changes

smadala and others added 5 commits July 16, 2018 14:20

Merge branch 'master' into users/kavipriya/procdumpchanges

e0254ac

PR comments

6e33e89

Fixing help text

a2df30d

Help text

8570979

Making read on binary reader thread safe

15d72a1

abhishkk reviewed Jul 25, 2018

View reviewed changes

kaadhina added 2 commits July 26, 2018 16:46

Refactoring

5612d6a

Renaming parameter

407442f

abhishkk approved these changes Jul 27, 2018

View reviewed changes

abhishkk and others added 3 commits July 27, 2018 14:52

Merge branch 'master' into users/kavipriya/procdumpchanges

81d6b30

Minor fixes

56c701f

Fixing tests

186df0a

kaadhina merged commit 9f8fb1e into microsoft:master Jul 27, 2018

kaadhina changed the title ~~Users/kavipriya/procdumpchanges~~ Enhancing blame data collector options Jul 31, 2018

smadala mentioned this pull request Aug 2, 2018

vstest.console.exe hangs indefinitely when enabling datacollector in In-Assembly Parallel mode #1671

Closed

nohwnd mentioned this pull request May 13, 2020

Add hangdump and crash dump capabilities and options #2434

Merged

Enhancing blame data collector options #1682

Enhancing blame data collector options #1682

Conversation

kaadhina commented Jul 10, 2018

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment