[sdk] Merge 1.5.1 fixes to main (1.6 dev) (#4619)

CodeBlanch · web-flow · commit 104cf32d97b2 · 2023-06-28T15:55:49.000-07:00
diff --git a/src/OpenTelemetry.Api.ProviderBuilderExtensions/CHANGELOG.md b/src/OpenTelemetry.Api.ProviderBuilderExtensions/CHANGELOG.md
@@ -7,6 +7,10 @@
   implementation (eg `LoggerProviderBuilder`) with dependency injection.
   ([#4433](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4433))
 
+## 1.5.1
+
+Released 2023-Jun-26
+
 ## 1.5.0
 
 Released 2023-Jun-05
diff --git a/src/OpenTelemetry.Api/CHANGELOG.md b/src/OpenTelemetry.Api/CHANGELOG.md
@@ -19,6 +19,10 @@
   implementation (`LoggerProviderBuilder`, `LoggerProvider`, `Logger`, etc.).
   ([#4433](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4433))
 
+## 1.5.1
+
+Released 2023-Jun-26
+
 ## 1.5.0
 
 Released 2023-Jun-05
diff --git a/src/OpenTelemetry.Api/Logs/LogRecordAttributeList.cs b/src/OpenTelemetry.Api/Logs/LogRecordAttributeList.cs
@@ -32,6 +32,7 @@ public struct LogRecordAttributeList : IReadOnlyList<KeyValuePair<string, object
     internal const int OverflowMaxCount = 8;
     internal const int OverflowAdditionalCapacity = 16;
     internal List<KeyValuePair<string, object?>>? OverflowAttributes;
+    private static readonly IReadOnlyList<KeyValuePair<string, object?>> Empty = Array.Empty<KeyValuePair<string, object?>>();
     private KeyValuePair<string, object?> attribute1;
     private KeyValuePair<string, object?> attribute2;
     private KeyValuePair<string, object?> attribute3;
@@ -207,12 +208,12 @@ public readonly Enumerator GetEnumerator()
     /// <inheritdoc/>
     readonly IEnumerator IEnumerable.GetEnumerator() => this.GetEnumerator();
 
-    internal readonly List<KeyValuePair<string, object?>>? Export(ref List<KeyValuePair<string, object?>>? attributeStorage)
+    internal readonly IReadOnlyList<KeyValuePair<string, object?>> Export(ref List<KeyValuePair<string, object?>>? attributeStorage)
     {
         int count = this.count;
         if (count <= 0)
         {
-            return null;
+            return Empty;
         }
 
         var overflowAttributes = this.OverflowAttributes;
diff --git a/src/OpenTelemetry.Exporter.Console/CHANGELOG.md b/src/OpenTelemetry.Exporter.Console/CHANGELOG.md
@@ -13,6 +13,10 @@
 * Added `LoggerProviderBuilder.AddConsoleExporter` registration extension.
   ([#4583](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4583))
 
+## 1.5.1
+
+Released 2023-Jun-26
+
 ## 1.5.0
 
 Released 2023-Jun-05
diff --git a/src/OpenTelemetry.Exporter.InMemory/CHANGELOG.md b/src/OpenTelemetry.Exporter.InMemory/CHANGELOG.md
@@ -5,6 +5,10 @@
 * Added `LoggerProviderBuilder.AddInMemoryExporter` registration extension.
   ([#4584](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4584))
 
+## 1.5.1
+
+Released 2023-Jun-26
+
 ## 1.5.0
 
 Released 2023-Jun-05
diff --git a/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md b/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+## 1.5.1
+
+Released 2023-Jun-26
+
 ## 1.5.0
 
 Released 2023-Jun-05
diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md
@@ -21,6 +21,10 @@
 * Updated to support `Severity` and `SeverityText` when exporting `LogRecord`s.
   ([#4568](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4568))
 
+## 1.5.1
+
+Released 2023-Jun-26
+
 ## 1.5.0
 
 Released 2023-Jun-05
diff --git a/src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md b/src/OpenTelemetry.Exporter.Zipkin/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+## 1.5.1
+
+Released 2023-Jun-26
+
 ## 1.5.0
 
 Released 2023-Jun-05
diff --git a/src/OpenTelemetry.Extensions.Hosting/CHANGELOG.md b/src/OpenTelemetry.Extensions.Hosting/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+## 1.5.1
+
+Released 2023-Jun-26
+
 ## 1.5.0
 
 Released 2023-Jun-05
diff --git a/src/OpenTelemetry.Extensions.Propagators/CHANGELOG.md b/src/OpenTelemetry.Extensions.Propagators/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+## 1.5.1
+
+Released 2023-Jun-26
+
 ## 1.5.0
 
 Released 2023-Jun-05
diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md
@@ -27,6 +27,26 @@
   exists.` when using any of the following exporters: `ConsoleExporter`,
   `OtlpExporter`, `ZipkinExporter`, `JaegerExporter`.
 
+## 1.5.1
+
+Released 2023-Jun-26
+
+* Fixed a breaking change causing `LogRecord.State` to be `null` where it was
+  previously set to a valid value when
+  `OpenTelemetryLoggerOptions.ParseStateValues` is `false` and states implement
+  `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
diff --git 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<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)
         {
@@ -319,6 +330,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
@@ -16,7 +16,6 @@
 
 #nullable enable
 
-using System.ComponentModel;
 using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.Runtime.CompilerServices;
@@ -95,9 +94,8 @@ public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Except
 
             LogRecordData.SetActivityContext(ref data, activity);
 
-            var attributes = record.Attributes = this.options.IncludeAttributes
-                ? ProcessState(record, ref iloggerData, in state, this.options.ParseStateValues)
-                : null;
+            var attributes = record.Attributes =
+                ProcessState(record, ref iloggerData, in state, this.options.IncludeAttributes, this.options.ParseStateValues);
 
             if (!TryGetOriginalFormatFromAttributes(attributes, out var originalFormat))
             {
@@ -153,9 +151,15 @@ internal static void SetLogRecordSeverityFields(ref LogRecordData logRecordData,
         LogRecord logRecord,
         ref LogRecord.LogRecordILoggerData iLoggerData,
         in TState state,
+        bool includeAttributes,
         bool parseStateValues)
     {
-        iLoggerData.State = null;
+        if (!includeAttributes
+            || (!typeof(TState).IsValueType && state is null))
+        {
+            iLoggerData.State = null;
+            return null;
+        }
 
         if (typeof(TState) == typeof(LogRecordAttributeList))
         {
@@ -166,14 +170,31 @@ internal static void SetLogRecordSeverityFields(ref LogRecordData logRecordData,
 
             var logRecordAttributes = (LogRecordAttributeList)(object)state!;
 
-            return logRecordAttributes.Export(ref logRecord.AttributeStorage);
+            var exportedAttributes = logRecordAttributes.Export(ref logRecord.AttributeStorage);
+
+            // Note: This is to preserve legacy behavior where State is exposed
+            // if we didn't parse state. We use exportedAttributes here to prevent a
+            // boxing of struct LogRecordAttributeList.
+            iLoggerData.State = !parseStateValues ? exportedAttributes : null;
+
+            return exportedAttributes;
         }
         else if (state is IReadOnlyList<KeyValuePair<string, object?>> stateList)
         {
+            // Note: This is to preserve legacy behavior where State is exposed
+            // if we didn't parse state. We use stateList here to prevent a
+            // second boxing of struct TStates.
+            iLoggerData.State = !parseStateValues ? stateList : null;
+
             return stateList;
         }
         else if (state is IEnumerable<KeyValuePair<string, object?>> stateValues)
         {
+            // Note: This is to preserve legacy behavior where State is exposed
+            // if we didn't parse state. We use stateValues here to prevent a
+            // second boxing of struct TStates.
+            iLoggerData.State = !parseStateValues ? stateValues : null;
+
             var attributeStorage = logRecord.AttributeStorage;
             if (attributeStorage == null)
             {
@@ -185,45 +206,23 @@ internal static void SetLogRecordSeverityFields(ref LogRecordData logRecordData,
                 return attributeStorage;
             }
         }
-        else if (!parseStateValues || state is null)
+        else if (!parseStateValues)
         {
             // Note: This is to preserve legacy behavior where State is
             // exposed if we didn't parse state.
             iLoggerData.State = state;
+
             return null;
         }
         else
         {
-            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;
-                    }
+            // Note: We clear State because the LogRecord we are processing may
+            // have come from the pool and may have State from a prior log.
+            iLoggerData.State = null;
 
-                    object? value = itemProperty.GetValue(state);
-                    if (value == null)
-                    {
-                        continue;
-                    }
+            OpenTelemetrySdkEventSource.Log.LoggerProcessStateSkipped<TState>();
 
-                    attributeStorage.Add(new KeyValuePair<string, object?>(itemProperty.Name, value));
-                }
-
-                return attributeStorage;
-            }
-            catch (Exception parseException)
-            {
-                OpenTelemetrySdkEventSource.Log.LoggerParseStateException<TState>(parseException);
-
-                return Array.Empty<KeyValuePair<string, object?>>();
-            }
+            return Array.Empty<KeyValuePair<string, object?>>();
         }
     }
 
diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs b/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; }
diff --git 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(37, warnings.Count());
+            Assert.Equal(36, warnings.Count());
         }
     }
 }
diff --git a/test/OpenTelemetry.Api.Tests/Logs/LogRecordAttributeListTests.cs b/test/OpenTelemetry.Api.Tests/Logs/LogRecordAttributeListTests.cs
@@ -117,7 +117,7 @@ public void ExportTest(int numberOfItems)
 
         if (numberOfItems == 0)
         {
-            Assert.Null(exportedAttributes);
+            Assert.Empty(exportedAttributes);
             Assert.Null(storage);
             return;
         }
diff --git a/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs b/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs
@@ -66,7 +66,7 @@ public void AddOtlpLogExporterReceivesAttributesWithParseStateValueSetToFalse()
             Assert.Single(logRecords);
             var logRecord = logRecords[0];
 #pragma warning disable CS0618 // Type or member is obsolete
-            Assert.Null(logRecord.State);
+            Assert.NotNull(logRecord.State);
 #pragma warning restore CS0618 // Type or member is obsolete
             Assert.NotNull(logRecord.Attributes);
         }
@@ -101,7 +101,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
             {
@@ -141,7 +145,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
