Using consoleLogger in Telemetry client within MultithreadedAnlyzeCom…

[AllDwarf]

This pull request includes changes to the MultithreadedAnalyzeCommand class in src/BinSkim.Driver to enhance logging capabilities by adding a new console logger and ensuring proper namespaces are included.

Enhancements to logging:

• src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs: Added a ConsoleLogger to the AggregatingLogger to enable console logging during analysis.

Namespace inclusion:

• src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs: Included the Microsoft.CodeAnalysis.Sarif.Writers namespace to support the new logging functionality.

[AllDwarf]

Testing

Issue was tested with multiple setups

Setup

Running analyze command against empty folder wihout targets inside like this:

    analyze --config default --ignorePdbLoadError --hashes --statistics --output C:\Users\marekaldorf\source\repos\software-development-tools-samples\src\MyRun.sarif C:\Users\marekaldorf\source\repos\AllDwarf\BlobStorageTrigger\bin\*.so --log ForceOverwrite

You can change the path to point to any empty folder and the sarif you want and reproduce it the same way I did.

    analyze --config default --ignorePdbLoadError --hashes --statistics --output {PATH_TO_SARIF}\{NAME}.sarif {PATH_TO_EMPTY_FOLDER}\*.so --log ForceOverwrite

1. --disable-telemetry set to false when env variable BinskimAppInsightsConnectionString set to point to AppInsights with connection string set.
   1. BEFORE
      
   2. AFTER
      

[michaelcfanning]

        base.InitializeGlobalContextFromOptions(options, ref context);

try moving this line to the beginning of the method, perhaps the default console logging init will work then.

---

Refers to: src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs:68 in c1af956. [](commit_id = c1af956, deletion_comment = True)

[michaelcfanning]

InitializeGlobalContextFromOptions

btw! now seems a good time to talk a bit about testing. one thing I observed in the previous change is that you might have broken support for the --quiet flag. So you might run a test: revert to your previous change and run all tests: hopefully we see that some test protecting the functionality of the quiet switch fired.

I was only peripherally involved with the telemetry client work, unfortunately, so I don't know the state of testing.

This method is very testable, however, you can create this object and call this public method on it. So I would suggest creating some tests that hit this code path.

Verification looks straightforward, populate the object with a non-null Telemetry object, then call this init method and ensure the context.Loggers property contains what you think that it should.

[AllDwarf]

Yep good point. I tried it with --quiet true and I think seems to be working as expected no ConsoleLogger set, just telemetry is sent.

C:\Users\marekaldorf\source\repos\binskim\bld\bin\x64_Debug\netcoreapp3.1\BinSkim.exe (process 40088) exited with code 1.
To automatically close the console when debugging stops, enable Tools->Options->Debugging->Automatically close the console when debugging stops.
Press any key to close this window . . .

context.Logger in this case it is without ConsoleLogger. It has empty IAnalysisLogger. So in quiet mode this specific code will add one empty logger and one RuleTelemetryLogger together.

I tried to redo it a bit with something like this:

            if (this.Telemetry?.TelemetryClient != null)
            {
                // Create an aggregating logger that will combine all loggers into a single logger.
                var aggregatingLogger = new AggregatingLogger();
                if (context.Logger is AggregatingLogger)
                {
                    aggregatingLogger = context.Logger as AggregatingLogger;
                }
                else
                {
                    aggregatingLogger.Loggers.Add(context.Logger);
                }

                var ruleTelemetryLogger = new RuleTelemetryLogger(this.Telemetry.TelemetryClient);
                ruleTelemetryLogger.AnalysisStarted();

                // Combine rule telemetry with any other loggers that may be present.
                aggregatingLogger.Loggers.Add(ruleTelemetryLogger);
                context.Logger = aggregatingLogger;
            }

In this case we'll achieve non empty IAnalyseLogger in the first place but it's a bit hacky I would say with the type checks and such WDYT?

[AllDwarf]

just FYI, I'm working now on the test hopefully tomorrow I'll make them working :)

[AllDwarf]

@michaelcfanning just added the tests

[michaelcfanning]

CommandLine

necessary? btw - in other of our OSS projects we've figured out how to enforce certain style/other guidelines (like strictly removing unused namespace directives) that we could port here. the relationships between build, dotnet format, and .editorconfig are not obvious.

[michaelcfanning]

nice tests!

[michaelcfanning]