From a0912af721ac312ff5cf900901a44d43d760bcbb Mon Sep 17 00:00:00 2001 From: xiang17 Date: Wed, 5 Apr 2023 17:26:57 -0700 Subject: [PATCH 01/12] Add DiagnosticSource version check rule --- .../RulesEngine/DiagnosticSourceRule.cs | 48 ++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs index 4af728eb05..9cc556f2f3 100644 --- a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs +++ b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs @@ -14,13 +14,59 @@ // limitations under the License. // +using System.Diagnostics; +using System.Reflection; +using OpenTelemetry.AutoInstrumentation.Logging; + namespace OpenTelemetry.AutoInstrumentation.RulesEngine; internal class DiagnosticSourceRule : Rule { + private static readonly 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 Auto-Instrumentation"; + } + internal override bool Evaluate() { - // TODO: implement checking logic + string? diagnosticSourcePackageVersion = null; + + try + { + // Look up for type with an assembly name, will load the library. + // The loaded version depends on the app's reference to the package. + var diagnosticSourceType = Type.GetType("System.Diagnostics.DiagnosticSource, System.Diagnostics.DiagnosticSource"); + if (diagnosticSourceType != null) + { + var loadedDiagnosticSourceAssembly = Assembly.GetAssembly(diagnosticSourceType); + var loadedDiagnosticSourceFileVersionInfo = FileVersionInfo.GetVersionInfo(loadedDiagnosticSourceAssembly?.Location); + var loadedDiagnosticSourceFileVersion = new Version(loadedDiagnosticSourceFileVersionInfo.FileVersion); + + var autoInstrumentationDiagnosticSourceLocation = Path.Combine(StartupHook.LoaderAssemblyLocation ?? string.Empty, "System.Diagnostics.DiagnosticSource.dll"); + var autoInstrumentationDiagnosticSourceFileVersionInfo = FileVersionInfo.GetVersionInfo(autoInstrumentationDiagnosticSourceLocation); + var autoInstrumentationDiagnosticSourceFileVersion = new Version(autoInstrumentationDiagnosticSourceFileVersionInfo.FileVersion); + + if (loadedDiagnosticSourceFileVersion < autoInstrumentationDiagnosticSourceFileVersion) + { + diagnosticSourcePackageVersion = loadedDiagnosticSourceFileVersionInfo.FileVersion; + } + } + } + catch (Exception ex) + { + // Exception in evaluation should not throw or crash the process. + Logger.Information($"Couldn't evaluate reference to System.Diagnostics.DiagnosticSource in an app. Exception: {ex}"); + } + + if (diagnosticSourcePackageVersion != null) + { + Logger.Error($"Application has direct or indirect reference to older version of System.Diagnostics.DiagnosticSource.dll {diagnosticSourcePackageVersion}."); + return false; + } + return true; } } From 204f9b73d0dfb749237ccfb59d9f6c6a31e6e6eb Mon Sep 17 00:00:00 2001 From: xiang17 Date: Thu, 6 Apr 2023 12:00:17 -0700 Subject: [PATCH 02/12] rename variable --- .../RulesEngine/DiagnosticSourceRule.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs index 9cc556f2f3..84d2ea721c 100644 --- a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs +++ b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs @@ -32,7 +32,7 @@ public DiagnosticSourceRule() internal override bool Evaluate() { - string? diagnosticSourcePackageVersion = null; + string? olderDiagnosticSourcePackageVersion = null; try { @@ -51,7 +51,7 @@ internal override bool Evaluate() if (loadedDiagnosticSourceFileVersion < autoInstrumentationDiagnosticSourceFileVersion) { - diagnosticSourcePackageVersion = loadedDiagnosticSourceFileVersionInfo.FileVersion; + olderDiagnosticSourcePackageVersion = loadedDiagnosticSourceFileVersionInfo.FileVersion; } } } @@ -61,9 +61,9 @@ internal override bool Evaluate() Logger.Information($"Couldn't evaluate reference to System.Diagnostics.DiagnosticSource in an app. Exception: {ex}"); } - if (diagnosticSourcePackageVersion != null) + if (olderDiagnosticSourcePackageVersion != null) { - Logger.Error($"Application has direct or indirect reference to older version of System.Diagnostics.DiagnosticSource.dll {diagnosticSourcePackageVersion}."); + Logger.Error($"Application has direct or indirect reference to older version of System.Diagnostics.DiagnosticSource.dll {olderDiagnosticSourcePackageVersion}."); return false; } From 65f4f0372c7c0b22c8023a1072c9d710741c4e24 Mon Sep 17 00:00:00 2001 From: xiang17 Date: Thu, 6 Apr 2023 13:21:31 -0700 Subject: [PATCH 03/12] Get version using `Assembly.GetAssembly(diagnosticSourceType).GetCustomAttribute()` --- .../RulesEngine/DiagnosticSourceRule.cs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs index 84d2ea721c..b57f6ebc4c 100644 --- a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs +++ b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs @@ -42,16 +42,19 @@ internal override bool Evaluate() if (diagnosticSourceType != null) { var loadedDiagnosticSourceAssembly = Assembly.GetAssembly(diagnosticSourceType); - var loadedDiagnosticSourceFileVersionInfo = FileVersionInfo.GetVersionInfo(loadedDiagnosticSourceAssembly?.Location); - var loadedDiagnosticSourceFileVersion = new Version(loadedDiagnosticSourceFileVersionInfo.FileVersion); + var loadedDiagnosticSourceAssemblyFileVersionAttribute = loadedDiagnosticSourceAssembly?.GetCustomAttribute(); + if (loadedDiagnosticSourceAssemblyFileVersionAttribute != null) + { + var loadedDiagnosticSourceFileVersion = new Version(loadedDiagnosticSourceAssemblyFileVersionAttribute.Version); - var autoInstrumentationDiagnosticSourceLocation = Path.Combine(StartupHook.LoaderAssemblyLocation ?? string.Empty, "System.Diagnostics.DiagnosticSource.dll"); - var autoInstrumentationDiagnosticSourceFileVersionInfo = FileVersionInfo.GetVersionInfo(autoInstrumentationDiagnosticSourceLocation); - var autoInstrumentationDiagnosticSourceFileVersion = new Version(autoInstrumentationDiagnosticSourceFileVersionInfo.FileVersion); + var autoInstrumentationDiagnosticSourceLocation = Path.Combine(StartupHook.LoaderAssemblyLocation ?? string.Empty, "System.Diagnostics.DiagnosticSource.dll"); + var autoInstrumentationDiagnosticSourceFileVersionInfo = FileVersionInfo.GetVersionInfo(autoInstrumentationDiagnosticSourceLocation); + var autoInstrumentationDiagnosticSourceFileVersion = new Version(autoInstrumentationDiagnosticSourceFileVersionInfo.FileVersion); - if (loadedDiagnosticSourceFileVersion < autoInstrumentationDiagnosticSourceFileVersion) - { - olderDiagnosticSourcePackageVersion = loadedDiagnosticSourceFileVersionInfo.FileVersion; + if (loadedDiagnosticSourceFileVersion < autoInstrumentationDiagnosticSourceFileVersion) + { + olderDiagnosticSourcePackageVersion = loadedDiagnosticSourceAssemblyFileVersionAttribute.Version; + } } } } From 95691ff48da90c1f8c392f64387f6c7691432bb5 Mon Sep 17 00:00:00 2001 From: xiang17 Date: Thu, 6 Apr 2023 16:27:47 -0700 Subject: [PATCH 04/12] Add unit test --- .../RulesEngine/DiagnosticSourceRule.cs | 40 ++++++++----- .../DiagnosticSourceRuleTests.cs | 58 +++++++++++++++++++ 2 files changed, 82 insertions(+), 16 deletions(-) create mode 100644 test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs index b57f6ebc4c..dfc3557675 100644 --- a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs +++ b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs @@ -36,25 +36,18 @@ internal override bool Evaluate() try { - // Look up for type with an assembly name, will load the library. - // The loaded version depends on the app's reference to the package. - var diagnosticSourceType = Type.GetType("System.Diagnostics.DiagnosticSource, System.Diagnostics.DiagnosticSource"); - if (diagnosticSourceType != null) + var loadedDiagnosticSourceAssemblyVersionString = GetDiagnosticSourceVersion(); + if (loadedDiagnosticSourceAssemblyVersionString != null) { - var loadedDiagnosticSourceAssembly = Assembly.GetAssembly(diagnosticSourceType); - var loadedDiagnosticSourceAssemblyFileVersionAttribute = loadedDiagnosticSourceAssembly?.GetCustomAttribute(); - if (loadedDiagnosticSourceAssemblyFileVersionAttribute != null) - { - var loadedDiagnosticSourceFileVersion = new Version(loadedDiagnosticSourceAssemblyFileVersionAttribute.Version); + var loadedDiagnosticSourceFileVersion = new Version(loadedDiagnosticSourceAssemblyVersionString); - var autoInstrumentationDiagnosticSourceLocation = Path.Combine(StartupHook.LoaderAssemblyLocation ?? string.Empty, "System.Diagnostics.DiagnosticSource.dll"); - var autoInstrumentationDiagnosticSourceFileVersionInfo = FileVersionInfo.GetVersionInfo(autoInstrumentationDiagnosticSourceLocation); - var autoInstrumentationDiagnosticSourceFileVersion = new Version(autoInstrumentationDiagnosticSourceFileVersionInfo.FileVersion); + var autoInstrumentationDiagnosticSourceLocation = Path.Combine(StartupHook.LoaderAssemblyLocation ?? string.Empty, "System.Diagnostics.DiagnosticSource.dll"); + var autoInstrumentationDiagnosticSourceFileVersionInfo = FileVersionInfo.GetVersionInfo(autoInstrumentationDiagnosticSourceLocation); + var autoInstrumentationDiagnosticSourceFileVersion = new Version(autoInstrumentationDiagnosticSourceFileVersionInfo.FileVersion); - if (loadedDiagnosticSourceFileVersion < autoInstrumentationDiagnosticSourceFileVersion) - { - olderDiagnosticSourcePackageVersion = loadedDiagnosticSourceAssemblyFileVersionAttribute.Version; - } + if (loadedDiagnosticSourceFileVersion < autoInstrumentationDiagnosticSourceFileVersion) + { + olderDiagnosticSourcePackageVersion = loadedDiagnosticSourceAssemblyVersionString; } } } @@ -72,4 +65,19 @@ internal override bool Evaluate() return true; } + + protected virtual string? GetDiagnosticSourceVersion() + { + // Look up for type with an assembly name, will load the library. + // The loaded version depends on the app's reference to the package. + var diagnosticSourceType = Type.GetType("System.Diagnostics.DiagnosticSource, System.Diagnostics.DiagnosticSource"); + if (diagnosticSourceType != null) + { + var loadedDiagnosticSourceAssembly = Assembly.GetAssembly(diagnosticSourceType); + var loadedDiagnosticSourceAssemblyFileVersionAttribute = loadedDiagnosticSourceAssembly?.GetCustomAttribute(); + return loadedDiagnosticSourceAssemblyFileVersionAttribute?.Version; + } + + return null; + } } diff --git a/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs b/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs new file mode 100644 index 0000000000..dff88312eb --- /dev/null +++ b/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs @@ -0,0 +1,58 @@ +// +// 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.RulesEngine; +using Xunit; + +namespace OpenTelemetry.AutoInstrumentation.StartupHook.Tests; +public class DiagnosticSourceRuleTests +{ + [Theory] + // These version are got when adding line `` in Example.AspNetCoreMvc.csproj. + + // DS of major version lower than 7 + [InlineData("7.00.423.11508", true)] // Version of DLL loaded when including version 4.7.0. It was auto-upgraded to 7.0.2, version of DS in store folder. + + // DS of major version of 7 but higher than auto instrumentation's version (7.0.0) + [InlineData("7.00.22.51805", true)] // Version of DLL loaded when including version 7.0.0. + [InlineData("7.00.323.6910", true)] // Version of DLL when including version 7.0.1. + + // DS of major version of 7 but lower than the auto-upgraded version (7.0.0) + [InlineData("7.0.22.47203", false)] // Version of DLL when including version 7.0.0-rc.2.22472.3 (had to use a pre-release version because 7.0.0 is the first official version of 7) + + // DS of major version higher than 7 + [InlineData("8.00.23.12803", true)] // Version of DLL when including version 8.0.0-preview.2.23128.3 + public void DiagnosticSourceVersion(string diagnosticSourceVersion, bool result) + { + var rule = new TestableDiagnosticSourceRule(diagnosticSourceVersion); + Assert.Equal(rule.Evaluate(), result); + } +} + +internal class TestableDiagnosticSourceRule : DiagnosticSourceRule +{ + private readonly string diagnosticSourceVersion; + + public TestableDiagnosticSourceRule(string diagnosticSourceVersion) + { + this.diagnosticSourceVersion = diagnosticSourceVersion; + } + + protected override string? GetDiagnosticSourceVersion() + { + return diagnosticSourceVersion; + } +} From f9fefe4521ab9f490a7ce08c461f16bd4a4f3e2c Mon Sep 17 00:00:00 2001 From: xiang17 Date: Thu, 6 Apr 2023 18:23:52 -0700 Subject: [PATCH 05/12] Revert "Add unit test" This reverts commit 95691ff48da90c1f8c392f64387f6c7691432bb5. --- .../RulesEngine/DiagnosticSourceRule.cs | 40 +++++-------- .../DiagnosticSourceRuleTests.cs | 58 ------------------- 2 files changed, 16 insertions(+), 82 deletions(-) delete mode 100644 test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs index dfc3557675..b57f6ebc4c 100644 --- a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs +++ b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs @@ -36,18 +36,25 @@ internal override bool Evaluate() try { - var loadedDiagnosticSourceAssemblyVersionString = GetDiagnosticSourceVersion(); - if (loadedDiagnosticSourceAssemblyVersionString != null) + // Look up for type with an assembly name, will load the library. + // The loaded version depends on the app's reference to the package. + var diagnosticSourceType = Type.GetType("System.Diagnostics.DiagnosticSource, System.Diagnostics.DiagnosticSource"); + if (diagnosticSourceType != null) { - var loadedDiagnosticSourceFileVersion = new Version(loadedDiagnosticSourceAssemblyVersionString); + var loadedDiagnosticSourceAssembly = Assembly.GetAssembly(diagnosticSourceType); + var loadedDiagnosticSourceAssemblyFileVersionAttribute = loadedDiagnosticSourceAssembly?.GetCustomAttribute(); + if (loadedDiagnosticSourceAssemblyFileVersionAttribute != null) + { + var loadedDiagnosticSourceFileVersion = new Version(loadedDiagnosticSourceAssemblyFileVersionAttribute.Version); - var autoInstrumentationDiagnosticSourceLocation = Path.Combine(StartupHook.LoaderAssemblyLocation ?? string.Empty, "System.Diagnostics.DiagnosticSource.dll"); - var autoInstrumentationDiagnosticSourceFileVersionInfo = FileVersionInfo.GetVersionInfo(autoInstrumentationDiagnosticSourceLocation); - var autoInstrumentationDiagnosticSourceFileVersion = new Version(autoInstrumentationDiagnosticSourceFileVersionInfo.FileVersion); + var autoInstrumentationDiagnosticSourceLocation = Path.Combine(StartupHook.LoaderAssemblyLocation ?? string.Empty, "System.Diagnostics.DiagnosticSource.dll"); + var autoInstrumentationDiagnosticSourceFileVersionInfo = FileVersionInfo.GetVersionInfo(autoInstrumentationDiagnosticSourceLocation); + var autoInstrumentationDiagnosticSourceFileVersion = new Version(autoInstrumentationDiagnosticSourceFileVersionInfo.FileVersion); - if (loadedDiagnosticSourceFileVersion < autoInstrumentationDiagnosticSourceFileVersion) - { - olderDiagnosticSourcePackageVersion = loadedDiagnosticSourceAssemblyVersionString; + if (loadedDiagnosticSourceFileVersion < autoInstrumentationDiagnosticSourceFileVersion) + { + olderDiagnosticSourcePackageVersion = loadedDiagnosticSourceAssemblyFileVersionAttribute.Version; + } } } } @@ -65,19 +72,4 @@ internal override bool Evaluate() return true; } - - protected virtual string? GetDiagnosticSourceVersion() - { - // Look up for type with an assembly name, will load the library. - // The loaded version depends on the app's reference to the package. - var diagnosticSourceType = Type.GetType("System.Diagnostics.DiagnosticSource, System.Diagnostics.DiagnosticSource"); - if (diagnosticSourceType != null) - { - var loadedDiagnosticSourceAssembly = Assembly.GetAssembly(diagnosticSourceType); - var loadedDiagnosticSourceAssemblyFileVersionAttribute = loadedDiagnosticSourceAssembly?.GetCustomAttribute(); - return loadedDiagnosticSourceAssemblyFileVersionAttribute?.Version; - } - - return null; - } } diff --git a/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs b/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs deleted file mode 100644 index dff88312eb..0000000000 --- a/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs +++ /dev/null @@ -1,58 +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.RulesEngine; -using Xunit; - -namespace OpenTelemetry.AutoInstrumentation.StartupHook.Tests; -public class DiagnosticSourceRuleTests -{ - [Theory] - // These version are got when adding line `` in Example.AspNetCoreMvc.csproj. - - // DS of major version lower than 7 - [InlineData("7.00.423.11508", true)] // Version of DLL loaded when including version 4.7.0. It was auto-upgraded to 7.0.2, version of DS in store folder. - - // DS of major version of 7 but higher than auto instrumentation's version (7.0.0) - [InlineData("7.00.22.51805", true)] // Version of DLL loaded when including version 7.0.0. - [InlineData("7.00.323.6910", true)] // Version of DLL when including version 7.0.1. - - // DS of major version of 7 but lower than the auto-upgraded version (7.0.0) - [InlineData("7.0.22.47203", false)] // Version of DLL when including version 7.0.0-rc.2.22472.3 (had to use a pre-release version because 7.0.0 is the first official version of 7) - - // DS of major version higher than 7 - [InlineData("8.00.23.12803", true)] // Version of DLL when including version 8.0.0-preview.2.23128.3 - public void DiagnosticSourceVersion(string diagnosticSourceVersion, bool result) - { - var rule = new TestableDiagnosticSourceRule(diagnosticSourceVersion); - Assert.Equal(rule.Evaluate(), result); - } -} - -internal class TestableDiagnosticSourceRule : DiagnosticSourceRule -{ - private readonly string diagnosticSourceVersion; - - public TestableDiagnosticSourceRule(string diagnosticSourceVersion) - { - this.diagnosticSourceVersion = diagnosticSourceVersion; - } - - protected override string? GetDiagnosticSourceVersion() - { - return diagnosticSourceVersion; - } -} From 3ee1f631eef782515a25d03f595a545811a76cdf Mon Sep 17 00:00:00 2001 From: xiang17 Date: Thu, 6 Apr 2023 16:27:47 -0700 Subject: [PATCH 06/12] Add unit test --- .../RulesEngine/DiagnosticSourceRule.cs | 40 ++++++++----- .../DiagnosticSourceRuleTests.cs | 58 +++++++++++++++++++ 2 files changed, 82 insertions(+), 16 deletions(-) create mode 100644 test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs index b57f6ebc4c..dfc3557675 100644 --- a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs +++ b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs @@ -36,25 +36,18 @@ internal override bool Evaluate() try { - // Look up for type with an assembly name, will load the library. - // The loaded version depends on the app's reference to the package. - var diagnosticSourceType = Type.GetType("System.Diagnostics.DiagnosticSource, System.Diagnostics.DiagnosticSource"); - if (diagnosticSourceType != null) + var loadedDiagnosticSourceAssemblyVersionString = GetDiagnosticSourceVersion(); + if (loadedDiagnosticSourceAssemblyVersionString != null) { - var loadedDiagnosticSourceAssembly = Assembly.GetAssembly(diagnosticSourceType); - var loadedDiagnosticSourceAssemblyFileVersionAttribute = loadedDiagnosticSourceAssembly?.GetCustomAttribute(); - if (loadedDiagnosticSourceAssemblyFileVersionAttribute != null) - { - var loadedDiagnosticSourceFileVersion = new Version(loadedDiagnosticSourceAssemblyFileVersionAttribute.Version); + var loadedDiagnosticSourceFileVersion = new Version(loadedDiagnosticSourceAssemblyVersionString); - var autoInstrumentationDiagnosticSourceLocation = Path.Combine(StartupHook.LoaderAssemblyLocation ?? string.Empty, "System.Diagnostics.DiagnosticSource.dll"); - var autoInstrumentationDiagnosticSourceFileVersionInfo = FileVersionInfo.GetVersionInfo(autoInstrumentationDiagnosticSourceLocation); - var autoInstrumentationDiagnosticSourceFileVersion = new Version(autoInstrumentationDiagnosticSourceFileVersionInfo.FileVersion); + var autoInstrumentationDiagnosticSourceLocation = Path.Combine(StartupHook.LoaderAssemblyLocation ?? string.Empty, "System.Diagnostics.DiagnosticSource.dll"); + var autoInstrumentationDiagnosticSourceFileVersionInfo = FileVersionInfo.GetVersionInfo(autoInstrumentationDiagnosticSourceLocation); + var autoInstrumentationDiagnosticSourceFileVersion = new Version(autoInstrumentationDiagnosticSourceFileVersionInfo.FileVersion); - if (loadedDiagnosticSourceFileVersion < autoInstrumentationDiagnosticSourceFileVersion) - { - olderDiagnosticSourcePackageVersion = loadedDiagnosticSourceAssemblyFileVersionAttribute.Version; - } + if (loadedDiagnosticSourceFileVersion < autoInstrumentationDiagnosticSourceFileVersion) + { + olderDiagnosticSourcePackageVersion = loadedDiagnosticSourceAssemblyVersionString; } } } @@ -72,4 +65,19 @@ internal override bool Evaluate() return true; } + + protected virtual string? GetDiagnosticSourceVersion() + { + // Look up for type with an assembly name, will load the library. + // The loaded version depends on the app's reference to the package. + var diagnosticSourceType = Type.GetType("System.Diagnostics.DiagnosticSource, System.Diagnostics.DiagnosticSource"); + if (diagnosticSourceType != null) + { + var loadedDiagnosticSourceAssembly = Assembly.GetAssembly(diagnosticSourceType); + var loadedDiagnosticSourceAssemblyFileVersionAttribute = loadedDiagnosticSourceAssembly?.GetCustomAttribute(); + return loadedDiagnosticSourceAssemblyFileVersionAttribute?.Version; + } + + return null; + } } diff --git a/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs b/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs new file mode 100644 index 0000000000..dff88312eb --- /dev/null +++ b/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs @@ -0,0 +1,58 @@ +// +// 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.RulesEngine; +using Xunit; + +namespace OpenTelemetry.AutoInstrumentation.StartupHook.Tests; +public class DiagnosticSourceRuleTests +{ + [Theory] + // These version are got when adding line `` in Example.AspNetCoreMvc.csproj. + + // DS of major version lower than 7 + [InlineData("7.00.423.11508", true)] // Version of DLL loaded when including version 4.7.0. It was auto-upgraded to 7.0.2, version of DS in store folder. + + // DS of major version of 7 but higher than auto instrumentation's version (7.0.0) + [InlineData("7.00.22.51805", true)] // Version of DLL loaded when including version 7.0.0. + [InlineData("7.00.323.6910", true)] // Version of DLL when including version 7.0.1. + + // DS of major version of 7 but lower than the auto-upgraded version (7.0.0) + [InlineData("7.0.22.47203", false)] // Version of DLL when including version 7.0.0-rc.2.22472.3 (had to use a pre-release version because 7.0.0 is the first official version of 7) + + // DS of major version higher than 7 + [InlineData("8.00.23.12803", true)] // Version of DLL when including version 8.0.0-preview.2.23128.3 + public void DiagnosticSourceVersion(string diagnosticSourceVersion, bool result) + { + var rule = new TestableDiagnosticSourceRule(diagnosticSourceVersion); + Assert.Equal(rule.Evaluate(), result); + } +} + +internal class TestableDiagnosticSourceRule : DiagnosticSourceRule +{ + private readonly string diagnosticSourceVersion; + + public TestableDiagnosticSourceRule(string diagnosticSourceVersion) + { + this.diagnosticSourceVersion = diagnosticSourceVersion; + } + + protected override string? GetDiagnosticSourceVersion() + { + return diagnosticSourceVersion; + } +} From 6a3162a30ca9eb8aabaa540a4e95fb5772793fcd Mon Sep 17 00:00:00 2001 From: xiang17 Date: Mon, 10 Apr 2023 13:36:57 -0700 Subject: [PATCH 07/12] Updated unit tests --- .../RulesEngine/DiagnosticSourceRule.cs | 29 ++++++++------ .../DiagnosticSourceRuleTests.cs | 38 +++++++++---------- 2 files changed, 35 insertions(+), 32 deletions(-) diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs index dfc3557675..a902738c79 100644 --- a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs +++ b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs @@ -36,18 +36,13 @@ internal override bool Evaluate() try { - var loadedDiagnosticSourceAssemblyVersionString = GetDiagnosticSourceVersion(); - if (loadedDiagnosticSourceAssemblyVersionString != null) + var loadedDiagnosticSourceFileVersion = GetVersionFromApp(); + if (loadedDiagnosticSourceFileVersion != null) { - var loadedDiagnosticSourceFileVersion = new Version(loadedDiagnosticSourceAssemblyVersionString); - - var autoInstrumentationDiagnosticSourceLocation = Path.Combine(StartupHook.LoaderAssemblyLocation ?? string.Empty, "System.Diagnostics.DiagnosticSource.dll"); - var autoInstrumentationDiagnosticSourceFileVersionInfo = FileVersionInfo.GetVersionInfo(autoInstrumentationDiagnosticSourceLocation); - var autoInstrumentationDiagnosticSourceFileVersion = new Version(autoInstrumentationDiagnosticSourceFileVersionInfo.FileVersion); - + var autoInstrumentationDiagnosticSourceFileVersion = GetVersionFromAutoInstrumentation(); if (loadedDiagnosticSourceFileVersion < autoInstrumentationDiagnosticSourceFileVersion) { - olderDiagnosticSourcePackageVersion = loadedDiagnosticSourceAssemblyVersionString; + olderDiagnosticSourcePackageVersion = autoInstrumentationDiagnosticSourceFileVersion.ToString(); } } } @@ -66,7 +61,7 @@ internal override bool Evaluate() return true; } - protected virtual string? GetDiagnosticSourceVersion() + protected virtual Version? GetVersionFromApp() { // Look up for type with an assembly name, will load the library. // The loaded version depends on the app's reference to the package. @@ -75,9 +70,21 @@ internal override bool Evaluate() { var loadedDiagnosticSourceAssembly = Assembly.GetAssembly(diagnosticSourceType); var loadedDiagnosticSourceAssemblyFileVersionAttribute = loadedDiagnosticSourceAssembly?.GetCustomAttribute(); - return loadedDiagnosticSourceAssemblyFileVersionAttribute?.Version; + 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 autoInstrumentationDiagnosticSourceFileVersionInfo = FileVersionInfo.GetVersionInfo(autoInstrumentationDiagnosticSourceLocation); + var autoInstrumentationDiagnosticSourceFileVersion = new Version(autoInstrumentationDiagnosticSourceFileVersionInfo.FileVersion); + return autoInstrumentationDiagnosticSourceFileVersion; + } } diff --git a/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs b/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs index dff88312eb..8486789827 100644 --- a/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs +++ b/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs @@ -21,38 +21,34 @@ namespace OpenTelemetry.AutoInstrumentation.StartupHook.Tests; public class DiagnosticSourceRuleTests { [Theory] - // These version are got when adding line `` in Example.AspNetCoreMvc.csproj. - - // DS of major version lower than 7 - [InlineData("7.00.423.11508", true)] // Version of DLL loaded when including version 4.7.0. It was auto-upgraded to 7.0.2, version of DS in store folder. - - // DS of major version of 7 but higher than auto instrumentation's version (7.0.0) - [InlineData("7.00.22.51805", true)] // Version of DLL loaded when including version 7.0.0. - [InlineData("7.00.323.6910", true)] // Version of DLL when including version 7.0.1. - - // DS of major version of 7 but lower than the auto-upgraded version (7.0.0) - [InlineData("7.0.22.47203", false)] // Version of DLL when including version 7.0.0-rc.2.22472.3 (had to use a pre-release version because 7.0.0 is the first official version of 7) - - // DS of major version higher than 7 - [InlineData("8.00.23.12803", true)] // Version of DLL when including version 8.0.0-preview.2.23128.3 - public void DiagnosticSourceVersion(string diagnosticSourceVersion, bool result) + [InlineData("6.0.0.0", "7.0.0.0", false)] + [InlineData("8.0.0.0", "7.0.0.0", true)] + [InlineData("7.0.0.0", "7.0.0.0", true)] + public void DiagnosticSourceVersion(string appVersion, string autoInstrumentationVersion, bool result) { - var rule = new TestableDiagnosticSourceRule(diagnosticSourceVersion); + var rule = new TestableDiagnosticSourceRule(appVersion, autoInstrumentationVersion); Assert.Equal(rule.Evaluate(), result); } } internal class TestableDiagnosticSourceRule : DiagnosticSourceRule { - private readonly string diagnosticSourceVersion; + private readonly string appVersion; + private readonly string autoInstrumentationVersion; + + public TestableDiagnosticSourceRule(string appVersion, string autoInstrumentationVersion) + { + this.appVersion = appVersion; + this.autoInstrumentationVersion = autoInstrumentationVersion; + } - public TestableDiagnosticSourceRule(string diagnosticSourceVersion) + protected override Version? GetVersionFromApp() { - this.diagnosticSourceVersion = diagnosticSourceVersion; + return new Version(appVersion); } - protected override string? GetDiagnosticSourceVersion() + protected override Version? GetVersionFromAutoInstrumentation() { - return diagnosticSourceVersion; + return new Version(autoInstrumentationVersion); } } From 893c25cea598b1987ac7fac0aa690386528b31ea Mon Sep 17 00:00:00 2001 From: xiang17 Date: Mon, 10 Apr 2023 13:45:28 -0700 Subject: [PATCH 08/12] Log warning for unexpected exception in DLL evaluation --- .../RulesEngine/DiagnosticSourceRule.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs index a902738c79..3ff765ac35 100644 --- a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs +++ b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs @@ -49,7 +49,7 @@ internal override bool Evaluate() catch (Exception ex) { // Exception in evaluation should not throw or crash the process. - Logger.Information($"Couldn't evaluate reference to System.Diagnostics.DiagnosticSource in an app. Exception: {ex}"); + Logger.Warning($"Couldn't evaluate reference to System.Diagnostics.DiagnosticSource in an app. Exception: {ex}"); } if (olderDiagnosticSourcePackageVersion != null) From 123b094176d659b221297d8c78e1764c904afa45 Mon Sep 17 00:00:00 2001 From: xiang17 Date: Tue, 11 Apr 2023 14:20:54 -0700 Subject: [PATCH 09/12] Include testing for logging information --- .../RulesEngine/DiagnosticSourceRule.cs | 17 +++++++--- .../DiagnosticSourceRuleTests.cs | 31 ++++++++++++++----- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs index 3ff765ac35..997c0aed91 100644 --- a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs +++ b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs @@ -22,7 +22,7 @@ namespace OpenTelemetry.AutoInstrumentation.RulesEngine; internal class DiagnosticSourceRule : Rule { - private static readonly IOtelLogger Logger = OtelLogging.GetLogger("StartupHook"); + private static IOtelLogger logger = OtelLogging.GetLogger("StartupHook"); public DiagnosticSourceRule() { @@ -30,6 +30,13 @@ public DiagnosticSourceRule() Description = "Ensure that the System.Diagnostics.DiagnosticSource version is not older than the version used by the Auto-Instrumentation"; } + // This constructor is used for test purpose. + protected DiagnosticSourceRule(IOtelLogger otelLogger) + : this() + { + logger = otelLogger; + } + internal override bool Evaluate() { string? olderDiagnosticSourcePackageVersion = null; @@ -42,22 +49,24 @@ internal override bool Evaluate() var autoInstrumentationDiagnosticSourceFileVersion = GetVersionFromAutoInstrumentation(); if (loadedDiagnosticSourceFileVersion < autoInstrumentationDiagnosticSourceFileVersion) { - olderDiagnosticSourcePackageVersion = autoInstrumentationDiagnosticSourceFileVersion.ToString(); + olderDiagnosticSourcePackageVersion = loadedDiagnosticSourceFileVersion.ToString(); } } } catch (Exception ex) { // Exception in evaluation should not throw or crash the process. - Logger.Warning($"Couldn't evaluate reference to System.Diagnostics.DiagnosticSource in an app. Exception: {ex}"); + logger.Warning($"Rule Engine: Couldn't evaluate reference to System.Diagnostics.DiagnosticSource in an app. Exception: {ex}"); + return false; } if (olderDiagnosticSourcePackageVersion != null) { - Logger.Error($"Application has direct or indirect reference to older version of System.Diagnostics.DiagnosticSource.dll {olderDiagnosticSourcePackageVersion}."); + 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; } diff --git a/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs b/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs index 8486789827..1214b1d629 100644 --- a/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs +++ b/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs @@ -14,6 +14,7 @@ // limitations under the License. // +using OpenTelemetry.AutoInstrumentation.Logging; using OpenTelemetry.AutoInstrumentation.RulesEngine; using Xunit; @@ -21,13 +22,18 @@ namespace OpenTelemetry.AutoInstrumentation.StartupHook.Tests; public class DiagnosticSourceRuleTests { [Theory] - [InlineData("6.0.0.0", "7.0.0.0", false)] - [InlineData("8.0.0.0", "7.0.0.0", true)] - [InlineData("7.0.0.0", "7.0.0.0", true)] - public void DiagnosticSourceVersion(string appVersion, string autoInstrumentationVersion, bool result) + [InlineData("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)] + [InlineData("8.0.0.0", "7.0.0.0", "Rule Engine: DiagnosticSourceRule evaluation success.", true)] + [InlineData("7.0.0.0", "7.0.0.0", "Rule Engine: DiagnosticSourceRule evaluation success.", true)] + [InlineData(null, "7.0.0.0", "Rule Engine: DiagnosticSourceRule evaluation success.", true)] + [InlineData("7.0.0.0", null, "Rule Engine: DiagnosticSourceRule evaluation success.", true)] + [InlineData(null, null, "Rule Engine: DiagnosticSourceRule evaluation success.", true)] + public void DiagnosticSourceVersion(string appVersion, string autoInstrumentationVersion, string logMessage, bool result) { - var rule = new TestableDiagnosticSourceRule(appVersion, autoInstrumentationVersion); - Assert.Equal(rule.Evaluate(), result); + var logger = new TestLogger(); + var rule = new TestableDiagnosticSourceRule(appVersion, autoInstrumentationVersion, logger); + Assert.Equal(result, rule.Evaluate()); + Assert.Equal(logMessage, logger.LogRecords[0].Message); } } @@ -36,7 +42,8 @@ internal class TestableDiagnosticSourceRule : DiagnosticSourceRule private readonly string appVersion; private readonly string autoInstrumentationVersion; - public TestableDiagnosticSourceRule(string appVersion, string autoInstrumentationVersion) + public TestableDiagnosticSourceRule(string appVersion, string autoInstrumentationVersion, IOtelLogger otelLogger) + : base(otelLogger) { this.appVersion = appVersion; this.autoInstrumentationVersion = autoInstrumentationVersion; @@ -44,11 +51,21 @@ public TestableDiagnosticSourceRule(string appVersion, string autoInstrumentatio protected override Version? GetVersionFromApp() { + if (appVersion == null) + { + return null; + } + return new Version(appVersion); } protected override Version? GetVersionFromAutoInstrumentation() { + if (autoInstrumentationVersion == null) + { + return null; + } + return new Version(autoInstrumentationVersion); } } From 65c6d2f82818770132f039ab9c9aa9b86564ff05 Mon Sep 17 00:00:00 2001 From: xiang17 Date: Tue, 11 Apr 2023 14:28:50 -0700 Subject: [PATCH 10/12] Add a comment on why AssemblyFileVersion is used for comparing versions --- .../RulesEngine/DiagnosticSourceRule.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs index 997c0aed91..2a3c8f7d1a 100644 --- a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs +++ b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs @@ -43,6 +43,8 @@ internal override bool Evaluate() 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 = GetVersionFromApp(); if (loadedDiagnosticSourceFileVersion != null) { From 5832d8230331a23166ccb0a05ea0255d91bf9486 Mon Sep 17 00:00:00 2001 From: xiang17 Date: Tue, 11 Apr 2023 15:17:18 -0700 Subject: [PATCH 11/12] Use `MemberData` instead of `InlineData` --- .../DiagnosticSourceRuleTests.cs | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs b/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs index 1214b1d629..0faf8a59a7 100644 --- a/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs +++ b/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs @@ -21,13 +21,21 @@ 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 }, + new object[] { "7.0.0.0", null, "Rule Engine: DiagnosticSourceRule evaluation success.", true }, + new object[] { null, null, "Rule Engine: DiagnosticSourceRule evaluation success.", true }, + }; +#pragma warning restore CS8625 // Cannot convert null literal to non-nullable reference type. + [Theory] - [InlineData("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)] - [InlineData("8.0.0.0", "7.0.0.0", "Rule Engine: DiagnosticSourceRule evaluation success.", true)] - [InlineData("7.0.0.0", "7.0.0.0", "Rule Engine: DiagnosticSourceRule evaluation success.", true)] - [InlineData(null, "7.0.0.0", "Rule Engine: DiagnosticSourceRule evaluation success.", true)] - [InlineData("7.0.0.0", null, "Rule Engine: DiagnosticSourceRule evaluation success.", true)] - [InlineData(null, null, "Rule Engine: DiagnosticSourceRule evaluation success.", true)] + [MemberData(nameof(RuleTestData))] public void DiagnosticSourceVersion(string appVersion, string autoInstrumentationVersion, string logMessage, bool result) { var logger = new TestLogger(); From 5c1c538020a8b140874c6d82321573819fd7e98a Mon Sep 17 00:00:00 2001 From: xiang17 Date: Tue, 11 Apr 2023 17:36:01 -0700 Subject: [PATCH 12/12] PR feedbacks --- .../RulesEngine/DiagnosticSourceRule.cs | 17 ++++++++++------- .../DiagnosticSourceRuleTests.cs | 11 ++--------- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs index 2a3c8f7d1a..7c3ef8f146 100644 --- a/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs +++ b/src/OpenTelemetry.AutoInstrumentation.StartupHook/RulesEngine/DiagnosticSourceRule.cs @@ -45,7 +45,7 @@ internal override bool Evaluate() { // 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 = GetVersionFromApp(); + var loadedDiagnosticSourceFileVersion = GetResolvedVersion(); if (loadedDiagnosticSourceFileVersion != null) { var autoInstrumentationDiagnosticSourceFileVersion = GetVersionFromAutoInstrumentation(); @@ -58,7 +58,7 @@ internal override bool Evaluate() catch (Exception ex) { // Exception in evaluation should not throw or crash the process. - logger.Warning($"Rule Engine: Couldn't evaluate reference to System.Diagnostics.DiagnosticSource in an app. Exception: {ex}"); + logger.Warning($"Rule Engine: Couldn't evaluate System.Diagnostics.DiagnosticSource version. Exception: {ex}"); return false; } @@ -72,10 +72,9 @@ internal override bool Evaluate() return true; } - protected virtual Version? GetVersionFromApp() + protected virtual Version? GetResolvedVersion() { // Look up for type with an assembly name, will load the library. - // The loaded version depends on the app's reference to the package. var diagnosticSourceType = Type.GetType("System.Diagnostics.DiagnosticSource, System.Diagnostics.DiagnosticSource"); if (diagnosticSourceType != null) { @@ -91,11 +90,15 @@ internal override bool Evaluate() return null; } - protected virtual Version? GetVersionFromAutoInstrumentation() + protected virtual Version GetVersionFromAutoInstrumentation() { var autoInstrumentationDiagnosticSourceLocation = Path.Combine(StartupHook.LoaderAssemblyLocation ?? string.Empty, "System.Diagnostics.DiagnosticSource.dll"); - var autoInstrumentationDiagnosticSourceFileVersionInfo = FileVersionInfo.GetVersionInfo(autoInstrumentationDiagnosticSourceLocation); - var autoInstrumentationDiagnosticSourceFileVersion = new Version(autoInstrumentationDiagnosticSourceFileVersionInfo.FileVersion); + var fileVersionInfo = FileVersionInfo.GetVersionInfo(autoInstrumentationDiagnosticSourceLocation); + var autoInstrumentationDiagnosticSourceFileVersion = new Version( + fileVersionInfo.FileMajorPart, + fileVersionInfo.FileMinorPart, + fileVersionInfo.FileBuildPart, + fileVersionInfo.FilePrivatePart); return autoInstrumentationDiagnosticSourceFileVersion; } } diff --git a/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs b/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs index 0faf8a59a7..1ea5e93f52 100644 --- a/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs +++ b/test/OpenTelemetry.AutoInstrumentation.StartupHook.Tests/DiagnosticSourceRuleTests.cs @@ -29,8 +29,6 @@ public class DiagnosticSourceRuleTests 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 }, - new object[] { "7.0.0.0", null, "Rule Engine: DiagnosticSourceRule evaluation success.", true }, - new object[] { null, null, "Rule Engine: DiagnosticSourceRule evaluation success.", true }, }; #pragma warning restore CS8625 // Cannot convert null literal to non-nullable reference type. @@ -57,7 +55,7 @@ public TestableDiagnosticSourceRule(string appVersion, string autoInstrumentatio this.autoInstrumentationVersion = autoInstrumentationVersion; } - protected override Version? GetVersionFromApp() + protected override Version? GetResolvedVersion() { if (appVersion == null) { @@ -67,13 +65,8 @@ public TestableDiagnosticSourceRule(string appVersion, string autoInstrumentatio return new Version(appVersion); } - protected override Version? GetVersionFromAutoInstrumentation() + protected override Version GetVersionFromAutoInstrumentation() { - if (autoInstrumentationVersion == null) - { - return null; - } - return new Version(autoInstrumentationVersion); } }