From e45898a134d86b5ec63cc70c673e481191d870c7 Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Wed, 13 Sep 2023 14:49:37 -0700 Subject: [PATCH 1/4] Remove rule dedicated to S.D.DS validation --- build/Build.Steps.cs | 13 ++- ...mblyRule.cs => AssemblyFileVersionRule.cs} | 44 ++++---- .../RulesEngine/DiagnosticSourceRule.cs | 104 ------------------ .../RulesEngine/RuleEngine.cs | 3 +- .../DiagnosticSourceRuleTests.cs | 72 ------------ 5 files changed, 33 insertions(+), 203 deletions(-) rename src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/{InstrumentationAssemblyRule.cs => AssemblyFileVersionRule.cs} (57%) delete mode 100644 src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs delete mode 100644 test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs diff --git a/build/Build.Steps.cs b/build/Build.Steps.cs index 49f05d187a..278a45e4c8 100644 --- a/build/Build.Steps.cs +++ b/build/Build.Steps.cs @@ -557,18 +557,17 @@ DotNetBuildSettings BuildTestApplication(DotNetBuildSettings x) => .Executes(() => { var netPath = TracerHomeDirectory / "net"; - var ruleEngineJsonFilePath = netPath / "ruleEngine.json"; - var ruleEngineJsonNugetFilePath = RootDirectory / "nuget" / "OpenTelemetry.AutoInstrumentation" / "contentFiles" / "any" / "any" / "RuleEngine.json"; - var fileInfoList = new List(); var files = Directory.GetFiles(netPath); + var fileInfoList = new List(files.Length); foreach (string file in files) { var fileName = Path.GetFileNameWithoutExtension(file); - var fileVersion = FileVersionInfo.GetVersionInfo(file).FileVersion; - if (fileName.StartsWith("OpenTelemetry.") && !fileName.StartsWith("OpenTelemetry.Api") && !fileName.StartsWith("OpenTelemetry.AutoInstrumentation")) + if (fileName == "System.Diagnostics.DiagnosticSource" || + (fileName.StartsWith("OpenTelemetry.") && !fileName.StartsWith("OpenTelemetry.Api") && !fileName.StartsWith("OpenTelemetry.AutoInstrumentation"))) { + var fileVersion = FileVersionInfo.GetVersionInfo(file).FileVersion; fileInfoList.Add(new { FileName = fileName, @@ -579,7 +578,11 @@ DotNetBuildSettings BuildTestApplication(DotNetBuildSettings x) => JsonSerializerOptions options = new JsonSerializerOptions { WriteIndented = true }; string jsonContent = JsonSerializer.Serialize(fileInfoList, options); + + var ruleEngineJsonFilePath = netPath / "ruleEngine.json"; File.WriteAllText(ruleEngineJsonFilePath, jsonContent); + + var ruleEngineJsonNugetFilePath = RootDirectory / "nuget" / "OpenTelemetry.AutoInstrumentation" / "contentFiles" / "any" / "any" / "RuleEngine.json"; File.Delete(ruleEngineJsonNugetFilePath); File.Copy(ruleEngineJsonFilePath, ruleEngineJsonNugetFilePath); }); diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/InstrumentationAssemblyRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/AssemblyFileVersionRule.cs similarity index 57% rename from src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/InstrumentationAssemblyRule.cs rename to src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/AssemblyFileVersionRule.cs index 7f6ca27cbf..3c3796a0fa 100644 --- a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/InstrumentationAssemblyRule.cs +++ b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/AssemblyFileVersionRule.cs @@ -1,4 +1,4 @@ -// +// // Copyright The OpenTelemetry Authors // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -21,14 +21,14 @@ namespace OpenTelemetry.AutoInstrumentation.RulesEngine; -internal class InstrumentationAssemblyRule : Rule +internal class AssemblyFileVersionRule : Rule { private static readonly IOtelLogger Logger = OtelLogging.GetLogger("StartupHook"); - public InstrumentationAssemblyRule() + public AssemblyFileVersionRule() { - Name = "Instrumentation Assembly Validator"; - Description = "Ensure that the version of the OpenTelemetry Instrumentation libraries is not older than the version used by Automatic Instrumentation."; + Name = "Assembly File Version Validator"; + Description = "Ensure that the version of key assemblies is not older than the version used by Automatic Instrumentation."; } internal override bool Evaluate() @@ -41,6 +41,7 @@ internal override bool Evaluate() var ruleEngineContent = File.ReadAllText(ruleEngineFileLocation); var ruleFileInfoList = JsonSerializer.Deserialize>(ruleEngineContent); var entryAssembly = Assembly.GetEntryAssembly(); + Logger.Information($"Rule Engine: Entry Assembly: {entryAssembly?.FullName}"); var referencedAssemblies = entryAssembly?.GetReferencedAssemblies(); if (referencedAssemblies == null) @@ -52,30 +53,33 @@ internal override bool Evaluate() foreach (var referencedAssembly in referencedAssemblies) { var ruleFileInfo = ruleFileInfoList.FirstOrDefault(file => file.FileName == referencedAssembly.Name); - if (ruleFileInfo != null) + if (ruleFileInfo is null) { - var autoInstrumentationFileVersion = new Version(ruleFileInfo.FileVersion); + // If the assembly is not directly referenced by the entry assembly, assume that it is okay to proceed. + continue; + } + + var autoInstrumentationFileVersion = new Version(ruleFileInfo.FileVersion); - var appInstrumentationAssembly = Assembly.Load(referencedAssembly); - var appInstrumentationFileVersionInfo = FileVersionInfo.GetVersionInfo(appInstrumentationAssembly.Location); - var appInstrumentationFileVersion = new Version(appInstrumentationFileVersionInfo.FileVersion); + var appInstrumentationAssembly = Assembly.Load(referencedAssembly); + var appInstrumentationFileVersionInfo = FileVersionInfo.GetVersionInfo(appInstrumentationAssembly.Location); + var appInstrumentationFileVersion = new Version(appInstrumentationFileVersionInfo.FileVersion); - if (appInstrumentationFileVersion < autoInstrumentationFileVersion) - { - result = false; - Logger.Error($"Rule Engine: Application has direct or indirect reference to older version of Instrumentation library {ruleFileInfo.FileName} - {ruleFileInfo.FileVersion}."); - } - else - { - Logger.Information($"Rule Engine: Application has reference to Instrumentation library {ruleFileInfo.FileName} and loaded successfully."); - } + if (appInstrumentationFileVersion < autoInstrumentationFileVersion) + { + result = false; + Logger.Error($"Rule Engine: Application has direct or indirect reference to older version of assembly {ruleFileInfo.FileName} - {ruleFileInfo.FileVersion}."); + } + else + { + Logger.Information($"Rule Engine: Application has reference to assembly {ruleFileInfo.FileName} and loaded successfully."); } } } catch (Exception ex) { // Exception in rule evaluation should not impact the result of the rule. - Logger.Warning($"Rule Engine:Couldn't evaluate OpenTelemetry Instrumentation Evaluation. Exception: {ex}"); + Logger.Warning($"Rule Engine: Couldn't evaluate assembly reference file version. Exception: {ex}"); } return result; diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs deleted file mode 100644 index 76ef0ef811..0000000000 --- a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs +++ /dev/null @@ -1,104 +0,0 @@ -// -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -using System.Diagnostics; -using System.Reflection; -using OpenTelemetry.AutoInstrumentation.Logging; - -namespace OpenTelemetry.AutoInstrumentation.RulesEngine; - -internal class DiagnosticSourceRule : Rule -{ - private static IOtelLogger logger = OtelLogging.GetLogger("StartupHook"); - - public DiagnosticSourceRule() - { - Name = "System.Diagnostics.DiagnosticSource Validator"; - Description = "Ensure that the System.Diagnostics.DiagnosticSource version is not older than the version used by the Automatic Instrumentation"; - } - - // This constructor is used for test purpose. - protected DiagnosticSourceRule(IOtelLogger otelLogger) - : this() - { - logger = otelLogger; - } - - internal override bool Evaluate() - { - string? olderDiagnosticSourcePackageVersion = null; - - try - { - // NuGet package version is not available at runtime. AssemblyVersion is available but it only changes in major version updates. - // Thus using AssemblyFileVersion to compare whether the loaded version is older than that of auto instrumentation. - var loadedDiagnosticSourceFileVersion = GetResolvedVersion(); - if (loadedDiagnosticSourceFileVersion != null) - { - var autoInstrumentationDiagnosticSourceFileVersion = GetVersionFromAutoInstrumentation(); - if (loadedDiagnosticSourceFileVersion < autoInstrumentationDiagnosticSourceFileVersion) - { - olderDiagnosticSourcePackageVersion = loadedDiagnosticSourceFileVersion.ToString(); - } - } - } - catch (Exception ex) - { - // Exception in evaluation should not throw or crash the process. - logger.Warning($"Rule Engine: Couldn't evaluate System.Diagnostics.DiagnosticSource version. Exception: {ex}"); - return false; - } - - if (olderDiagnosticSourcePackageVersion != null) - { - logger.Error($"Rule Engine: Application has direct or indirect reference to older version of System.Diagnostics.DiagnosticSource.dll {olderDiagnosticSourcePackageVersion}."); - return false; - } - - logger.Information("Rule Engine: DiagnosticSourceRule evaluation success."); - return true; - } - - protected virtual Version? GetResolvedVersion() - { - // Look up for type with an assembly name, will load the library. - var diagnosticSourceType = Type.GetType("System.Diagnostics.DiagnosticSource, System.Diagnostics.DiagnosticSource"); - if (diagnosticSourceType != null) - { - var loadedDiagnosticSourceAssembly = Assembly.GetAssembly(diagnosticSourceType); - var loadedDiagnosticSourceAssemblyFileVersionAttribute = loadedDiagnosticSourceAssembly?.GetCustomAttribute(); - var versionString = loadedDiagnosticSourceAssemblyFileVersionAttribute?.Version; - if (versionString != null) - { - return new Version(versionString); - } - } - - return null; - } - - protected virtual Version GetVersionFromAutoInstrumentation() - { - var autoInstrumentationDiagnosticSourceLocation = Path.Combine(StartupHook.LoaderAssemblyLocation ?? string.Empty, "System.Diagnostics.DiagnosticSource.dll"); - var fileVersionInfo = FileVersionInfo.GetVersionInfo(autoInstrumentationDiagnosticSourceLocation); - var autoInstrumentationDiagnosticSourceFileVersion = new Version( - fileVersionInfo.FileMajorPart, - fileVersionInfo.FileMinorPart, - fileVersionInfo.FileBuildPart, - fileVersionInfo.FilePrivatePart); - return autoInstrumentationDiagnosticSourceFileVersion; - } -} diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/RuleEngine.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/RuleEngine.cs index 8b4eb3e3e3..b4da092982 100644 --- a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/RuleEngine.cs +++ b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/RuleEngine.cs @@ -31,8 +31,7 @@ internal class RuleEngine private readonly List _otherRules = new() { new OpenTelemetrySdkMinimumVersionRule(), - new DiagnosticSourceRule(), - new InstrumentationAssemblyRule(), + new AssemblyFileVersionRule(), new NativeProfilerDiagnosticsRule() }; diff --git a/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs b/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs deleted file mode 100644 index 1ea5e93f52..0000000000 --- a/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs +++ /dev/null @@ -1,72 +0,0 @@ -// -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -using OpenTelemetry.AutoInstrumentation.Logging; -using OpenTelemetry.AutoInstrumentation.RulesEngine; -using Xunit; - -namespace OpenTelemetry.AutoInstrumentation.StartupHook.Tests; -public class DiagnosticSourceRuleTests -{ -#pragma warning disable CS8625 // Cannot convert null literal to non-nullable reference type. - public static IEnumerable RuleTestData => - new List - { - new object[] { "6.0.0.0", "7.0.0.0", "Rule Engine: Application has direct or indirect reference to older version of System.Diagnostics.DiagnosticSource.dll 6.0.0.0.", false }, - new object[] { "8.0.0.0", "7.0.0.0", "Rule Engine: DiagnosticSourceRule evaluation success.", true }, - new object[] { "7.0.0.0", "7.0.0.0", "Rule Engine: DiagnosticSourceRule evaluation success.", true }, - new object[] { null, "7.0.0.0", "Rule Engine: DiagnosticSourceRule evaluation success.", true }, - }; -#pragma warning restore CS8625 // Cannot convert null literal to non-nullable reference type. - - [Theory] - [MemberData(nameof(RuleTestData))] - public void DiagnosticSourceVersion(string appVersion, string autoInstrumentationVersion, string logMessage, bool result) - { - var logger = new TestLogger(); - var rule = new TestableDiagnosticSourceRule(appVersion, autoInstrumentationVersion, logger); - Assert.Equal(result, rule.Evaluate()); - Assert.Equal(logMessage, logger.LogRecords[0].Message); - } -} - -internal class TestableDiagnosticSourceRule : DiagnosticSourceRule -{ - private readonly string appVersion; - private readonly string autoInstrumentationVersion; - - public TestableDiagnosticSourceRule(string appVersion, string autoInstrumentationVersion, IOtelLogger otelLogger) - : base(otelLogger) - { - this.appVersion = appVersion; - this.autoInstrumentationVersion = autoInstrumentationVersion; - } - - protected override Version? GetResolvedVersion() - { - if (appVersion == null) - { - return null; - } - - return new Version(appVersion); - } - - protected override Version GetVersionFromAutoInstrumentation() - { - return new Version(autoInstrumentationVersion); - } -} From c10e32e4aa3b8569d2450e72e828c96e0b6bcc8b Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Wed, 13 Sep 2023 19:41:44 -0700 Subject: [PATCH 2/4] Reduce size of code change --- .../RulesEngine/AssemblyFileVersionRule.cs | 31 +++++++++---------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/AssemblyFileVersionRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/AssemblyFileVersionRule.cs index 3c3796a0fa..41e90e38a3 100644 --- a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/AssemblyFileVersionRule.cs +++ b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/AssemblyFileVersionRule.cs @@ -53,26 +53,23 @@ internal override bool Evaluate() foreach (var referencedAssembly in referencedAssemblies) { var ruleFileInfo = ruleFileInfoList.FirstOrDefault(file => file.FileName == referencedAssembly.Name); - if (ruleFileInfo is null) + if (ruleFileInfo != null) { - // If the assembly is not directly referenced by the entry assembly, assume that it is okay to proceed. - continue; - } - - var autoInstrumentationFileVersion = new Version(ruleFileInfo.FileVersion); + var autoInstrumentationFileVersion = new Version(ruleFileInfo.FileVersion); - var appInstrumentationAssembly = Assembly.Load(referencedAssembly); - var appInstrumentationFileVersionInfo = FileVersionInfo.GetVersionInfo(appInstrumentationAssembly.Location); - var appInstrumentationFileVersion = new Version(appInstrumentationFileVersionInfo.FileVersion); + var appInstrumentationAssembly = Assembly.Load(referencedAssembly); + var appInstrumentationFileVersionInfo = FileVersionInfo.GetVersionInfo(appInstrumentationAssembly.Location); + var appInstrumentationFileVersion = new Version(appInstrumentationFileVersionInfo.FileVersion); - if (appInstrumentationFileVersion < autoInstrumentationFileVersion) - { - result = false; - Logger.Error($"Rule Engine: Application has direct or indirect reference to older version of assembly {ruleFileInfo.FileName} - {ruleFileInfo.FileVersion}."); - } - else - { - Logger.Information($"Rule Engine: Application has reference to assembly {ruleFileInfo.FileName} and loaded successfully."); + if (appInstrumentationFileVersion < autoInstrumentationFileVersion) + { + result = false; + Logger.Error($"Rule Engine: Application has direct or indirect reference to older version of assembly {ruleFileInfo.FileName} - {ruleFileInfo.FileVersion}."); + } + else + { + Logger.Information($"Rule Engine: Application has reference to assembly {ruleFileInfo.FileName} and loaded successfully."); + } } } } From 59298ff46c8d02889ae060c86d0b04a473c5bcdf Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Wed, 13 Sep 2023 19:49:08 -0700 Subject: [PATCH 3/4] Update CHANGELOG.md --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 44e7b240c8..ff6aef964e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,9 @@ This component adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.h ### Fixed +- Fixed Rule checking System.Diagnostics.DiagnosticSource version for net7.0 + failing on correct configuration [#2950](https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/issues/2950). + ### Security ## [1.0.0](https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/releases/tag/v1.0.0) From 625f106663da64d399e097d811964cbcc3b64e4e Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Thu, 14 Sep 2023 15:22:10 -0700 Subject: [PATCH 4/4] Remove temporary debugging log statement --- .../RulesEngine/AssemblyFileVersionRule.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/AssemblyFileVersionRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/AssemblyFileVersionRule.cs index 41e90e38a3..f449342ccb 100644 --- a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/AssemblyFileVersionRule.cs +++ b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/AssemblyFileVersionRule.cs @@ -41,7 +41,6 @@ internal override bool Evaluate() var ruleEngineContent = File.ReadAllText(ruleEngineFileLocation); var ruleFileInfoList = JsonSerializer.Deserialize>(ruleEngineContent); var entryAssembly = Assembly.GetEntryAssembly(); - Logger.Information($"Rule Engine: Entry Assembly: {entryAssembly?.FullName}"); var referencedAssemblies = entryAssembly?.GetReferencedAssemblies(); if (referencedAssemblies == null)