#2228 Create --kind and --level arguments for more granular control #2241

jameswinkler · 2021-01-14T18:33:14Z

jameswinkler · 2021-01-14T18:37:29Z

src/Sarif.Driver/Sdk/AnalyzeCommandBase.cs

@@ -267,13 +267,13 @@ internal AggregatingLogger InitializeLogger(AnalyzeOptionsBase analyzeOptions)

            if (!analyzeOptions.Quiet)
            {
-                _consoleLogger = new ConsoleLogger(analyzeOptions.Verbose, _tool.Driver.Name) { CaptureOutput = _captureConsoleOutput };
+                _consoleLogger = new ConsoleLogger(analyzeOptions.Verbose, _tool.Driver.Name, analyzeOptions.Level, analyzeOptions.Kind) { CaptureOutput = _captureConsoleOutput };


Maybe we should remove analyzeOptions.Verbose from the ConsoleLogger constructor signature, in preparation for that Verbose's eventual retirement? Instead, if verbose is passed, we could set Level and Kind to the maximum level. We do something similar with "options.ComputeFileHashes". We could also do the opposite with analyzeOptions.Quiet. #Resolved

…_base-logger

…ogger #2228 Remove verbose and largely replace it with "quiet"

…with --quiet. Correct some tests

michaelcfanning · 2021-01-18T18:45:52Z

src/Sarif/Writers/BaseLogger.cs

+            {
+                if (failureLevels.Contains(FailureLevel.Error))
+                {
+                    throw new ArgumentException("Invalid kind & level combination");


throw new ArgumentException("Invalid kind & level combination"); [](start = 20, length = 64)

This validation is not correct. It's fine to configure a mixture of things. The levels are only relevant if kind == Fail.

And so your actual validation here is if Kind does not include Fail, then Level must be zero. #Closed

Fixed.

In reply to: 559746343 [](ancestors = 559746343)

michaelcfanning · 2021-01-18T18:49:31Z

src/Sarif/Writers/CacheByFileHashLogger.cs

    {
        private bool cacheLoggingData;
        private string currentFileHash;

        public Dictionary<string, List<Notification>> HashToNotificationsMap { get; private set; }
        public Dictionary<string, List<Tuple<ReportingDescriptor, Result>>> HashToResultsMap { get; private set; }

-        public CacheByFileHashLogger(bool verbose)
+        public CacheByFileHashLogger(IEnumerable<FailureLevel> level, IEnumerable<ResultKind> kind) : base(level, kind)


level [](start = 63, length = 5)

s/be plural names #Closed

Fixed.

In reply to: 559747791 [](ancestors = 559747791)

michaelcfanning · 2021-01-18T18:53:51Z

src/Sarif/Writers/ConsoleLogger.cs

 using System.Globalization;
 using System.Linq;
 using System.Text;

 namespace Microsoft.CodeAnalysis.Sarif.Writers
 {
-    public class ConsoleLogger : IAnalysisLogger
+    public class ConsoleLogger : BaseLogger, IAnalysisLogger


ConsoleLogger [](start = 17, length = 13)

Note James: our console output should emit results in an ordered and deterministic way!
#Closed

Per our emails, created an issue here: #2250

In reply to: 559749330 [](ancestors = 559749330)

michaelcfanning · 2021-01-18T18:57:15Z

src/Sarif.Driver/Sdk/AnalyzeOptionsBase.cs

+            "level",
+            Separator = ';',
+            Default = new FailureLevel[] { FailureLevel.Error, FailureLevel.Warning },
+            HelpText = "Filter results. Valid values: None, Note, Warning, and Error.")]


Filter results. Valid values: None, Note, Warning, and Error. [](start = 24, length = 61)

"Filter output of scan results to one or more failure levels. Valid values: Error, Warning and Note." #Closed

Done.

In reply to: 559750572 [](ancestors = 559750572)

michaelcfanning · 2021-01-18T18:58:38Z

src/Sarif.Driver/Sdk/AnalyzeOptionsBase.cs

+            "kind",
+            Separator = ';',
+            Default = new ResultKind[] { ResultKind.Fail },
+            HelpText = "Filter results. Valid values: None, NotApplicable, Pass, Facil, Review, Open, and Informational")]


HelpText = "Filter results. Valid values: None, NotApplicable, Pass, Facil, Review, Open, and Informational")] [](start = 12, length = 110)

