From ae0384060f39ef58126c7f97ec89855d355c119b Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Mon, 26 Jun 2023 10:19:50 -0700 Subject: [PATCH] [sdk-1.5.0-hotfix] Fix LogRecord.State being null when TState matches known interfaces (#4609) --- src/OpenTelemetry/CHANGELOG.md | 6 +++ .../Logs/ILogger/OpenTelemetryLogger.cs | 31 ++++++++++++--- .../OtlpLogExporterTests.cs | 2 +- .../OpenTelemetry.Tests/Logs/LogRecordTest.cs | 39 ++++++++++++++----- 4 files changed, 62 insertions(+), 16 deletions(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 12adabd6966..5eee0c5b453 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -2,6 +2,12 @@ ## Unreleased +* 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`s. + ([#4609](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4609)) + ## 1.5.0 Released 2023-Jun-05 diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs index 1ca14d3e0bd..e66c548d2bc 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs @@ -95,9 +95,8 @@ public void Log(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 +152,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; + } /* TODO: Enable this if/when LogRecordAttributeList becomes public. if (typeof(TState) == typeof(LogRecordAttributeList)) @@ -172,10 +177,20 @@ internal static void SetLogRecordSeverityFields(ref LogRecordData logRecordData, else */ if (state is IReadOnlyList> 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> 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) { @@ -187,7 +202,7 @@ 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. @@ -196,9 +211,13 @@ internal static void SetLogRecordSeverityFields(ref LogRecordData logRecordData, } else { + // 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; + try { - PropertyDescriptorCollection itemProperties = TypeDescriptor.GetProperties(state); + PropertyDescriptorCollection itemProperties = TypeDescriptor.GetProperties(state!); var attributeStorage = logRecord.AttributeStorage ??= new List>(itemProperties.Count); diff --git a/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs b/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs index 404b41e3533..d9e79fee60d 100644 --- a/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs +++ b/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs @@ -65,7 +65,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); } diff --git a/test/OpenTelemetry.Tests/Logs/LogRecordTest.cs b/test/OpenTelemetry.Tests/Logs/LogRecordTest.cs index f9f6e6cb038..a9861d03aca 100644 --- a/test/OpenTelemetry.Tests/Logs/LogRecordTest.cs +++ b/test/OpenTelemetry.Tests/Logs/LogRecordTest.cs @@ -82,7 +82,7 @@ public void CheckStateForUnstructuredLog(bool includeFormattedMessage) const string message = "Hello, World!"; logger.LogInformation(message); - Assert.Null(exportedItems[0].State); + Assert.NotNull(exportedItems[0].State); var attributes = exportedItems[0].Attributes; Assert.NotNull(attributes); @@ -113,7 +113,7 @@ public void CheckStateForUnstructuredLogWithStringInterpolation(bool includeForm var message = $"Hello from potato {0.99}."; logger.LogInformation(message); - Assert.Null(exportedItems[0].State); + Assert.NotNull(exportedItems[0].State); var attributes = exportedItems[0].Attributes; Assert.NotNull(attributes); @@ -143,7 +143,7 @@ public void CheckStateForStructuredLogWithTemplate(bool includeFormattedMessage) const string message = "Hello from {name} {price}."; logger.LogInformation(message, "tomato", 2.99); - Assert.Null(exportedItems[0].State); + Assert.NotNull(exportedItems[0].State); var attributes = exportedItems[0].Attributes; Assert.NotNull(attributes); @@ -185,7 +185,7 @@ public void CheckStateForStructuredLogWithStrongType(bool includeFormattedMessag var food = new Food { Name = "artichoke", Price = 3.99 }; logger.LogInformation("{food}", food); - Assert.Null(exportedItems[0].State); + Assert.NotNull(exportedItems[0].State); var attributes = exportedItems[0].Attributes; Assert.NotNull(attributes); @@ -226,7 +226,7 @@ public void CheckStateForStructuredLogWithAnonymousType(bool includeFormattedMes var anonymousType = new { Name = "pumpkin", Price = 5.99 }; logger.LogInformation("{food}", anonymousType); - Assert.Null(exportedItems[0].State); + Assert.NotNull(exportedItems[0].State); var attributes = exportedItems[0].Attributes; Assert.NotNull(attributes); @@ -271,7 +271,7 @@ public void CheckStateForStructuredLogWithGeneralType(bool includeFormattedMessa }; logger.LogInformation("{food}", food); - Assert.Null(exportedItems[0].State); + Assert.NotNull(exportedItems[0].State); var attributes = exportedItems[0].Attributes; Assert.NotNull(attributes); @@ -321,7 +321,13 @@ public void CheckStateForExceptionLogged() const string message = "Exception Occurred"; logger.LogInformation(exception, message); - Assert.Null(exportedItems[0].State); + Assert.NotNull(exportedItems[0].State); + + var state = exportedItems[0].State; + var itemCount = state.GetType().GetProperty("Count").GetValue(state); + + // state only has {OriginalFormat} + Assert.Equal(1, itemCount); var attributes = exportedItems[0].Attributes; Assert.NotNull(attributes); @@ -334,6 +340,7 @@ public void CheckStateForExceptionLogged() Assert.Equal(exceptionMessage, loggedException.Message); Assert.Equal(message, exportedItems[0].Body); + Assert.Equal(message, state.ToString()); Assert.Null(exportedItems[0].FormattedMessage); } @@ -711,7 +718,14 @@ public void VerifyParseStateValues_UsingStandardExtensions(bool parseStateValues var logRecord = exportedItems[0]; Assert.NotNull(logRecord.StateValues); - Assert.Null(logRecord.State); + if (parseStateValues) + { + Assert.Null(logRecord.State); + } + else + { + Assert.NotNull(logRecord.State); + } Assert.NotNull(logRecord.StateValues); Assert.Equal(3, logRecord.StateValues.Count); @@ -725,7 +739,14 @@ public void VerifyParseStateValues_UsingStandardExtensions(bool parseStateValues logRecord = exportedItems[1]; Assert.NotNull(logRecord.StateValues); - Assert.Null(logRecord.State); + if (parseStateValues) + { + Assert.Null(logRecord.State); + } + else + { + Assert.NotNull(logRecord.State); + } Assert.NotNull(logRecord.StateValues); Assert.Equal(4, logRecord.StateValues.Count);