[sdk-1.5.0-hotfix] Removed support for parsing custom log states using reflection (#4614)

Author: CodeBlanch

src/OpenTelemetry/CHANGELOG.md

@@ -8,6 +8,16 @@
   `IReadOnlyList` or `IEnumerable` of `KeyValuePair<string, object>`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<TState>` API when `ParseStateValues` is true and `TState`
+  does not implement either `IReadOnlyList<KeyValuePair<string, object>>` or
+  `IEnumerable<KeyValuePair<string, object>>`. 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.
+  ([#4614](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4614))
+
 ## 1.5.0
 
 Released 2023-Jun-05

src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs

@@ -158,6 +158,17 @@ public void LoggerProviderException(string methodName, Exception ex)
             }
         }
 
+        [NonEvent]
+        public void LoggerProcessStateSkipped<TState>()
+        {
+            if (this.IsEnabled(EventLevel.Warning, EventKeywords.All))
+            {
+                this.LoggerProcessStateSkipped(
+                    typeof(TState).FullName!,
+                    "because it does not implement a supported interface (either IReadOnlyList<KeyValuePair<string, object>> or IEnumerable<KeyValuePair<string, object>>)");
+            }
+        }
+
         [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
         {

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<KeyValuePair<string, object?>>(itemProperties.Count);
-
-                foreach (PropertyDescriptor? itemProperty in itemProperties)
-                {
-                    if (itemProperty == null)
-                    {
-                        continue;
-                    }
-
-                    object? value = itemProperty.GetValue(state);
-                    if (value == null)
-                    {
-                        continue;
-                    }
-
-                    attributeStorage.Add(new KeyValuePair<string, object?>(itemProperty.Name, value));
-                }
+            OpenTelemetrySdkEventSource.Log.LoggerProcessStateSkipped<TState>();
 
-                return attributeStorage;
-            }
-            catch (Exception parseException)
-            {
-                OpenTelemetrySdkEventSource.Log.LoggerParseStateException<TState>(parseException);
-
-                return Array.Empty<KeyValuePair<string, object?>>();
-            }
+            return Array.Empty<KeyValuePair<string, object?>>();
         }
     }
 

src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs

@@ -58,13 +58,21 @@ public class OpenTelemetryLoggerOptions
     /// <remarks>
     /// Notes:
     /// <list type="bullet">
-    /// <item>Parsing is only executed when the state logged does NOT
-    /// implement <see cref="IReadOnlyList{T}"/> or <see
+    /// <item>As of OpenTelemetry v1.5 state parsing is handled automatically if
+    /// the state logged implements <see cref="IReadOnlyList{T}"/> or <see
     /// cref="IEnumerable{T}"/> where <c>T</c> is <c>KeyValuePair&lt;string,
-    /// object&gt;</c>.</item>
+    /// object&gt;</c> than <see cref="LogRecord.Attributes"/> will be set
+    /// regardless of the value of <see cref="ParseStateValues"/>.</item>
     /// <item>When <see cref="ParseStateValues"/> is set to <see
     /// langword="true"/> <see cref="LogRecord.State"/> will always be <see
-    /// langword="null"/>.</item>
+    /// langword="null"/>. When <see cref="ParseStateValues"/> is set to <see
+    /// langword="false"/> <see cref="LogRecord.State"/> will always be set to
+    /// the logged state to support legacy exporters which access <see
+    /// cref="LogRecord.State"/> directly. Exporters should NOT access <see
+    /// cref="LogRecord.State"/> directly because is NOT safe and may lead to
+    /// exceptions or incorrect data especially when using batching. Exporters
+    /// should use <see cref="LogRecord.Attributes"/> to safely access any data
+    /// attached to log messages.</item>
     /// </list>
     /// </remarks>
     public bool ParseStateValues { get; set; }

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());
         }
     }
 }

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
             {

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<LogRecord> exportedItems, configure: options => options.ParseStateValues = true);
             var logger = loggerFactory.CreateLogger<LogRecordTest>();
@@ -848,12 +848,11 @@ public void ParseStateValuesUsingCustomTest()
 
             Assert.Null(logRecord.State);
             Assert.NotNull(logRecord.StateValues);
-            Assert.Equal(1, logRecord.StateValues.Count);
-
-            KeyValuePair<string, object> 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]