"Filter output one or more result kinds. Valid values: Fail (for literal scan results), Pass, Review, Open, NotApplicable and Informational." #Closed

Done.

In reply to: 559751125 [](ancestors = 559751125)

…ngOptions, and correct unit tests affected

…ogger #2228 Renaming, remove quiet from Logging Options

…ogger #2228 Some changes from code review

eddynaka · 2021-01-21T00:23:19Z

src/ReleaseHistory.md

+## **v2.4.0** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.4.0) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.4.0) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.4.0) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.4.0) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/2.4.0)
+* BREAKING: Entirely remove "verbose" whose fuctionality has been replaced by --level and --kind.
+* BREAKING: Rename "LoggingOptions" to "LogFilePersistenceOptions"
+* FEATURE: --quiet will now suppress all console messages except for errors.


nit: add the pr number and link just like the others, pls #Closed

Done.

In reply to: 561424247 [](ancestors = 561424247)

eddynaka · 2021-01-21T00:25:41Z

src/Sarif/Writers/CachingLogger.cs

-
-        public void LogMessage(bool verbose, string message)
-        {
+            if (ShouldLog(notification))


if (ShouldLog(notification)) [](start = 12, length = 28)

nit: just following the pattern, can u do if (!shouldLog) -> return, so all places would be the same?
Apply this for the method below #Closed

Done.

In reply to: 561426472 [](ancestors = 561426472)

eddynaka · 2021-01-21T00:26:18Z

src/Sarif/Writers/ConsoleLogger.cs

    {
-        public ConsoleLogger(bool verbose, string toolName)
+        public ConsoleLogger(bool quietConsole, string toolName, IEnumerable<FailureLevel> level = null, IEnumerable<ResultKind> kind = null) : base(level, kind)


level [](start = 91, length = 5)

rename to levels and kinds, just like the other places #Closed

Done.

In reply to: 561427077 [](ancestors = 561427077)

eddynaka · 2021-01-21T00:26:38Z

src/Sarif/Writers/CachingLogger.cs

    {
+        public CachingLogger(IEnumerable<FailureLevel> level, IEnumerable<ResultKind> kind) : base(level, kind)


level [](start = 55, length = 5)

rename to levels and kinds #Closed

Done.

In reply to: 561427374 [](ancestors = 561427374)

eddynaka · 2021-01-21T00:28:09Z

src/Sarif/Writers/ConsoleLogger.cs

@@ -238,12 +177,18 @@ public static string NormalizeMessage(string message, bool enquote)

        public void LogToolNotification(Notification notification)
        {
-            WriteToConsole(notification);
+            if (ShouldLog(notification))


ShouldLog [](start = 16, length = 9)

nit: invert condition just like the other places #Closed

Done.

In reply to: 561428877 [](ancestors = 561428877)

eddynaka · 2021-01-21T00:28:49Z

src/Sarif/Writers/LogFilePersistenceOptions.cs

@@ -6,22 +6,20 @@
 namespace Microsoft.CodeAnalysis.Sarif.Writers
 {
    [Flags]
-    public enum LoggingOptions
+    public enum LogFilePersistenceOptions
+


remove empty line #Closed

Done.

In reply to: 561429474 [](ancestors = 561429474)

eddynaka · 2021-01-21T00:29:49Z

src/Sarif/Writers/SarifLogger.cs

            OptionallyEmittedData dataToInsert = OptionallyEmittedData.None,
            OptionallyEmittedData dataToRemove = OptionallyEmittedData.None,
            Tool tool = null,
            Run run = null,
            IEnumerable<string> analysisTargets = null,
            IEnumerable<string> invocationTokensToRedact = null,
            IEnumerable<string> invocationPropertiesToLog = null,
-            string defaultFileEncoding = null)
+            string defaultFileEncoding = null,
+            IEnumerable<FailureLevel> level = null,


level [](start = 38, length = 5)

nit: rename to levels and kinds #Closed

Done.

In reply to: 561430439 [](ancestors = 561430439)

eddynaka

eddynaka · 2021-01-21T16:45:23Z

src/Sarif/Writers/SarifLogger.cs

+            TextWriter textWriter,
+            LogFilePersistenceOptions logFilePersistenceOptions,
+            bool closeWriterOnDipose,
+            IEnumerable<FailureLevel> level,


level [](start = 38, length = 5)

nit: levels and kinds #Resolved

Done.

In reply to: 562032468 [](ancestors = 562032468)

eddynaka · 2021-01-21T16:46:20Z

src/Sarif/Writers/SarifOneZeroZeroLogger.cs

            OptionallyEmittedData dataToInsert = OptionallyEmittedData.None,
            OptionallyEmittedData dataToRemove = OptionallyEmittedData.None,
            Tool tool = null,
            Run run = null,
            IEnumerable<string> analysisTargets = null,
            IEnumerable<string> invocationTokensToRedact = null,
            IEnumerable<string> invocationPropertiesToLog = null,
-            string defaultFileEncoding = null)
+            string defaultFileEncoding = null,
+            IEnumerable<FailureLevel> level = null,


level [](start = 38, length = 5)

nit: levels and kinds #Resolved

Done.

In reply to: 562033198 [](ancestors = 562033198)

michaelcfanning · 2021-01-22T22:47:23Z

src/Sarif/Writers/BaseLogger.cs

+                return _failureLevels.Contains(result.Level);
+            }
+
+            return _resultKinds.Contains(result.Kind) && _failureLevels.Contains(result.Level);


return _resultKinds.Contains(result.Kind) && _failureLevels.Contains(result.Level); [](start = 12, length = 83)

If you have this check, you must always populate failure levels with 'None' to accommodate kinds, like 'Open' that never set a failure level (because a level only applies to kind 'Fail')

This code would be much cleaner if you first checked the kind. And then, IFF kind == Fail, check the failure levels.

In reply to: 562957160 [](ancestors = 562957160)

michaelcfanning · 2021-01-22T22:50:58Z

src/Sarif/Writers/BaseLogger.cs

+            //  If resultKinds does not include "fail" then failureLevels MUST be none/zero
+            if (!_resultKinds.Contains(ResultKind.Fail))
+            {
+                if (_failureLevels.Count > 1 || _failureLevels[0] != FailureLevel.None)


_failureLevels[0] != FailureLevel.None [](start = 48, length = 38)

people should never specify None. #Closed

As discussed, this is redundant but not bad input.

In reply to: 562958403 [](ancestors = 562958403)

michaelcfanning · 2021-01-22T22:53:03Z

src/Test.UnitTests.Sarif/Writers/BaseLoggerTests.cs

+
+namespace Microsoft.CodeAnalysis.Test.UnitTests.Sarif.Writers
+{
+    public class BaseLoggerTests


BaseLoggerTests [](start = 17, length = 15)

These tests are comprehensive enough. Please add a test for this user configuration:

--kind Open;Fail --level Error

Your code currently won't work for this combination (which should allow through any Open or Fail result (but only Fail messages of level Error)

michaelcfanning · 2021-01-22T22:54:18Z

src/Sarif.Driver/Sdk/AnalyzeCommandBase.cs


            if (!succeeded)
            {
+                //  TODO: This seems like uninformative error output.  All these errors get squished into one generic message
+                //  whenever something goes wrong.


File a GH issue and add the link to this comment.

michaelcfanning

🕐

* Change validation in BaseLogger and correct unit tests and code broken as a result * Correct formatting * Test SarifLogger honors Kind/Level * Refactor, remove redundant code

eddynaka and others added 5 commits January 12, 2021 14:57

Creating BaseLogger to share methods

d8fbaee

removing logmessage from interface

dd2a148

updating more loggers

5bccb3d

Merge branch 'main' into users/ednakamu/base-logger

ceb56ae

Fix parameters

c52e91a

jameswinkler commented Jan 14, 2021

View reviewed changes

jameswinkler and others added 16 commits January 14, 2021 16:07

Replace verbose in ConsoleLogger. It should know about quiet instead

1b16fb8

Merge branch 'main' into users/v-jwinkler/#2228_base-logger

ec37c1e

Remove verbose

c3d8e91

Merge main -> users/ednakamu/base-logger

2e07ac5

Remove unused method

d8fc5e4

Merge branch 'users/ednakamu/base-logger' into users/v-jwinkler/#2228…

35247a6

…_base-logger

Remove unnecessary nullcheck

e54cf2c

fixing functional tests

de5f2b9

back to quiet

81fa57e

fixing tests

662c140

Use ShouldLog in more locations

af84726

Add to release history for breaking change

0f12856

Merge pull request #2246 from microsoft/users/v-jwinkler/#2228_base-l…

3d7fab4

…ogger #2228 Remove verbose and largely replace it with "quiet"

Fix formatting

3092114

Make more loggers children of BaseLogger. Cleanup some functionality …

f068b44

…with --quiet. Correct some tests

fixing spacing

cd8a99f

michaelcfanning reviewed Jan 18, 2021

View reviewed changes

jameswinkler and others added 3 commits January 18, 2021 14:20

Rename some parameters, classes, and files. Remove "quiet" from Loggi…

130b1fa

…ngOptions, and correct unit tests affected

More complete renaming

508ae6b

Merge pull request #2247 from microsoft/users/v-jwinkler/#2228_base-l…

0b081e3

…ogger #2228 Renaming, remove quiet from Logging Options

jameswinkler and others added 6 commits January 19, 2021 11:40

Add unit tests for DriverExtensionMethod and BaseLogger

b2ad7af

Fix casting error

7440769

fixing samples compilation issue

2b53283

Small changes to address comments

ba0c58b

Merge pull request #2249 from microsoft/users/v-jwinkler/#2228_base-l…

908e5ae

…ogger #2228 Some changes from code review

Formatting fixes

a49b35d

jameswinkler marked this pull request as ready for review January 20, 2021 18:51

jameswinkler requested a review from eddynaka as a code owner January 20, 2021 18:51

eddynaka reviewed Jan 21, 2021

View reviewed changes

eddynaka approved these changes Jan 21, 2021

View reviewed changes

Address renaming and refactoring comments

66e51f2

eddynaka reviewed Jan 21, 2021

View reviewed changes

jameswinkler and others added 2 commits January 21, 2021 09:24

More renaming

1720e89

Merge branch 'main' into users/ednakamu/base-logger

0f0bee9

michaelcfanning reviewed Jan 22, 2021

View reviewed changes

michaelcfanning requested changes Jan 22, 2021

View reviewed changes

jameswinkler and others added 2 commits January 25, 2021 18:45

Change validation in BaseLogger and correct unit tests and code (#2262)

1a1c3fe

* Change validation in BaseLogger and correct unit tests and code broken as a result * Correct formatting * Test SarifLogger honors Kind/Level * Refactor, remove redundant code

Merge branch 'main' into users/ednakamu/base-logger

ea67391

eddynaka merged commit 6c931ba into main Jan 25, 2021

eddynaka deleted the users/ednakamu/base-logger branch January 25, 2021 22:02

		{
		public CachingLogger(IEnumerable<FailureLevel> level, IEnumerable<ResultKind> kind) : base(level, kind)

#2228 Create --kind and --level arguments for more granular control #2241

#2228 Create --kind and --level arguments for more granular control #2241

Conversation

jameswinkler commented Jan 14, 2021

jameswinkler Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

michaelcfanning Jan 18, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 18, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 18, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 18, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 18, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka left a comment

Choose a reason for hiding this comment

eddynaka Jan 21, 2021 • edited by jameswinkler Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jan 21, 2021 • edited by jameswinkler 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

michaelcfanning Jan 22, 2021 • edited by jameswinkler 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

michaelcfanning left a comment

Choose a reason for hiding this comment

jameswinkler Jan 14, 2021 •

edited

Loading

michaelcfanning Jan 18, 2021 •

edited by eddynaka

Loading

michaelcfanning Jan 18, 2021 •

edited by eddynaka

Loading

michaelcfanning Jan 18, 2021 •

edited by eddynaka

Loading

michaelcfanning Jan 18, 2021 •

edited by eddynaka

Loading

michaelcfanning Jan 18, 2021 •

edited by eddynaka

Loading

eddynaka Jan 21, 2021 •

edited

Loading

eddynaka Jan 21, 2021 •

edited

Loading

eddynaka Jan 21, 2021 •

edited

Loading

eddynaka Jan 21, 2021 •

edited

Loading

eddynaka Jan 21, 2021 •

edited

Loading

eddynaka Jan 21, 2021 •

edited

Loading

eddynaka Jan 21, 2021 •

edited

Loading

eddynaka Jan 21, 2021 •

edited by jameswinkler

Loading

eddynaka Jan 21, 2021 •

edited by jameswinkler

Loading

michaelcfanning Jan 22, 2021 •

edited by jameswinkler

Loading