Adding trace level in diag argument #1681

abhishkk · 2018-07-09T11:46:29Z

Description

Currently \diag: arg always stores logs of verbose level. This PR adds customization in diag argument to allow users to pass trace level for logs.

Limitations

dotnet test command has --diag option. TraceLevel will not be supported in this --diag option.

mayankbansal018 · 2018-07-09T12:51:51Z

src/vstest.console/Processors/EnableDiagArgumentProcessor.cs

            if (string.IsNullOrWhiteSpace(argument))
            {
-                throw new CommandLineException(CommandLineResources.EnableDiagUsage);
+                HandleInvalidDiagArgument();


throw Exception here itself

mayankbansal018 · 2018-07-09T12:53:58Z

src/vstest.console/Processors/EnableDiagArgumentProcessor.cs

+        /// <returns>Diag argument list.</returns>
+        private string[] GetDiagArgumentList(string argument)
+        {
+            var argumentSeperator = new char[] { ';' };


Refactor / Reuse from Collect/Logger processors.

mayankbansal018 · 2018-07-09T13:08:56Z

src/testhost.x86/DefaultEngineInvoker.cs

@@ -228,7 +230,8 @@ private static void InitializeEqtTrace(IDictionary<string, string> argsDictionar
            // Setup logging if enabled
            if (argsDictionary.TryGetValue(LogFileArgument, out string logFile))
            {
-                EqtTrace.InitializeVerboseTrace(logFile);
+                var traceLevelInt = CommandLineArgumentsHelper.GetIntArgFromDict(argsDictionary, TraceLevelArgument);


verify for invalid tracelevel values,
Also verify vstest.console with older testhost

Added check for invalid tracelevel values
Verified with older testhost as well.

abhishkk · 2018-07-10T06:13:13Z

src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/ConsoleParameters.cs

+        /// <summary>
+        /// Trace level for logs.
+        /// </summary>
+        public PlatformTraceLevel TraceLevel { get; set; } = PlatformTraceLevel.Verbose;


Should I use System.Diagnostics.TraceLevel here OR Microsoft.VisualStudio.TestPlatform.ObjectModel.PlatformTraceLevel.

Internally we use PlatformTraceLevel wherever TraceLevel is not present. Example: NETSTANDARD1_4

PlatformTraceLevel is in PlatformAbstraction, this will mandate translationlayer users to have a reference to PlatformAbstraction as well.

If possible we should remove all the usage for System.Diagnostics.TraceLevel, rather have another TraceLevel enum in OM for this.

Removed dependency on platform abstractions.
Cant remove System.Diagnostics.TraceLevel as used by exe config trace level.

singhsarab · 2018-07-12T08:18:43Z

src/vstest.console/Processors/EnableDiagArgumentProcessor.cs

+            var diagArgumentList = GetDiagArgumentList(argument);
+
+            // Get diag file path.
+            // Note: Even though semi colon is valid file path, we are not respecting the file name having semi-colon [As we are separating arguments based on semi colon].


What is the behavior in that case, do we throw an error to the user ?

Yes we throw error to the user outputing diag arg usage.

singhsarab · 2018-07-12T10:26:45Z

src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/ConsoleParameters.cs

+        /// <summary>
+        /// Trace level for logs.
+        /// </summary>
+        public PlatformTraceLevel TraceLevel { get; set; } = PlatformTraceLevel.Verbose;


PlatformTraceLevel is in PlatformAbstraction, this will mandate translationlayer users to have a reference to PlatformAbstraction as well.

If possible we should remove all the usage for System.Diagnostics.TraceLevel, rather have another TraceLevel enum in OM for this.

singhsarab · 2018-07-12T10:29:01Z

src/datacollector/DataCollectorMain.cs

+                var traceLevelInt = CommandLineArgumentsHelper.GetIntArgFromDict(argsDictionary, TraceLevelArgument);
+
+                // In case traceLevelInt is not defined in PlatfromTraceLevel, default it to verbose.
+                var traceLevel = Enum.IsDefined(typeof(PlatformTraceLevel), traceLevelInt) ?


This is unexpected, I know it is a bit tricky, can we log a warning for this ?

singhsarab · 2018-07-12T10:30:16Z

src/vstest.console/Processors/EnableDiagArgumentProcessor.cs

+            var diagArgumentList = ArgumemtProcessorUtilities.GetArgumentList(argument, ArgumemtProcessorUtilities.SemiColonArgumentSeperator, CommandLineResources.EnableDiagUsage);
+
+            // Get diag file path.
+            // Note: Even though semi colon is valid file path, we are not respecting the file name having semi-colon [As we are separating arguments based on semi colon].


Warning for case file path contains ";" ?

We cant know if the semi colon is present in file path. we simply split the argument based on semi colon and the first argument is our file path

This means we will get some warning right ? I am expecting we wont be able to understand the string post ";" and should throw. Can you check ?

singhsarab · 2018-07-12T10:37:05Z

src/vstest.console/Processors/Utilities/ArgumemtProcessorUtilities.cs

+
+    internal class ArgumemtProcessorUtilities
+    {
+        public static readonly char[] SemiColonArgumentSeperator = { ';' };


Nit: Separator*

singhsarab · 2018-07-12T10:45:49Z

src/vstest.console/Processors/Utilities/ArgumemtProcessorUtilities.cs

+    using System;
+    using System.Collections.Generic;
+
+    internal class ArgumemtProcessorUtilities


Nit: Argument*

kaadhina · 2018-07-16T08:15:17Z

src/vstest.console/Processors/Utilities/ArgumemtProcessorUtilities.cs

+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+
+namespace Microsoft.VisualStudio.TestPlatform.CommandLine.Processors.Utilities


should this also handle validation of arguments based on a list? instead of just returning any value from the raw string?

We are handling the validation of argument here.

abhishkk · 2018-07-27T09:14:51Z

Include kaadhina PR and makes changes there as well.

singhsarab · 2018-08-01T07:45:04Z

src/Microsoft.TestPlatform.CoreUtilities/Tracing/EqtTrace.cs

+        /// Initializes the tracing with custom log file and trace level.
+        /// Overrides if any trace is set before.
+        /// </summary>
+        /// <param name="customLogFile">Customr log file for trace messages.</param>


singhsarab · 2018-08-01T07:49:20Z

src/datacollector/DataCollectorMain.cs

+                    (PlatformTraceLevel)traceLevelInt :
+                    PlatformTraceLevel.Verbose;
+
+                EqtTrace.Warning("DataCollectorMain.Run: Invalid trace level: {0}, defaulting to vebose tracelevel.", traceLevelInt);


This will log always. Have a proper if for the previous block.
Nit:verbose*

And without Initialize, will it work ?

Corrected. thanks

singhsarab · 2018-08-01T07:53:13Z

src/vstest.console/Processors/EnableBlameArgumentProcessor.cs

+        /// </summary>
+        /// <param name="collectDumpKey">Collect dump key.</param>
+        /// <returns>Flag for collect dump key valid or not.</returns>
+        private bool ValidateCollectDumpKey(string collectDumpKey)


I am confused. How is this part of this PR ?

We have made the parsing logic generic with my PR as per Mayank's PR comment. Thus blame, diag and logger have been changed to use the new generic logic.

These are just refactoring and not the code changes.

singhsarab · 2018-08-01T07:56:31Z

src/vstest.console/Processors/EnableDiagArgumentProcessor.cs

+            var diagArgumentList = ArgumemtProcessorUtilities.GetArgumentList(argument, ArgumemtProcessorUtilities.SemiColonArgumentSeperator, CommandLineResources.EnableDiagUsage);
+
+            // Get diag file path.
+            // Note: Even though semi colon is valid file path, we are not respecting the file name having semi-colon [As we are separating arguments based on semi colon].


This means we will get some warning right ? I am expecting we wont be able to understand the string post ";" and should throw. Can you check ?

singhsarab · 2018-08-01T07:58:03Z

src/vstest.console/Processors/EnableDiagArgumentProcessor.cs

+        }
+
+        /// <summary>
+        /// Initialize diag loggin.


Nit:logging

…to diagVerbosity

mayankbansal018

abhishkk added 4 commits July 6, 2018 19:16

Diag verbosity param support initial commit

56c3c67

Trace level changes

e188a59

Adding unit tests

6f77e35

resource correction

ed71fdf

abhishkk requested review from smadala, cltshivash and singhsarab July 9, 2018 11:46

mayankbansal018 reviewed Jul 9, 2018

View reviewed changes

review comments

fa521cb

abhishkk commented Jul 10, 2018

View reviewed changes

abhishkk added 3 commits July 10, 2018 15:05

review comments

371d522

Merge branch 'master' into diagVerbosity

4714b02

UT fix

090621e

abhishkk mentioned this pull request Jul 11, 2018

Enhancing blame data collector options #1682

Merged

singhsarab reviewed Jul 12, 2018

View reviewed changes

Merge branch 'master' into diagVerbosity

9bce75f

singhsarab reviewed Jul 13, 2018

View reviewed changes

kaadhina reviewed Jul 16, 2018

View reviewed changes

Merge branch 'master' into diagVerbosity

fa1f9ee

abhishkk added 4 commits July 30, 2018 11:58

Merge branch 'master' into diagVerbosity

7530406

PR comments fix

b5939f6

removing platform abstractions dependency from translation layer

47aa5b3

Merge branch 'master' into diagVerbosity

75700ef

singhsarab reviewed Aug 1, 2018

View reviewed changes

abhishkk added 2 commits August 1, 2018 14:32

PR comments

68490bb

Merge branch 'diagVerbosity' of https://github.com/abhishkk/vstest in…

17caaf9

…to diagVerbosity

singhsarab approved these changes Aug 1, 2018

View reviewed changes

mayankbansal018 approved these changes Aug 1, 2018

View reviewed changes

abhishkk merged commit 7fbfc61 into microsoft:master Aug 1, 2018

smadala mentioned this pull request Aug 9, 2018

Support all log level in --diag options #1461

Closed

		// Licensed under the MIT license. See LICENSE file in the project root for full license information.


		namespace Microsoft.VisualStudio.TestPlatform.CommandLine.Processors.Utilities

Adding trace level in diag argument #1681

Adding trace level in diag argument #1681

Conversation

abhishkk commented Jul 9, 2018 • edited Loading

Description

Limitations

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

mayankbansal018 Jul 9, 2018 • edited Loading

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

abhishkk commented Jul 27, 2018

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

mayankbansal018 left a comment

Choose a reason for hiding this comment

abhishkk commented Jul 9, 2018 •

edited

Loading

mayankbansal018 Jul 9, 2018 •

edited

Loading