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

Create one session/telemetry configuration per run #515

Merged
merged 7 commits into from
Nov 10, 2021
Merged

Create one session/telemetry configuration per run #515

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a837219
Create one session per run
eddynaka Nov 5, 2021
ae1548b
updating tests
eddynaka Nov 6, 2021
aef6a64
Validating logged hash
eddynaka Nov 6, 2021
e119e62
removing redundant call to tostring
eddynaka Nov 6, 2021
9099778
Merge branch 'main' into users/ednakamu/create-one-session-per-run
eddynaka Nov 9, 2021
da02bdc
Addressing PR feedback
eddynaka Nov 9, 2021
df00214
deleting nuget.config
eddynaka Nov 9, 2021
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
54 changes: 36 additions & 18 deletions src/BinSkim.Sdk/CompilerDataLogger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,19 @@ public class CompilerDataLogger
private const string CommandLineEventName = "CommandLineInformation";
private const string AssemblyReferencesEventName = "AssemblyReferencesInformation";

private readonly int ChunkSize;
private readonly string sha256;
private string Sha256 => context?.Hashes?.Sha256;
Copy link
Contributor Author

@eddynaka eddynaka Nov 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

context?.Hashes?.Sha256

since we changed how hashes are generated, we should make a reference to the context so we will have the correct value of the hash when needed. #ByDesign

Copy link
Member

@michaelcfanning michaelcfanning Nov 9, 2021 •
edited by eddynaka
Loading

Choose a reason for hiding this comment

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

context

the code previously seemed to return string.empty in the null case, is that still required or is it ok to drop it? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when we send a null to AppInsights, it won't render the key value pair. If we try to print it in console, a null is showed as an empty string.

so, in this case, we don't need to create a default to empty


private readonly int chunkSize;
private readonly string relativeFilePath;
Copy link
Member

Choose a reason for hiding this comment

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

relativeFilePath

just retrieve this from the context as you do hashes

private readonly IAnalysisContext context;

private static string s_sessionId;
internal static string s_sessionId;
Copy link
Member

@michaelcfanning michaelcfanning Nov 9, 2021 •
edited by eddynaka
Loading

Choose a reason for hiding this comment

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

internal

please pull aside all the internal static properties (that are overridden by tests) and group them together.

we do this to improve the readability of code, i.e., that accessibility is a clue to the maintainer there is a special external caller (our tests) that update these values.

make sense? you are future-proofing this code to help whoever comes along when those tests fail. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!
thanks for the feedback!

private static bool s_printHeader = true;
Copy link
Member

Choose a reason for hiding this comment

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

private static bool s_printHeader = true;

add private static ctor, all static initialization is explicit in a common place.

private static TelemetryClient s_telemetryClient;
private static TelemetryConfiguration s_telemetryConfiguration;

private static readonly object s_syncRoot = new object();

public static bool TelemetryEnabled => s_telemetryClient != null;

