Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[AOT] Removed support for ParseStateValues if the state neither implements IReadOnlyList nor IEnumberable when ParseStateValues is true. #4560

Closed
wants to merge 22 commits into from
Closed

[AOT] Removed support for ParseStateValues if the state neither implements IReadOnlyList nor IEnumberable when ParseStateValues is true. #4560

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
d59c258
initial
Yun-Ting Jun 7, 2023
3413cfb
parse for older .net targets
Yun-Ting Jun 8, 2023
715ad28
update test
Yun-Ting Jun 8, 2023
a74ed86
removed ParseState
Yun-Ting Jun 15, 2023
ea364a4
merge main
Yun-Ting Jun 15, 2023
af5df9f
removed LoggerParseStateException
Yun-Ting Jun 15, 2023
1e17473
nit
Yun-Ting Jun 16, 2023
f018b04
test
Yun-Ting Jun 16, 2023
4b19d17
removed test
Yun-Ting Jun 16, 2023
aa0f470
changelog
Yun-Ting Jun 16, 2023
7ff4a77
comments
Yun-Ting Jun 16, 2023
5050f6f
test
Yun-Ting Jun 16, 2023
136cec1
changelog
Yun-Ting Jun 21, 2023
7e60a6c
ci
Yun-Ting Jun 21, 2023
c005f46
addressed comments
Yun-Ting Jun 22, 2023
6f528ce
Merge branch 'main' into yunl/aotTypeDescriptor
Yun-Ting Jun 22, 2023
5daab01
addressed comments
Yun-Ting Jun 22, 2023
9589b76
Merge branch 'yunl/aotTypeDescriptor' of https:/Yun-Ting/…
Yun-Ting Jun 22, 2023
9089a63
Merge branch 'main' into yunl/aotTypeDescriptor
Yun-Ting Jun 22, 2023
aff152f
comments
Yun-Ting Jun 22, 2023
cbdef79
Merge branch 'main' into yunl/aotTypeDescriptor
Yun-Ting Jun 22, 2023
90771fe
ci
Yun-Ting Jun 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@

## Unreleased

