From 563c3fa634259671453e71f8a64d563e5333eb59 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Mon, 26 Jun 2023 10:49:02 -0700 Subject: [PATCH 1/5] Removed support for parsing custom log states using reflection. --- src/OpenTelemetry/CHANGELOG.md | 10 ++++++ .../Internal/OpenTelemetrySdkEventSource.cs | 17 ++++++++++ .../Logs/ILogger/OpenTelemetryLogger.cs | 33 ++----------------- .../AotCompatibilityTests.cs | 2 +- .../OtlpLogExporterTests.cs | 12 +++++-- .../OpenTelemetry.Tests/Logs/LogRecordTest.cs | 11 +++---- 6 files changed, 46 insertions(+), 39 deletions(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 5eee0c5b453..d3037b2c043 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -8,6 +8,16 @@ `IReadOnlyList` or `IEnumerable` of `KeyValuePair`s. ([#4609](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4609)) +* **Breaking Change** Removed the support for parsing `TState` types passed to + the `ILogger.Log` API when `ParseStateValues` is true and `TState` + does not implement either `IReadOnlyList>` or + `IEnumerable>`. This feature was first introduced + in the `1.5.0` stable release with + [#4334](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4334) and + has been removed because it makes the OpenTelemetry .NET SDK incompatible with + native AOT. + ([#XXXX](https://github.com/open-telemetry/opentelemetry-dotnet/pull/XXXX)) + ## 1.5.0 Released 2023-Jun-05 diff --git a/src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs b/src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs index a13bc1adba0..f456b1bd0e0 100644 --- a/src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs +++ b/src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs @@ -158,6 +158,17 @@ public void LoggerProviderException(string methodName, Exception ex) } } + [NonEvent] + public void LoggerProcessStateSkipped() + { + if (this.IsEnabled(EventLevel.Warning, EventKeywords.All)) + { + this.LoggerProcessStateSkipped( + typeof(TState).FullName!, + "because it does not implement either IReadOnlyList> or IEnumerable>"); + } + } + [Event(4, Message = "Unknown error in SpanProcessor event '{0}': '{1}'.", Level = EventLevel.Error)] public void SpanProcessorException(string evnt, string ex) { @@ -325,6 +336,12 @@ public void LoggerProviderException(string methodName, string ex) this.WriteEvent(50, methodName, ex); } + [Event(51, Message = "Skipped processing log state of type '{0}' {1}.", Level = EventLevel.Warning)] + public void LoggerProcessStateSkipped(string type, string reason) + { + this.WriteEvent(51, type, reason); + } + #if DEBUG public class OpenTelemetryEventListener : EventListener { diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs index e66c548d2bc..9d15e3f1355 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs @@ -16,7 +16,6 @@ #nullable enable -using System.ComponentModel; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Runtime.CompilerServices; @@ -207,6 +206,7 @@ internal static void SetLogRecordSeverityFields(ref LogRecordData logRecordData, // Note: This is to preserve legacy behavior where State is // exposed if we didn't parse state. iLoggerData.State = state; + return null; } else @@ -215,36 +215,9 @@ internal static void SetLogRecordSeverityFields(ref LogRecordData logRecordData, // have come from the pool and may have State from a prior log. iLoggerData.State = null; - try - { - PropertyDescriptorCollection itemProperties = TypeDescriptor.GetProperties(state!); - - var attributeStorage = logRecord.AttributeStorage ??= new List>(itemProperties.Count); - - foreach (PropertyDescriptor? itemProperty in itemProperties) - { - if (itemProperty == null) - { - continue; - } - - object? value = itemProperty.GetValue(state); - if (value == null) - { - continue; - } - - attributeStorage.Add(new KeyValuePair(itemProperty.Name, value)); - } + OpenTelemetrySdkEventSource.Log.LoggerProcessStateSkipped(); - return attributeStorage; - } - catch (Exception parseException) - { - OpenTelemetrySdkEventSource.Log.LoggerParseStateException(parseException); - - return Array.Empty>(); - } + return Array.Empty>(); } } diff --git a/test/OpenTelemetry.AotCompatibility.Tests/AotCompatibilityTests.cs b/test/OpenTelemetry.AotCompatibility.Tests/AotCompatibilityTests.cs index 2b9cf77d9c3..0e54f129a10 100644 --- a/test/OpenTelemetry.AotCompatibility.Tests/AotCompatibilityTests.cs +++ b/test/OpenTelemetry.AotCompatibility.Tests/AotCompatibilityTests.cs @@ -85,7 +85,7 @@ public void EnsureAotCompatibility() Assert.True(process.ExitCode == 0, "Publishing the AotCompatibility app failed. See test output for more details."); var warnings = expectedOutput.ToString().Split('\n', '\r').Where(line => line.Contains("warning IL")); - Assert.Equal(43, warnings.Count()); + Assert.Equal(42, warnings.Count()); } } } diff --git a/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs b/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs index d9e79fee60d..b5dd80f329d 100644 --- a/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs +++ b/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs @@ -100,7 +100,11 @@ public void AddOtlpLogExporterParseStateValueCanBeTurnedOff(bool parseState) { Assert.Null(logRecord.State); Assert.NotNull(logRecord.Attributes); - Assert.Contains(logRecord.Attributes, kvp => kvp.Key == "propertyA" && (string)kvp.Value == "valueA"); + + // Note: We currently do not support parsing custom states which do + // not implement the standard interfaces. We return empty attributes + // for these. + Assert.Empty(logRecord.Attributes); } else { @@ -140,7 +144,11 @@ public void AddOtlpLogExporterParseStateValueCanBeTurnedOffHosting(bool parseSta { Assert.Null(logRecord.State); Assert.NotNull(logRecord.Attributes); - Assert.Contains(logRecord.Attributes, kvp => kvp.Key == "propertyA" && (string)kvp.Value == "valueA"); + + // Note: We currently do not support parsing custom states which do + // not implement the standard interfaces. We return empty attributes + // for these. + Assert.Empty(logRecord.Attributes); } else { diff --git a/test/OpenTelemetry.Tests/Logs/LogRecordTest.cs b/test/OpenTelemetry.Tests/Logs/LogRecordTest.cs index a9861d03aca..a828e7af63e 100644 --- a/test/OpenTelemetry.Tests/Logs/LogRecordTest.cs +++ b/test/OpenTelemetry.Tests/Logs/LogRecordTest.cs @@ -826,7 +826,7 @@ public void ParseStateValuesUsingIEnumerableTest() } [Fact] - public void ParseStateValuesUsingCustomTest() + public void ParseStateValuesUsingNonconformingCustomTypeTest() { using var loggerFactory = InitializeLoggerFactory(out List exportedItems, configure: options => options.ParseStateValues = true); var logger = loggerFactory.CreateLogger(); @@ -848,12 +848,11 @@ public void ParseStateValuesUsingCustomTest() Assert.Null(logRecord.State); Assert.NotNull(logRecord.StateValues); - Assert.Equal(1, logRecord.StateValues.Count); - - KeyValuePair actualState = logRecord.StateValues[0]; - Assert.Equal("Property", actualState.Key); - Assert.Equal("Value", actualState.Value); + // Note: We currently do not support parsing custom states which do + // not implement the standard interfaces. We return empty attributes + // for these. + Assert.Empty(logRecord.StateValues); } [Fact] From 07dcf7a02fbb52fe7ae19102b511c1853dccfbae Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Mon, 26 Jun 2023 11:20:04 -0700 Subject: [PATCH 2/5] Updated the XML comments for OpenTelemetryLoggerOptions.ParseStateValues. --- .../Logs/ILogger/OpenTelemetryLoggerOptions.cs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs index f2a392e52e7..3cb16f8d922 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs @@ -58,13 +58,21 @@ public class OpenTelemetryLoggerOptions /// /// Notes: /// - /// Parsing is only executed when the state logged does NOT - /// implement or As of OpenTelemetry 1.5.0 state parsing is handled automatically + /// if the state logged implements or where T is KeyValuePair<string, - /// object>. + /// object> than will be set + /// regardless of the value of . /// When is set to will always be . + /// langword="null"/>. When is set to will always be set to + /// the logged state to support legacy exporters which access directly. Exporters should NOT access directly because is NOT safe and may lead to + /// exceptions or incorrect data especially when using batching. Exporters + /// should use to safely access any data + /// attached to log messages. /// /// public bool ParseStateValues { get; set; } From d48bfaec8c4c4c727f0ff4523167149f287fc668 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Mon, 26 Jun 2023 11:20:27 -0700 Subject: [PATCH 3/5] CHANGELOG patch. --- src/OpenTelemetry/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index d3037b2c043..b2aa7b15ea9 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -16,7 +16,7 @@ [#4334](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4334) and has been removed because it makes the OpenTelemetry .NET SDK incompatible with native AOT. - ([#XXXX](https://github.com/open-telemetry/opentelemetry-dotnet/pull/XXXX)) + ([#4614](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4614)) ## 1.5.0 From a636402fba0579c096df94bd654f6d2b46cccff1 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Mon, 26 Jun 2023 11:22:59 -0700 Subject: [PATCH 4/5] Code review. --- src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs b/src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs index f456b1bd0e0..db4aff9ce65 100644 --- a/src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs +++ b/src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs @@ -165,7 +165,7 @@ public void LoggerProcessStateSkipped() { this.LoggerProcessStateSkipped( typeof(TState).FullName!, - "because it does not implement either IReadOnlyList> or IEnumerable>"); + "because it does not implement a supported interface (either IReadOnlyList> or IEnumerable>)"); } } From ec1676454138860d5cd8ac9f8654ed976b48d9a6 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Mon, 26 Jun 2023 12:27:49 -0700 Subject: [PATCH 5/5] Tweak. --- src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs index 3cb16f8d922..d3ff3681f2a 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs @@ -58,8 +58,8 @@ public class OpenTelemetryLoggerOptions /// /// Notes: /// - /// As of OpenTelemetry 1.5.0 state parsing is handled automatically - /// if the state logged implements or As of OpenTelemetry v1.5 state parsing is handled automatically if + /// the state logged implements or where T is KeyValuePair<string, /// object> than will be set /// regardless of the value of .