public CompilerDataLogger(IAnalysisContext analysisContext,
Expand All @@ -37,10 +41,10 @@ public CompilerDataLogger(IAnalysisContext analysisContext,
TelemetryClient telemetryClient = null,
int chunkSize = 8192)
{
s_telemetryConfiguration = telemetryConfiguration;
s_telemetryClient = telemetryClient;
s_sessionId = TelemetryEnabled ? Guid.NewGuid().ToString() : null;
ChunkSize = chunkSize;
s_telemetryConfiguration ??= telemetryConfiguration;
Copy link
Contributor Author

@eddynaka eddynaka Nov 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

telemetryConfiguration

we should only use telemetryConfiguration/telemetryClient if the respective static value is null. This should only be used during tests. #ByDesign

Copy link
Member

Choose a reason for hiding this comment

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

s_telemetryConfiguration ??= telemetryConfiguration

ensure both are null or non-null in the ctor.

s_telemetryClient ??= telemetryClient;
s_sessionId ??= Guid.NewGuid().ToString();
this.chunkSize = chunkSize;

if (!TelemetryEnabled)
{
Expand All @@ -58,7 +62,7 @@ public CompilerDataLogger(IAnalysisContext analysisContext,
}
}

this.sha256 = analysisContext?.Hashes?.Sha256 ?? string.Empty;
this.context = analysisContext;
this.relativeFilePath = analysisContext?.TargetUri?.LocalPath ?? string.Empty;

foreach (string path in targetFileSpecifiers)
Expand All @@ -77,9 +81,15 @@ public static void Initialize(string instrumentationKey)
{
if (s_telemetryConfiguration == null && s_telemetryClient == null)
{
s_sessionId = Guid.NewGuid().ToString();
s_telemetryConfiguration = new TelemetryConfiguration(instrumentationKey);
s_telemetryClient = new TelemetryClient(s_telemetryConfiguration);
lock (s_syncRoot)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

when we changed from single thread to multi thread, we started to create multiple configurations/clients/sessions.

With that in mind, we must synchronize the threads to prevent this behavior.

{
if (s_telemetryConfiguration == null && s_telemetryClient == null)
Copy link
Member

Choose a reason for hiding this comment

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

s_telemetryClient

I don't understand the visibility of things in your class, why is s_sessionId internal but s_telemetry not?

Why is it ok to allow s_sessionId to be overridden by tests but then overwritten again if these two values are null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

session/configuration/client can be injected using the constructor but the session is always defined when we have the telemetry enabled.

It's why we don't need to make the configuration/client internal.

{
s_sessionId = Guid.NewGuid().ToString();
s_telemetryConfiguration = new TelemetryConfiguration(instrumentationKey);
s_telemetryClient = new TelemetryClient(s_telemetryConfiguration);
}
}
}
}

Expand Down Expand Up @@ -152,7 +162,7 @@ public void Write(CompilerData compilerData)
{ "moduleName", compilerData.ModuleName ?? string.Empty },
{ "moduleLibrary", (compilerData.ModuleName == compilerData.ModuleLibrary ? string.Empty : compilerData.ModuleLibrary ?? string.Empty) },
{ "sessionId", s_sessionId },
{ "hash", this.sha256 },
{ "hash", this.Sha256 },
{ "error", string.Empty }
};

Expand Down Expand Up @@ -183,7 +193,7 @@ public void Write(CompilerData compilerData)
}
else
{
string log = $"{this.relativeFilePath},{compilerData},{this.sha256},";
string log = $"{this.relativeFilePath},{compilerData},{this.Sha256},";
Console.WriteLine(log);
}
}
Expand All @@ -208,13 +218,13 @@ public void WriteException(string errorMessage)
{ "moduleName", string.Empty },
{ "moduleLibrary", string.Empty },
{ "sessionId", s_sessionId },
{ "hash", this.sha256 },
{ "hash", this.Sha256 },
{ "error", errorMessage },
});
}
else
{
string log = $"{this.relativeFilePath},,,,,,,,,,,,,{this.sha256},{errorMessage}";
string log = $"{this.relativeFilePath},,,,,,,,,,,,,{this.Sha256},{errorMessage}";
Console.WriteLine(log);
}
}
Expand Down Expand Up @@ -268,13 +278,20 @@ public static void Summarize(AnalysisSummary summary)
}
}

internal static void Reset()
{
s_sessionId = null;
Copy link
Member

@michaelcfanning michaelcfanning Nov 9, 2021 •
edited by eddynaka
Loading

Choose a reason for hiding this comment

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

s_sessionId

why don't you lock here? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some docs in this method. We don't need a lock because it will only be called during tests.
All the tests that requires this aren't running in parallel.

s_telemetryClient = null;
s_telemetryConfiguration = null;
}

