Skip to content
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

Using consoleLogger in Telemetry client within MultithreadedAnlyzeCom… #1002

Merged
merged 4 commits into from
Aug 12, 2024
Merged

Using consoleLogger in Telemetry client within MultithreadedAnlyzeCom… #1002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3f42e99
Adding ConsoleLogger to a compiler data logger outputs
AllDwarf Jul 29, 2024
d8bc8cd
Add comment to clarify logger aggregation
AllDwarf Aug 6, 2024
da3bd09
Adding type check to avoid empty IAnalysisLogger in the first plase o…
AllDwarf Aug 7, 2024
a8a4d83
Add telemetry-related unit tests and using directives
AllDwarf Aug 9, 2024
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
1 change: 1 addition & 0 deletions ReleaseHistory.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
- NEW => new feature

## UNRELEASED
* BUG: Fork telemetry to log always to Console and AppInsights in the same time when Error occur. [1002](https://github.com/microsoft/binskim/pull/1002)

## **v4.3.0**
* DEP: Update `msdia140.dll` from 14.36.32532.0 to 14.40.33810.0. This update fixes the `System.AccessViolationException: Attempted to read or write protected memory` exception that occurs when reading certain PDB files. [996](https://github.com/microsoft/binskim/pull/996)
Expand Down
16 changes: 15 additions & 1 deletion src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
using System.Linq;
using System.Reflection;

using CommandLine;
Copy link
Member

Choose a reason for hiding this comment

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

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.


using Microsoft.CodeAnalysis.BinaryParsers;
using Microsoft.CodeAnalysis.IL.Rules;
using Microsoft.CodeAnalysis.IL.Sdk;
Expand Down Expand Up @@ -48,14 +50,26 @@

public override BinaryAnalyzerContext InitializeGlobalContextFromOptions(AnalyzeOptions options, ref BinaryAnalyzerContext context)
{
base.InitializeGlobalContextFromOptions(options, ref context);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@michaelcfanning just added the tests


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

Check warning

Code scanning / CodeQL

Dereferenced variable may be null Warning

Variable
aggregatingLogger
Loading
may be null at this access because of
this
Loading
assignment.
context.Logger = aggregatingLogger;
}

Expand All @@ -65,7 +79,7 @@
? options.MaxFileSizeInKilobytes.Value
: long.MaxValue;

base.InitializeGlobalContextFromOptions(options, ref context);


// Update context object based on command-line parameters.
context.SymbolPath = options.SymbolsPath ?? context.SymbolPath;
Expand Down
89 changes: 89 additions & 0 deletions src/Test.UnitTests.BinSkim.Driver/MultithreadedAnalyzeCommandTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,16 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Collections.Generic;
using System.Text;

using FluentAssertions;

using Microsoft.ApplicationInsights;
using Microsoft.ApplicationInsights.Extensibility;
using Microsoft.CodeAnalysis.IL;
using Microsoft.CodeAnalysis.IL.Sdk;
using Microsoft.CodeAnalysis.Sarif.Writers;

using Xunit;

Expand Down Expand Up @@ -150,5 +155,89 @@ private static string[] Shuffle(string[] data)

return data;
}

[Fact]
public void MultithreadedAnalyzeCommand_InitializeGlobalContextFromOptions_TelemetryNotSpecifiedHasOneConsoleLogger()
Copy link
Member

Choose a reason for hiding this comment

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

nice tests!

{
var options = new AnalyzeOptions();
options.TargetFileSpecifiers = new List<string> { "test.dll" };
options.DisableTelemetry = true;
options.OutputFilePath = "test/path/";
var context = new BinaryAnalyzerContext();

var command = new MultithreadedAnalyzeCommand();
command.InitializeGlobalContextFromOptions(options, ref context);

Assert.IsType<Sarif.Driver.AggregatingLogger>(context.Logger);

var aggregatingLogger = (Sarif.Driver.AggregatingLogger)context.Logger;
Assert.Contains(aggregatingLogger.Loggers, l => l is ConsoleLogger);
Assert.Equal(1, aggregatingLogger.Loggers.Count);
}

[Fact]
public void MultithreadedAnalyzeCommand_InitializeGlobalContextFromOptions_TelemetrySpecifiedHasTwoLoggers()
{
var telemetryConfiguration = TelemetryConfiguration.CreateDefault();
var telemetryClient = new TelemetryClient(telemetryConfiguration);
var telemetry = new Telemetry(telemetryConfiguration);

var options = new AnalyzeOptions();
options.TargetFileSpecifiers = new List<string> { "test.dll" };
options.OutputFilePath = "test/path/";
var context = new BinaryAnalyzerContext();

var command = new MultithreadedAnalyzeCommand(telemetry);
command.InitializeGlobalContextFromOptions(options, ref context);

Assert.IsType<Sarif.Driver.AggregatingLogger>(context.Logger);

var aggregatingLogger = (Sarif.Driver.AggregatingLogger)context.Logger;
Assert.Contains(aggregatingLogger.Loggers, l => l is ConsoleLogger);
Assert.Contains(aggregatingLogger.Loggers, l => l is RuleTelemetryLogger);
Assert.Equal(2, aggregatingLogger.Loggers.Count);
}

[Fact]
public void MultithreadedAnalyzeCommand_InitializeGlobalContextFromOptions_QuietOptionSetHasOneRuleTelemetryLogger()
{
var telemetryConfiguration = TelemetryConfiguration.CreateDefault();
var telemetryClient = new TelemetryClient(telemetryConfiguration);
var telemetry = new Telemetry(telemetryConfiguration);

var options = new AnalyzeOptions();
options.TargetFileSpecifiers = new List<string> { "test.dll" };
options.OutputFilePath = "test/path/";
options.Quiet = true;
var context = new BinaryAnalyzerContext();

var command = new MultithreadedAnalyzeCommand(telemetry);
command.InitializeGlobalContextFromOptions(options, ref context);

Assert.IsType<Sarif.Driver.AggregatingLogger>(context.Logger);

var aggregatingLogger = (Sarif.Driver.AggregatingLogger)context.Logger;
Assert.Contains(aggregatingLogger.Loggers, l => l is RuleTelemetryLogger);
Assert.Equal(1, aggregatingLogger.Loggers.Count);
}

[Fact]
public void MultithreadedAnalyzeCommand_InitializeGlobalContextFromOptions_QuietOptionSetAndNoTelemetryHasOneConsoleLogger()
{
var options = new AnalyzeOptions();
options.TargetFileSpecifiers = new List<string> { "test.dll" };
options.DisableTelemetry = true;
options.Quiet = true;
options.OutputFilePath = "test/path/";
var context = new BinaryAnalyzerContext();

var command = new MultithreadedAnalyzeCommand();
command.InitializeGlobalContextFromOptions(options, ref context);

Assert.IsType<Sarif.Driver.AggregatingLogger>(context.Logger);

var aggregatingLogger = (Sarif.Driver.AggregatingLogger)context.Logger;
Assert.Equal(0, aggregatingLogger.Loggers.Count);
}
}
}
Loading