* **Breaking Change** Removed support for ParseStateValues if the state
implements neither `IReadOnlyList<KeyValuePair<string, object>>` nor
`IEnumerable<KeyValuePair<string, object>>` when `ParseStateValues` is
true to make `ProcessState` AOT compatible. This feature was first introduced
in `1.5.0` stable release with
[#4334](https:/open-telemetry/opentelemetry-dotnet/pull/4334).
([#4560](https:/open-telemetry/opentelemetry-dotnet/pull/4560))
Yun-Ting marked this conversation as resolved.
Show resolved Hide resolved

* Add back support for Exemplars. See [exemplars](../../docs/metrics/customizing-the-sdk/README.md#exemplars)
for instructions to enable exemplars.
([#4553](https:/open-telemetry/opentelemetry-dotnet/pull/4553))
Expand Down
24 changes: 12 additions & 12 deletions src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,20 +141,20 @@ public void DroppedExportProcessorItems(string exportProcessorName, string expor
}

[NonEvent]
public void LoggerParseStateException<TState>(Exception exception)
public void LoggerProviderException(string methodName, Exception ex)
{
if (this.IsEnabled(EventLevel.Warning, EventKeywords.All))
if (this.IsEnabled(EventLevel.Error, EventKeywords.All))
{
this.LoggerParseStateException(typeof(TState).FullName!, exception.ToInvariantString());
this.LoggerProviderException(methodName, ex.ToInvariantString());
}
}

[NonEvent]
public void LoggerProviderException(string methodName, Exception ex)
public void LoggerProcessStateSkipped<TState>(string typeOfTState, string fix)
{
if (this.IsEnabled(EventLevel.Error, EventKeywords.All))
if (this.IsEnabled(EventLevel.Warning, EventKeywords.All))
{
this.LoggerProviderException(methodName, ex.ToInvariantString());
this.LoggerProcessStateSkipped(typeOfTState, fix);
Yun-Ting marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -301,12 +301,6 @@ public void InvalidEnvironmentVariable(string key, string? value)
this.WriteEvent(47, key, value);
}

[Event(48, Message = "Exception thrown parsing log state of type '{0}'. Exception: '{1}'", Level = EventLevel.Warning)]
public void LoggerParseStateException(string type, string error)
{
this.WriteEvent(48, type, error);
}

[Event(49, Message = "LoggerProviderSdk event: '{0}'", Level = EventLevel.Verbose)]
public void LoggerProviderSdkEvent(string message)
{
Expand All @@ -319,6 +313,12 @@ public void LoggerProviderException(string methodName, string ex)
this.WriteEvent(50, methodName, ex);
}

[Event(51, Message = "Skip processing log state of type '{0}' because it does not implement either IReadOnlyList<KeyValuePair<string, object>> or IEnumerable<KeyValuePair<string, object>>. Suggested action: '{1}'.", Level = EventLevel.Warning)]
public void LoggerProcessStateSkipped(string type, string fix)
{
this.WriteEvent(51, type, fix);
}

#if DEBUG
public class OpenTelemetryEventListener : EventListener
{
Expand Down
33 changes: 2 additions & 31 deletions src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

#nullable enable

using System.ComponentModel;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
Expand Down Expand Up @@ -194,36 +193,8 @@ internal static void SetLogRecordSeverityFields(ref LogRecordData logRecordData,
}
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;
}

object? value = itemProperty.GetValue(state);
if (value == null)
{
continue;
}

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?>>();
}
OpenTelemetrySdkEventSource.Log.LoggerProcessStateSkipped<TState>(typeof(TState).FullName!, "This can be fixed by updating the state to be a type that implements either IReadOnlyList<KeyValuePair<string, object>> or IEnumerable<KeyValuePair<string, object>>.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the documentation of ParseStateValues option to reflect this change.

opentelemetry-dotnet/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs

Lines 53 to 70 in bd0d8d6

/// <summary>
/// Gets or sets a value indicating whether or not log state should be
/// parsed into <see cref="LogRecord.Attributes"/> on generated <see
/// cref="LogRecord"/>s. Default value: <see langword="false"/>.
/// </summary>
/// <remarks>
/// Notes:
/// <list type="bullet">
/// <item>Parsing is only executed when the state logged does NOT
/// implement <see cref="IReadOnlyList{T}"/> or <see
/// cref="IEnumerable{T}"/> where <c>T</c> is <c>KeyValuePair&lt;string,
/// object&gt;</c>.</item>
/// <item>When <see cref="ParseStateValues"/> is set to <see
/// langword="true"/> <see cref="LogRecord.State"/> will always be <see
/// langword="null"/>.</item>
/// </list>
/// </remarks>
public bool ParseStateValues { get; set; }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI removing this code ParseStateValues essentially does nothing. Should we obsolete it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would break build for users who have used this option in their Logs SDK setup, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#4334 redefines ParseStateValues.
With the redefined ParseStateValues, value of public bool ParseStateValues { get; set; } is default to false and only should be set to true if state does not implement either IReadOnlyList or IEnumerable if users want to get the properties with Reflection.

Shall we obsolete this API in this PR or use a follow-up PR for that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we obsolete this API in this PR or use a follow-up PR for that?

It should be its own PR (if at all we decide to do that).

return Array.Empty<KeyValuePair<string, object?>>();
}
}

Expand Down
2 changes: 1 addition & 1 deletion test/OpenTelemetry.AotCompatibility.Tests/AotCompatibilityTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
}
2 changes: 0 additions & 2 deletions test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ 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");
}
else
{
Expand Down Expand Up @@ -141,7 +140,6 @@ 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");
}
else
{
Expand Down
31 changes: 0 additions & 31 deletions test/OpenTelemetry.Tests/Logs/LogRecordTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,37 +804,6 @@ public void ParseStateValuesUsingIEnumerableTest()
Assert.Equal(new KeyValuePair<string, object>("Key1", "Value1"), logRecord.StateValues[0]);
}

[Fact]
public void ParseStateValuesUsingCustomTest()
{
using var loggerFactory = InitializeLoggerFactory(out List<LogRecord> exportedItems, configure: options => options.ParseStateValues = true);
var logger = loggerFactory.CreateLogger<LogRecordTest>();

// Tests unknown state parse path.

CustomState state = new CustomState
{
Property = "Value",
};

logger.Log(
LogLevel.Information,
0,
state,
null,
(s, e) => "OpenTelemetry!");
var logRecord = exportedItems[0];

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

[Fact]
public void DisposingStateTest()
{
Expand Down