Create one session/telemetry configuration per run #515

eddynaka · 2021-11-06T15:09:44Z

Description:

When we changed from single thread to multi-thread application, the reporting rule started to behave differently:

many configurations were being created
many sessions were being created
hash wasn't showing correctly since the hash isn't calculated previously

Fixes:

This PR will fix the following issues:

session/telemetry objects should not be overridden if already exists
session/telemetry objects should be created only once (even in a multithread environment)
hash should have a reference to the context since it will be only created once we start loading a specific file

eddynaka · 2021-11-06T16:38:35Z

src/BinSkim.Sdk/CompilerDataLogger.cs

@@ -20,15 +20,19 @@
        private const string CommandLineEventName = "CommandLineInformation";
        private const string AssemblyReferencesEventName = "AssemblyReferencesInformation";

-        private readonly int ChunkSize;
-        private readonly string sha256;
+        private string Sha256 => context?.Hashes?.Sha256;


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

eddynaka · 2021-11-06T16:41:18Z

src/BinSkim.Sdk/CompilerDataLogger.cs

-            s_telemetryClient = telemetryClient;
-            s_sessionId = TelemetryEnabled ? Guid.NewGuid().ToString() : null;
-            ChunkSize = chunkSize;
+            s_telemetryConfiguration ??= telemetryConfiguration;


telemetryConfiguration

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

michaelcfanning · 2021-11-09T16:17:19Z

src/BinSkim.Sdk/CompilerDataLogger.cs

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


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

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

michaelcfanning · 2021-11-09T16:19:37Z

src/BinSkim.Sdk/CompilerDataLogger.cs


-        private static string s_sessionId;
+        internal static string s_sessionId;


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

Got it!
thanks for the feedback!

michaelcfanning · 2021-11-09T16:22:39Z

src/BinSkim.Sdk/CompilerDataLogger.cs

-                s_telemetryClient = new TelemetryClient(s_telemetryConfiguration);
+                lock (s_syncRoot)
+                {
+                    if (s_telemetryConfiguration == null && s_telemetryClient == null)


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?

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.

michaelcfanning · 2021-11-09T16:23:37Z

src/BinSkim.Sdk/CompilerDataLogger.cs

            }
        }

+        internal static void Reset()
+        {
+            s_sessionId = null;


s_sessionId

why don't you lock here? #Resolved

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.

michaelcfanning · 2021-11-09T16:24:36Z

src/BinSkim.Sdk/Properties/AssemblyInfo.cs


 [assembly: AssemblyTitle("BinSkim SDK")]
 [assembly: AssemblyDescription("BinSkim SDK and utilities for authoring analysis checks")]
+
+[assembly: InternalsVisibleTo("Test.UnitTests.BinSkim.Driver")]


UnitTests

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

fixed!
thanks!

eddynaka · 2021-11-09T21:49:57Z

src/BinSkim.Sdk/CompilerDataLogger.cs

-                s_sessionId = Guid.NewGuid().ToString();
-                s_telemetryConfiguration = new TelemetryConfiguration(instrumentationKey);
-                s_telemetryClient = new TelemetryClient(s_telemetryConfiguration);
+                lock (s_syncRoot)


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.

michaelcfanning · 2021-11-09T22:54:43Z

src/BinSkim.Sdk/CompilerDataLogger.cs


-        private static string s_sessionId;
        private static bool s_printHeader = true;


private static bool s_printHeader = true;

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

michaelcfanning · 2021-11-09T22:58:44Z

src/BinSkim.Sdk/CompilerDataLogger.cs

-        private readonly string sha256;
+        private string Sha256 => context?.Hashes?.Sha256;
+
+        private readonly int chunkSize;
        private readonly string relativeFilePath;


relativeFilePath

just retrieve this from the context as you do hashes

michaelcfanning · 2021-11-09T23:00:04Z

src/BinSkim.Sdk/CompilerDataLogger.cs

-            s_telemetryClient = telemetryClient;
-            s_sessionId = TelemetryEnabled ? Guid.NewGuid().ToString() : null;
-            ChunkSize = chunkSize;
+            s_telemetryConfiguration ??= telemetryConfiguration;


s_telemetryConfiguration ??= telemetryConfiguration

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

eddynaka added 2 commits November 5, 2021 14:23

Create one session per run

a837219

updating tests

ae1548b

eddynaka commented Nov 6, 2021

View reviewed changes

eddynaka added 2 commits November 6, 2021 14:00

Validating logged hash

aef6a64

removing redundant call to tostring

e119e62

eddynaka changed the title ~~Create one session per run~~ Create one session/telemetry configuration per run Nov 6, 2021

eddynaka marked this pull request as ready for review November 6, 2021 19:17

eddynaka requested a review from michaelcfanning as a code owner November 6, 2021 19:17

michaelcfanning reviewed Nov 9, 2021

View reviewed changes

eddynaka and others added 3 commits November 9, 2021 09:28

Merge branch 'main' into users/ednakamu/create-one-session-per-run

9099778

Addressing PR feedback

da02bdc

deleting nuget.config

df00214

eddynaka commented Nov 9, 2021

View reviewed changes

michaelcfanning reviewed Nov 9, 2021

View reviewed changes

eddynaka merged commit f4909e9 into main Nov 10, 2021

eddynaka deleted the users/ednakamu/create-one-session-per-run branch November 10, 2021 01:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create one session/telemetry configuration per run #515

Create one session/telemetry configuration per run #515

eddynaka commented Nov 6, 2021 •

edited

Loading

eddynaka Nov 6, 2021 •

edited

Loading

eddynaka Nov 6, 2021 •

edited

Loading

michaelcfanning Nov 9, 2021 •

edited by eddynaka

Loading

eddynaka Nov 9, 2021

michaelcfanning Nov 9, 2021 •

edited by eddynaka

Loading

eddynaka Nov 9, 2021

michaelcfanning Nov 9, 2021

eddynaka Nov 9, 2021

michaelcfanning Nov 9, 2021 •

edited by eddynaka

Loading

eddynaka Nov 9, 2021

michaelcfanning Nov 9, 2021 •

edited by eddynaka

Loading

eddynaka Nov 9, 2021

eddynaka Nov 9, 2021

michaelcfanning Nov 9, 2021

michaelcfanning Nov 9, 2021

michaelcfanning Nov 9, 2021


		private static string s_sessionId;
		internal static string s_sessionId;

Create one session/telemetry configuration per run #515

Create one session/telemetry configuration per run #515

Conversation

eddynaka commented Nov 6, 2021 • edited Loading

Description:

Fixes:

eddynaka Nov 6, 2021 • edited Loading

Choose a reason for hiding this comment

eddynaka Nov 6, 2021 • edited Loading

Choose a reason for hiding this comment

michaelcfanning Nov 9, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Nov 9, 2021 • edited by eddynaka 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 Nov 9, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Nov 9, 2021 • edited by eddynaka 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

eddynaka commented Nov 6, 2021 •

edited

Loading

eddynaka Nov 6, 2021 •

edited

Loading

eddynaka Nov 6, 2021 •

edited

Loading

michaelcfanning Nov 9, 2021 •

edited by eddynaka

Loading

michaelcfanning Nov 9, 2021 •

edited by eddynaka

Loading

michaelcfanning Nov 9, 2021 •

edited by eddynaka

Loading

michaelcfanning Nov 9, 2021 •

edited by eddynaka

Loading