Enable Roslyn analysis - StartupHook (#4665)

Kielek · web-flow · commit cfbee7af5cbf · 2025-12-03T07:59:06.000+01:00
* StartupHook - enable analysis

* CA1035

* CA1309

* CA1310

* CA1806

* CA2201

* CA2201

* CA1859

* Mute CA1031 on the project level
diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/OpenTelemetry.AutoInstrumentation.StartupHook.csproj b/src/OpenTelemetry.AutoInstrumentation.StartupHook/OpenTelemetry.AutoInstrumentation.StartupHook.csproj
@@ -6,6 +6,9 @@
          Reference: https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/issues/1644 -->
     <TargetFrameworks>netcoreapp3.1</TargetFrameworks>
     <Description>StartupHook used by the OpenTelemetry.AutoInstrumentation project.</Description>
+    <!-- Suppress CA1031 warnings about catching general exception types. In this project we need to catch all exceptions to prevent the application from crashing. -->
+    <NoWarn>$(NoWarn);CA1031</NoWarn>
+    <AnalysisLevel>latest-All</AnalysisLevel>
   </PropertyGroup>
 
   <ItemGroup>
diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/ApplicationInExcludeListRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/ApplicationInExcludeListRule.cs
@@ -20,7 +20,7 @@ public ApplicationInExcludeListRule()
     internal override bool Evaluate()
     {
         var appDomainName = GetAppDomainName();
-        if (appDomainName.Equals("dotnet", StringComparison.InvariantCultureIgnoreCase))
+        if (appDomainName.Equals("dotnet", StringComparison.OrdinalIgnoreCase))
         {
             Logger.Information($"Rule Engine: AppDomain name is dotnet. Skipping initialization.");
             return false;
@@ -65,7 +65,7 @@ private static string GetAppDomainName()
         }
     }
 
-    private static ICollection<string> GetExcludedApplicationNames()
+    private static List<string> GetExcludedApplicationNames()
     {
         var excludedProcesses = new List<string>();
 
diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/AssemblyFileVersionRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/AssemblyFileVersionRule.cs
@@ -61,7 +61,7 @@ internal override bool Evaluate()
         }
         catch (Exception ex)
         {
-            // Exception in rule evaluation should not impact the result of the rule.
+            // An exception in rule evaluation should not impact the result of the rule.
             Logger.Warning($"Rule Engine: Couldn't evaluate assembly reference file version. Exception: {ex}");
         }
 
diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/StartupHook.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/StartupHook.cs
@@ -23,7 +23,7 @@ internal class StartupHook
     /// </summary>
     public static void Initialize()
     {
-        bool.TryParse(Environment.GetEnvironmentVariable(ConfigurationKeys.FailFast), out var failFast);
+        _ = bool.TryParse(Environment.GetEnvironmentVariable(ConfigurationKeys.FailFast), out var failFast);
 
         try
         {
@@ -32,7 +32,7 @@ public static void Initialize()
             var ruleEngine = new RuleEngine();
             if (!ruleEngine.ValidateRules())
             {
-                throw new Exception(
+                throw new InvalidOperationException(
                     "Rule Engine Failure: One or more rules failed validation. Automatic Instrumentation won't be loaded.");
             }
 
@@ -47,7 +47,7 @@ public static void Initialize()
             {
                 if (failFast)
                 {
-                    throw new Exception("StartupHook failed to create an instance of the Loader");
+                    throw new InvalidOperationException("StartupHook failed to create an instance of the Loader");
                 }
             }
             else
@@ -74,15 +74,15 @@ private static string GetLoaderAssemblyLocation()
         try
         {
             var startupAssemblyFilePath = Assembly.GetExecutingAssembly().Location;
-            if (startupAssemblyFilePath.StartsWith(@"\\?\"))
+            if (startupAssemblyFilePath.StartsWith(@"\\?\", StringComparison.Ordinal))
             {
                 // This will only be used in case the local path exceeds max_path size limit
                 startupAssemblyFilePath = startupAssemblyFilePath.Substring(4);
             }
 
             // StartupHook and Loader assemblies are in the same path
             var startupAssemblyDirectoryPath = Path.GetDirectoryName(startupAssemblyFilePath) ??
-                                               throw new NullReferenceException("StartupAssemblyFilePath is NULL");
+                                               throw new InvalidOperationException("StartupAssemblyFilePath is NULL");
             return startupAssemblyDirectoryPath;
         }
         catch (Exception ex)
diff --git a/src/OpenTelemetry.AutoInstrumentation/Logging/InternalLogger.cs b/src/OpenTelemetry.AutoInstrumentation/Logging/InternalLogger.cs
@@ -1,6 +1,8 @@
 // Copyright The OpenTelemetry Authors
 // SPDX-License-Identifier: Apache-2.0
 
+using System.Globalization;
+
 namespace OpenTelemetry.AutoInstrumentation.Logging;
 
 internal class InternalLogger : IOtelLogger
@@ -214,7 +216,7 @@ private void WriteImpl(LogLevel level, Exception? exception, string messageTempl
         try
         {
             // Use template if no arguments provided, otherwise format the template with provided arguments.
-            var rawMessage = args == NoPropertyValues ? messageTemplate : string.Format(messageTemplate, args);
+            var rawMessage = args == NoPropertyValues ? messageTemplate : string.Format(CultureInfo.InvariantCulture, messageTemplate, args);
             if (exception != null)
             {
                 rawMessage += $"{Environment.NewLine}Exception: {exception.Message}{Environment.NewLine}{exception}";
@@ -226,7 +228,9 @@ private void WriteImpl(LogLevel level, Exception? exception, string messageTempl
                 WriteEventSourceLog(level, rawMessage);
             }
         }
-        catch
+#pragma warning disable CA1031 // Do not catch general exception
+        catch (Exception)
+#pragma warning restore CA1031 // Do not catch general exception
         {
             try
             {
@@ -236,7 +240,9 @@ private void WriteImpl(LogLevel level, Exception? exception, string messageTempl
                     : "; " + string.Join(", ", args);
                 Console.Error.WriteLine($"{messageTemplate}{properties}{ex}");
             }
-            catch
+#pragma warning disable CA1031 // Do not catch general exception
+            catch (Exception)
+#pragma warning restore CA1031 // Do not catch general exception
             {
                 // ignore
             }
diff --git a/src/OpenTelemetry.AutoInstrumentation/Logging/OtelLogging.cs b/src/OpenTelemetry.AutoInstrumentation/Logging/OtelLogging.cs
@@ -54,7 +54,9 @@ public static void CloseLogger(string suffix, IOtelLogger otelLogger)
                 otelLogger.Close();
             }
         }
+#pragma warning disable CA1031 // Do not catch general exception
         catch (Exception)
+#pragma warning restore CA1031 // Do not catch general exception
         {
             // intentionally empty
         }
@@ -85,7 +87,9 @@ internal static void Reset()
                 _ => logLevel
             };
         }
+#pragma warning disable CA1031 // Do not catch general exception
         catch (Exception)
+#pragma warning restore CA1031 // Do not catch general exception
         {
             // theoretically, can happen when process has no privileges to check env
         }
@@ -110,7 +114,9 @@ internal static LogSink GetConfiguredLogSink()
                 _ => logSink
             };
         }
+#pragma warning disable CA1031 // Do not catch general exception
         catch (Exception)
+#pragma warning restore CA1031 // Do not catch general exception
         {
             // theoretically, can happen when process has no privileges to check env
         }
@@ -132,7 +138,9 @@ internal static long GetConfiguredFileSizeLimitBytes()
 
             return long.TryParse(configuredFileSizeLimit, out var limit) && limit > 0 ? limit : defaultFileSizeLimitBytes;
         }
+#pragma warning disable CA1031 // Do not catch general exception
         catch (Exception)
+#pragma warning restore CA1031 // Do not catch general exception
         {
             // theoretically, can happen when process has no privileges to check env
             return defaultFileSizeLimitBytes;
@@ -154,7 +162,7 @@ private static IOtelLogger CreateLogger(string suffix)
     private static ISink CreateSink(string suffix)
     {
         // Uses ISink? here, sink creation can fail so we specify default fallback at the end.
-        var sink = _configuredLogSink switch
+        ISink? sink = _configuredLogSink switch
         {
             LogSink.NoOp => NoopSink.Instance,
             LogSink.Console => new ConsoleSink(suffix),
@@ -168,7 +176,7 @@ private static ISink CreateSink(string suffix)
             NoopSink.Instance;
     }
 
-    private static ISink? CreateFileSink(string suffix)
+    private static PeriodicFlushToDiskSink? CreateFileSink(string suffix)
     {
         try
         {
@@ -188,7 +196,9 @@ private static ISink CreateSink(string suffix)
                 return new PeriodicFlushToDiskSink(rollingFileSink, TimeSpan.FromSeconds(5));
             }
         }
+#pragma warning disable CA1031 // Do not catch general exception
         catch (Exception)
+#pragma warning restore CA1031 // Do not catch general exception
         {
             // unable to configure logging to a file
         }
@@ -208,7 +218,9 @@ private static string GetLogFileName(string suffix)
                 ? $"otel-dotnet-auto-{process.Id}-{appDomainName}-.log"
                 : $"otel-dotnet-auto-{process.Id}-{appDomainName}-{suffix}-.log";
         }
-        catch
+#pragma warning disable CA1031 // Do not catch general exception
+        catch (Exception)
+#pragma warning restore CA1031 // Do not catch general exception
         {
             // We can't get the process info
             return string.IsNullOrEmpty(suffix)
@@ -250,7 +262,9 @@ private static string GetEncodedAppDomainName()
 
             logDirectory = CreateDirectoryIfMissing(logDirectory) ?? Path.GetTempPath();
         }
-        catch
+#pragma warning disable CA1031 // Do not catch general exception
+        catch (Exception)
+#pragma warning restore CA1031 // Do not catch general exception
         {
             // The try block may throw a SecurityException if not granted the System.Security.Permissions.FileIOPermission
             // because of the following API calls
@@ -272,7 +286,9 @@ private static string GetEncodedAppDomainName()
             Directory.CreateDirectory(pathToCreate);
             return pathToCreate;
         }
-        catch
+#pragma warning disable CA1031 // Do not catch general exception
+        catch (Exception)
+#pragma warning restore CA1031 // Do not catch general exception
         {
             // Unable to create the directory meaning that the user will have to create it on their own.
             // It is unsafe to log here, so return null to defer deciding what the path is

Original file line number	Diff line number	Diff line change
`@@ -20,7 +20,7 @@ public ApplicationInExcludeListRule()`
`20`	`20`	`internal override bool Evaluate()`
`21`	`21`	`{`
`22`	`22`	`var appDomainName = GetAppDomainName();`
`23`		`- if (appDomainName.Equals("dotnet", StringComparison.InvariantCultureIgnoreCase))`
	`23`	`+ if (appDomainName.Equals("dotnet", StringComparison.OrdinalIgnoreCase))`
`24`	`24`	`{`
`25`	`25`	`Logger.Information($"Rule Engine: AppDomain name is dotnet. Skipping initialization.");`
`26`	`26`	`return false;`
`@@ -65,7 +65,7 @@ private static string GetAppDomainName()`
`65`	`65`	`}`
`66`	`66`	`}`
`67`	`67`
`68`		`- private static ICollection<string> GetExcludedApplicationNames()`
	`68`	`+ private static List<string> GetExcludedApplicationNames()`
`69`	`69`	`{`
`70`	`70`	`var excludedProcesses = new List<string>();`
`71`	`71`
Original file line number	Diff line number	Diff line change
`@@ -61,7 +61,7 @@ internal override bool Evaluate()`
`61`	`61`	`}`
`62`	`62`	`catch (Exception ex)`
`63`	`63`	`{`
`64`		`- // Exception in rule evaluation should not impact the result of the rule.`
	`64`	`+ // An exception in rule evaluation should not impact the result of the rule.`
`65`	`65`	`Logger.Warning($"Rule Engine: Couldn't evaluate assembly reference file version. Exception: {ex}");`
`66`	`66`	`}`
`67`	`67`
Original file line number	Diff line number	Diff line change
`@@ -23,7 +23,7 @@ internal class StartupHook`
`23`	`23`	`/// </summary>`
`24`	`24`	`public static void Initialize()`
`25`	`25`	`{`
`26`		`- bool.TryParse(Environment.GetEnvironmentVariable(ConfigurationKeys.FailFast), out var failFast);`
	`26`	`+ _ = bool.TryParse(Environment.GetEnvironmentVariable(ConfigurationKeys.FailFast), out var failFast);`
`27`	`27`
`28`	`28`	`try`
`29`	`29`	`{`
`@@ -32,7 +32,7 @@ public static void Initialize()`
`32`	`32`	`var ruleEngine = new RuleEngine();`
`33`	`33`	`if (!ruleEngine.ValidateRules())`
`34`	`34`	`{`
`35`		`- throw new Exception(`
	`35`	`+ throw new InvalidOperationException(`
`36`	`36`	`"Rule Engine Failure: One or more rules failed validation. Automatic Instrumentation won't be loaded.");`
`37`	`37`	`}`
`38`	`38`
`@@ -47,7 +47,7 @@ public static void Initialize()`
`47`	`47`	`{`
`48`	`48`	`if (failFast)`
`49`	`49`	`{`
`50`		`- throw new Exception("StartupHook failed to create an instance of the Loader");`
	`50`	`+ throw new InvalidOperationException("StartupHook failed to create an instance of the Loader");`
`51`	`51`	`}`
`52`	`52`	`}`
`53`	`53`	`else`
`@@ -74,15 +74,15 @@ private static string GetLoaderAssemblyLocation()`
`74`	`74`	`try`
`75`	`75`	`{`
`76`	`76`	`var startupAssemblyFilePath = Assembly.GetExecutingAssembly().Location;`
`77`		`- if (startupAssemblyFilePath.StartsWith(@"\\?\"))`
	`77`	`+ if (startupAssemblyFilePath.StartsWith(@"\\?\", StringComparison.Ordinal))`
`78`	`78`	`{`
`79`	`79`	`// This will only be used in case the local path exceeds max_path size limit`
`80`	`80`	`startupAssemblyFilePath = startupAssemblyFilePath.Substring(4);`
`81`	`81`	`}`
`82`	`82`
`83`	`83`	`// StartupHook and Loader assemblies are in the same path`
`84`	`84`	`var startupAssemblyDirectoryPath = Path.GetDirectoryName(startupAssemblyFilePath) ??`
`85`		`- throw new NullReferenceException("StartupAssemblyFilePath is NULL");`
	`85`	`+ throw new InvalidOperationException("StartupAssemblyFilePath is NULL");`
`86`	`86`	`return startupAssemblyDirectoryPath;`
`87`	`87`	`}`
`88`	`88`	`catch (Exception ex)`
Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,8 @@`
`1`	`1`	`// Copyright The OpenTelemetry Authors`
`2`	`2`	`// SPDX-License-Identifier: Apache-2.0`
`3`	`3`
	`4`	`+using System.Globalization;`
	`5`	`+`
`4`	`6`	`namespace OpenTelemetry.AutoInstrumentation.Logging;`
`5`	`7`
`6`	`8`	`internal class InternalLogger : IOtelLogger`
`@@ -214,7 +216,7 @@ private void WriteImpl(LogLevel level, Exception? exception, string messageTempl`
`214`	`216`	`try`
`215`	`217`	`{`
`216`	`218`	`// Use template if no arguments provided, otherwise format the template with provided arguments.`
`217`		`- var rawMessage = args == NoPropertyValues ? messageTemplate : string.Format(messageTemplate, args);`
	`219`	`+ var rawMessage = args == NoPropertyValues ? messageTemplate : string.Format(CultureInfo.InvariantCulture, messageTemplate, args);`
`218`	`220`	`if (exception != null)`
`219`	`221`	`{`
`220`	`222`	`rawMessage += $"{Environment.NewLine}Exception: {exception.Message}{Environment.NewLine}{exception}";`
`@@ -226,7 +228,9 @@ private void WriteImpl(LogLevel level, Exception? exception, string messageTempl`
`226`	`228`	`WriteEventSourceLog(level, rawMessage);`
`227`	`229`	`}`
`228`	`230`	`}`
`229`		`- catch`
	`231`	`+#pragma warning disable CA1031 // Do not catch general exception`
	`232`	`+ catch (Exception)`
	`233`	`+#pragma warning restore CA1031 // Do not catch general exception`
`230`	`234`	`{`
`231`	`235`	`try`
`232`	`236`	`{`
`@@ -236,7 +240,9 @@ private void WriteImpl(LogLevel level, Exception? exception, string messageTempl`
`236`	`240`	`: "; " + string.Join(", ", args);`
`237`	`241`	`Console.Error.WriteLine($"{messageTemplate}{properties}{ex}");`
`238`	`242`	`}`
`239`		`- catch`
	`243`	`+#pragma warning disable CA1031 // Do not catch general exception`
	`244`	`+ catch (Exception)`
	`245`	`+#pragma warning restore CA1031 // Do not catch general exception`
`240`	`246`	`{`
`241`	`247`	`// ignore`
`242`	`248`	`}`
Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,9 @@ public static void CloseLogger(string suffix, IOtelLogger otelLogger)`
`54`	`54`	`otelLogger.Close();`
`55`	`55`	`}`
`56`	`56`	`}`
	`57`	`+#pragma warning disable CA1031 // Do not catch general exception`
`57`	`58`	`catch (Exception)`
	`59`	`+#pragma warning restore CA1031 // Do not catch general exception`
`58`	`60`	`{`
`59`	`61`	`// intentionally empty`
`60`	`62`	`}`
`@@ -85,7 +87,9 @@ internal static void Reset()`
`85`	`87`	`_ => logLevel`
`86`	`88`	`};`
`87`	`89`	`}`
	`90`	`+#pragma warning disable CA1031 // Do not catch general exception`
`88`	`91`	`catch (Exception)`
	`92`	`+#pragma warning restore CA1031 // Do not catch general exception`
`89`	`93`	`{`
`90`	`94`	`// theoretically, can happen when process has no privileges to check env`
`91`	`95`	`}`
`@@ -110,7 +114,9 @@ internal static LogSink GetConfiguredLogSink()`
`110`	`114`	`_ => logSink`
`111`	`115`	`};`
`112`	`116`	`}`
	`117`	`+#pragma warning disable CA1031 // Do not catch general exception`
`113`	`118`	`catch (Exception)`
	`119`	`+#pragma warning restore CA1031 // Do not catch general exception`
`114`	`120`	`{`
`115`	`121`	`// theoretically, can happen when process has no privileges to check env`
`116`	`122`	`}`
`@@ -132,7 +138,9 @@ internal static long GetConfiguredFileSizeLimitBytes()`
`132`	`138`
`133`	`139`	`return long.TryParse(configuredFileSizeLimit, out var limit) && limit > 0 ? limit : defaultFileSizeLimitBytes;`
`134`	`140`	`}`
	`141`	`+#pragma warning disable CA1031 // Do not catch general exception`
`135`	`142`	`catch (Exception)`
	`143`	`+#pragma warning restore CA1031 // Do not catch general exception`
`136`	`144`	`{`
`137`	`145`	`// theoretically, can happen when process has no privileges to check env`
`138`	`146`	`return defaultFileSizeLimitBytes;`
`@@ -154,7 +162,7 @@ private static IOtelLogger CreateLogger(string suffix)`
`154`	`162`	`private static ISink CreateSink(string suffix)`
`155`	`163`	`{`
`156`	`164`	`// Uses ISink? here, sink creation can fail so we specify default fallback at the end.`
`157`		`- var sink = _configuredLogSink switch`
	`165`	`+ ISink? sink = _configuredLogSink switch`
`158`	`166`	`{`
`159`	`167`	`LogSink.NoOp => NoopSink.Instance,`
`160`	`168`	`LogSink.Console => new ConsoleSink(suffix),`
`@@ -168,7 +176,7 @@ private static ISink CreateSink(string suffix)`
`168`	`176`	`NoopSink.Instance;`
`169`	`177`	`}`
`170`	`178`
`171`		`- private static ISink? CreateFileSink(string suffix)`
	`179`	`+ private static PeriodicFlushToDiskSink? CreateFileSink(string suffix)`
`172`	`180`	`{`
`173`	`181`	`try`
`174`	`182`	`{`
`@@ -188,7 +196,9 @@ private static ISink CreateSink(string suffix)`
`188`	`196`	`return new PeriodicFlushToDiskSink(rollingFileSink, TimeSpan.FromSeconds(5));`
`189`	`197`	`}`
`190`	`198`	`}`
	`199`	`+#pragma warning disable CA1031 // Do not catch general exception`
`191`	`200`	`catch (Exception)`
	`201`	`+#pragma warning restore CA1031 // Do not catch general exception`
`192`	`202`	`{`
`193`	`203`	`// unable to configure logging to a file`
`194`	`204`	`}`
`@@ -208,7 +218,9 @@ private static string GetLogFileName(string suffix)`
`208`	`218`	`? $"otel-dotnet-auto-{process.Id}-{appDomainName}-.log"`
`209`	`219`	`: $"otel-dotnet-auto-{process.Id}-{appDomainName}-{suffix}-.log";`
`210`	`220`	`}`
`211`		`- catch`
	`221`	`+#pragma warning disable CA1031 // Do not catch general exception`
	`222`	`+ catch (Exception)`
	`223`	`+#pragma warning restore CA1031 // Do not catch general exception`
`212`	`224`	`{`
`213`	`225`	`// We can't get the process info`
`214`	`226`	`return string.IsNullOrEmpty(suffix)`
`@@ -250,7 +262,9 @@ private static string GetEncodedAppDomainName()`
`250`	`262`
`251`	`263`	`logDirectory = CreateDirectoryIfMissing(logDirectory) ?? Path.GetTempPath();`
`252`	`264`	`}`
`253`		`- catch`
	`265`	`+#pragma warning disable CA1031 // Do not catch general exception`
	`266`	`+ catch (Exception)`
	`267`	`+#pragma warning restore CA1031 // Do not catch general exception`
`254`	`268`	`{`
`255`	`269`	`// The try block may throw a SecurityException if not granted the System.Security.Permissions.FileIOPermission`
`256`	`270`	`// because of the following API calls`
`@@ -272,7 +286,9 @@ private static string GetEncodedAppDomainName()`
`272`	`286`	`Directory.CreateDirectory(pathToCreate);`
`273`	`287`	`return pathToCreate;`
`274`	`288`	`}`
`275`		`- catch`
	`289`	`+#pragma warning disable CA1031 // Do not catch general exception`
	`290`	`+ catch (Exception)`
	`291`	`+#pragma warning restore CA1031 // Do not catch general exception`
`276`	`292`	`{`
`277`	`293`	`// Unable to create the directory meaning that the user will have to create it on their own.`
`278`	`294`	`// It is unsafe to log here, so return null to defer deciding what the path is`