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

Yun-Ting · 2023-06-07T23:08:12Z

Towards #3429

Changes

Most TStates implements IReadOnlyList and IEnumerable.
Added a event to log a warning for states, which requires fetching properties at runtime, will be skipped if RuntimeFeature.IsDynamicCodeSupported evaluates to false.

Merge requirement checklist

CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes

codecov · 2023-06-08T00:06:43Z

Codecov Report

Merging #4560 (cbdef79) into main (5f825cb) will increase coverage by 0.03%.
The diff coverage is 62.50%.

❗ Current head cbdef79 differs from pull request most recent head 90771fe. Consider uploading reports for the commit 90771fe to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4560      +/-   ##
==========================================
+ Coverage   84.88%   84.92%   +0.03%     
==========================================
  Files         313      313              
  Lines       12604    12594      -10     
==========================================
- Hits        10699    10695       -4     
+ Misses       1905     1899       -6

Impacted Files	Coverage Δ
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs	`81.91% <50.00%> (+2.12%)`	⬆️
.../OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs	`87.87% <100.00%> (+1.64%)`	⬆️

... and 4 files with indirect coverage changes

src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs

CodeBlanch

LGTM

src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs

test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs

src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs

eerhardt

Assuming @utpilla and @CodeBlanch are fine with this change, LGTM.

src/OpenTelemetry/CHANGELOG.md

src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs

src/OpenTelemetry/CHANGELOG.md

src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs

utpilla · 2023-06-22T01:12:02Z

src/OpenTelemetry/CHANGELOG.md

@@ -2,6 +2,12 @@

 ## Unreleased

+* **Breaking Change** Removed support for ParseStateValues if the state
+  implements neither IReadOnlyList nor IEnumberable when `ParseStateValues` is


Suggested change

implements neither IReadOnlyList nor IEnumberable when `ParseStateValues` is

implements neither `IReadOnlyList<KeyValuePair<string, object>>` nor `IEnumerable<KeyValuePair<string, object>>` when `ParseStateValues` is

Same for:

https://github.com/open-telemetry/opentelemetry-dotnet/pull/4560/files#diff-f1b040b01138ef7a10c01209ca53e58d20b78bb4580eafa0f51030b01d529145R316

https://github.com/open-telemetry/opentelemetry-dotnet/pull/4560/files#diff-48886d84cf3065379188763f0948382a1b89bf9ebf9bdef6f8146fa6acf4776eR196

@CodeBlanch Should we be using this instead?

IReadOnlyList<out T> nor IEnumerable<out T> where T is KeyValuePair<string, object>.

…opentelemetry-dotnet into yunl/aotTypeDescriptor

utpilla · 2023-06-22T01:59:48Z

src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs

-
-                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>>.");


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<string,

/// object></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; }

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

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

#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?

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).

src/OpenTelemetry/CHANGELOG.md

src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs

utpilla · 2023-06-28T22:59:40Z

This change was merged in #4619

initial

d59c258

Yun-Ting force-pushed the yunl/aotTypeDescriptor branch from c1caf1f to d59c258 Compare June 7, 2023 23:55

Yun-Ting marked this pull request as ready for review June 8, 2023 00:09

Yun-Ting requested a review from a team June 8, 2023 00:09

utpilla reviewed Jun 8, 2023

View reviewed changes

src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs Show resolved Hide resolved

Yun-Ting added 2 commits June 8, 2023 14:18

parse for older .net targets

3413cfb

update test

715ad28

CodeBlanch approved these changes Jun 8, 2023

View reviewed changes

utpilla reviewed Jun 9, 2023

View reviewed changes

src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs Outdated Show resolved Hide resolved

utpilla reviewed Jun 9, 2023

View reviewed changes

test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs Outdated Show resolved Hide resolved

eerhardt reviewed Jun 9, 2023

View reviewed changes

src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs Outdated Show resolved Hide resolved

Yun-Ting added 6 commits June 15, 2023 16:35

removed ParseState

a74ed86

merge main

ea364a4

removed LoggerParseStateException

af5df9f

nit

1e17473

test

f018b04

removed test

4b19d17

vitek-karas approved these changes Jun 16, 2023

View reviewed changes

eerhardt approved these changes Jun 16, 2023

View reviewed changes

Yun-Ting changed the title ~~[AOT] Addressed TypeDescriptor.GetProperties warning.~~ [AOT] Addressed TypeDescriptor.GetProperties warning by removing it. Jun 16, 2023

changelog

aa0f470

reyang reviewed Jun 16, 2023

View reviewed changes

src/OpenTelemetry/CHANGELOG.md Outdated Show resolved Hide resolved