private void SendChunkedContent(string eventName, string contentId, string contentName, string content)
{
int j = 1;
int size = (int)Math.Ceiling(1.0 * content.Length / ChunkSize);
for (int i = 0; i < content.Length; i += ChunkSize)
int size = (int)Math.Ceiling(1.0 * content.Length / this.chunkSize);
for (int i = 0; i < content.Length; i += this.chunkSize)
{
string chunckedContent = content.Substring(i, Math.Min(ChunkSize, content.Length - i));
string chunckedContent = content.Substring(i, Math.Min(this.chunkSize, content.Length - i));

s_telemetryClient.TrackEvent(eventName, properties: new Dictionary<string, string>
{
Expand All @@ -284,6 +301,7 @@ private void SendChunkedContent(string eventName, string contentId, string conte
{ "totalNumber", size.ToString() },
{ $"chunked{contentName}", chunckedContent },
});

j++;
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/BinSkim.Sdk/Properties/AssemblyInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Reflection;
using System.Runtime.CompilerServices;

[assembly: AssemblyTitle("BinSkim SDK")]
[assembly: AssemblyDescription("BinSkim SDK and utilities for authoring analysis checks")]

[assembly: InternalsVisibleTo("Test.UnitTests.BinSkim.Driver")]
Copy link
Member

@michaelcfanning michaelcfanning Nov 9, 2021 •
edited by eddynaka
Loading

Choose a reason for hiding this comment

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

UnitTests

you added this to the wrong file, it should be in Shared\AssemblyInfo.cs #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!
thanks!

105 changes: 78 additions & 27 deletions src/Test.UnitTests.BinSkim.Driver/CompilerDataLoggerUnitTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

using System;
using System.Collections.Generic;
using System.Text;
using System.Threading.Tasks;

using FluentAssertions;

Expand All @@ -18,54 +18,105 @@ namespace Microsoft.CodeAnalysis.BinSkim.Rules
{
public class CompilerDataLoggerUnitTests
{
private TelemetryConfiguration telemetryConfiguration;
private TelemetryClient telemetryClient;
private List<ITelemetry> sendItems;

private void TestSetup()
{
if (this.telemetryConfiguration == null && this.telemetryClient == null)
{
this.telemetryConfiguration = new TelemetryConfiguration();
this.sendItems = new List<ITelemetry>();
this.telemetryConfiguration.TelemetryChannel = new StubTelemetryChannel { OnSend = item => this.sendItems.Add(item) };
this.telemetryConfiguration.InstrumentationKey = Guid.NewGuid().ToString();
this.telemetryConfiguration.TelemetryInitializers.Add(new OperationCorrelationTelemetryInitializer());
this.telemetryClient = new TelemetryClient(this.telemetryConfiguration);
}
}

[Fact]
public void CompilerDataLogger_Write_ShouldSendAssemblyReferencesInChunks()
public void CompilerDataLogger_Write_ShouldSendAssemblyReferencesInChunks_WhenTelemetryIsEnabled()
{
this.TestSetup();
(TelemetryConfiguration, TelemetryClient, List<ITelemetry>) setup = TestSetup();

var context = new BinaryAnalyzerContext { };
var context = new BinaryAnalyzerContext();
string[] targetFileSpecifier = new[] { @"E:\applications\Tool\*.exe" };
int chunksize = 10;
string assemblies = "Microsoft.DiaSymReader (1.3.0);Newtonsoft.Json (13.0.1)";
int chunkNumber = (assemblies.Length + chunksize - 1) / chunksize;

CompilerDataLogger logger = new CompilerDataLogger(context, targetFileSpecifier, this.telemetryConfiguration, this.telemetryClient, chunksize);
var logger = new CompilerDataLogger(context, targetFileSpecifier, setup.Item1, setup.Item2, chunksize);
logger.Write(new CompilerData { CompilerName = ".NET Compiler", AssemblyReferences = assemblies });

this.sendItems.Count.Should().Be(chunkNumber + 1); // first item should be CompilerInformation
setup.Item3.Count.Should().Be(chunkNumber + 1); // first item should be CompilerInformation
CompilerDataLogger.Reset();
}

[Fact]
public void CompilerDataLogger_Write_ShouldNotSend_IfNoAssemblyReferences()
{
this.TestSetup();
(TelemetryConfiguration, TelemetryClient, List<ITelemetry>) setup = TestSetup();

var context = new BinaryAnalyzerContext { };
var context = new BinaryAnalyzerContext();
string[] targetFileSpecifier = new[] { @"E:\applications\Tool\*.exe" };
int chunksize = 10;
string assemblies = null;

CompilerDataLogger logger = new CompilerDataLogger(context, targetFileSpecifier, this.telemetryConfiguration, this.telemetryClient, chunksize);
var logger = new CompilerDataLogger(context, targetFileSpecifier, setup.Item1, setup.Item2, chunksize);
logger.Write(new CompilerData { CompilerName = ".NET Compiler", AssemblyReferences = assemblies });

this.sendItems.Count.Should().Be(1); // first item should be CompilerInformation
setup.Item3.Count.Should().Be(1); // first item should be CompilerInformation
CompilerDataLogger.Reset();
}

[Fact]
public void CompilerDataLogger_ShouldGenerateOnlyOneSessionPerRun()
{
string sessionId = string.Empty;
int numberOfCompilerDataLoggers = 1000;
var context = new BinaryAnalyzerContext();
string[] targetFileSpecifier = new[] { @"E:\applications\Tool\*.exe" };

Environment.SetEnvironmentVariable("BinskimAppInsightsKey", Guid.NewGuid().ToString());

Parallel.For(0, numberOfCompilerDataLoggers, _ =>
{
var compilerDataLogger = new CompilerDataLogger(context, targetFileSpecifier);
if (string.IsNullOrEmpty(sessionId))
{
sessionId = CompilerDataLogger.s_sessionId;
}

sessionId.Should().Be(CompilerDataLogger.s_sessionId);
});

CompilerDataLogger.Reset();
}

[Fact]
public void CompilerDataLogger_SessionIdShouldNotBeReplacedWhenValid()
{
var context = new BinaryAnalyzerContext();
string[] targetFileSpecifier = new[] { @"E:\applications\Tool\*.exe" };

_ = new CompilerDataLogger(context, targetFileSpecifier);
CompilerDataLogger.s_sessionId.Should().NotBeNullOrWhiteSpace();

string currentGuid = Guid.NewGuid().ToString();
Environment.SetEnvironmentVariable("BinskimAppInsightsKey", currentGuid);

// SessionId will be created when BinskimAppInsightsKey is retrieved.
_ = new CompilerDataLogger(context, targetFileSpecifier);
CompilerDataLogger.s_sessionId.Should().Be(currentGuid);

// Since sessionId is not null, no new session should be created.
_ = new CompilerDataLogger(context, targetFileSpecifier);
CompilerDataLogger.s_sessionId.Should().Be(currentGuid);

CompilerDataLogger.Reset();
}

private (TelemetryConfiguration, TelemetryClient, List<ITelemetry>) TestSetup()
{
List<ITelemetry> sendItems = null;
TelemetryClient telemetryClient = null;
TelemetryConfiguration telemetryConfiguration = null;

if (telemetryConfiguration == null && telemetryClient == null)
{
telemetryConfiguration = new TelemetryConfiguration();
sendItems = new List<ITelemetry>();
telemetryConfiguration.TelemetryChannel = new StubTelemetryChannel { OnSend = item => sendItems.Add(item) };
telemetryConfiguration.InstrumentationKey = Guid.NewGuid().ToString();
telemetryConfiguration.TelemetryInitializers.Add(new OperationCorrelationTelemetryInitializer());
telemetryClient = new TelemetryClient(telemetryConfiguration);
}

return (telemetryConfiguration, telemetryClient, sendItems);
}
}
}