reyang reviewed Jun 16, 2023

View reviewed changes

src/OpenTelemetry/CHANGELOG.md Outdated Show resolved Hide resolved

comments

7ff4a77

Yun-Ting changed the title ~~[AOT] Addressed TypeDescriptor.GetProperties warning by removing it.~~ [AOT] Removed support for ParseStateValues if the state neither implements IReadOnlyList nor IEnumberable when ParseStateValues is true. Jun 16, 2023

test

5050f6f

Yun-Ting commented Jun 19, 2023

View reviewed changes

src/OpenTelemetry/CHANGELOG.md Outdated Show resolved Hide resolved

changelog

136cec1

Yun-Ting force-pushed the yunl/aotTypeDescriptor branch from 4fb135a to 136cec1 Compare June 21, 2023 17:01

ci

7e60a6c

cijothomas reviewed Jun 21, 2023

View reviewed changes

src/OpenTelemetry/CHANGELOG.md Outdated Show resolved Hide resolved

cijothomas reviewed Jun 21, 2023

View reviewed changes

src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs Outdated Show resolved Hide resolved

Yun-Ting added 2 commits June 21, 2023 17:39

addressed comments

c005f46

Merge branch 'main' into yunl/aotTypeDescriptor

6f528ce

utpilla reviewed Jun 22, 2023

View reviewed changes

src/OpenTelemetry/CHANGELOG.md Outdated Show resolved Hide resolved

utpilla reviewed Jun 22, 2023

View reviewed changes

src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs Outdated Show resolved Hide resolved

utpilla reviewed Jun 22, 2023

View reviewed changes

src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs Outdated Show resolved Hide resolved

utpilla reviewed Jun 22, 2023

View reviewed changes

Yun-Ting added 3 commits June 21, 2023 18:30

addressed comments

5daab01

Merge branch 'yunl/aotTypeDescriptor' of https://github.com/Yun-Ting/…

9589b76

…opentelemetry-dotnet into yunl/aotTypeDescriptor

Merge branch 'main' into yunl/aotTypeDescriptor

9089a63

utpilla reviewed Jun 22, 2023

View reviewed changes

CodeBlanch reviewed Jun 22, 2023

View reviewed changes

src/OpenTelemetry/CHANGELOG.md Outdated Show resolved Hide resolved

CodeBlanch reviewed Jun 22, 2023

View reviewed changes

src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs Outdated Show resolved Hide resolved

Yun-Ting added 3 commits June 22, 2023 16:52

comments

aff152f

Merge branch 'main' into yunl/aotTypeDescriptor

cbdef79

ci

90771fe

utpilla mentioned this pull request Jun 23, 2023

[sdk-1.5.0-hotfix] Fix LogRecord.State being null when TState matches known interfaces #4609

Merged

3 tasks

utpilla closed this Jun 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Yun-Ting commented Jun 7, 2023 •

edited

Loading

codecov bot commented Jun 8, 2023 •

edited

Loading

CodeBlanch left a comment

eerhardt left a comment

utpilla Jun 22, 2023 •

edited

Loading

utpilla Jun 22, 2023

utpilla Jun 22, 2023

utpilla Jun 22, 2023

CodeBlanch Jun 22, 2023

utpilla Jun 22, 2023

Yun-Ting Jun 22, 2023

utpilla Jun 22, 2023

utpilla commented Jun 28, 2023

	implements neither IReadOnlyList nor IEnumberable when `ParseStateValues` is
	implements neither `IReadOnlyList<KeyValuePair<string, object>>` nor `IEnumerable<KeyValuePair<string, object>>` when `ParseStateValues` is

	/// <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<string,
	/// object></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; }

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

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

Conversation

Yun-Ting commented Jun 7, 2023 • edited Loading

Changes

Merge requirement checklist

codecov bot commented Jun 8, 2023 • edited Loading

Codecov Report

CodeBlanch left a comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

utpilla Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

utpilla Jun 22, 2023

Choose a reason for hiding this comment

utpilla Jun 22, 2023

Choose a reason for hiding this comment

utpilla Jun 22, 2023

Choose a reason for hiding this comment

CodeBlanch Jun 22, 2023

Choose a reason for hiding this comment

utpilla Jun 22, 2023

Choose a reason for hiding this comment

Yun-Ting Jun 22, 2023

Choose a reason for hiding this comment

utpilla Jun 22, 2023

Choose a reason for hiding this comment

utpilla commented Jun 28, 2023

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

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

Yun-Ting commented Jun 7, 2023 •

edited

Loading

codecov bot commented Jun 8, 2023 •

edited

Loading

utpilla Jun 22, 2023 •

edited

Loading