Skip to content

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

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

Merged
utpilla merged 7 commits into from
Jun 28, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ae03840
[sdk-1.5.0-hotfix] Fix LogRecord.State being null when TState matches…
CodeBlanch Jun 26, 2023
b03e031
[sdk-1.5.0-hotfix] Removed support for parsing custom log states usin…
CodeBlanch Jun 26, 2023
8d61691
[sdk-1.5.0-hotfix] Update CHANGELOG for release (#4615)
CodeBlanch Jun 26, 2023
012c08f
Merge 1.5.1 hotfix changes to main.
CodeBlanch Jun 26, 2023
1cf54f1
Updates for 1.6 logging changes.
CodeBlanch Jun 26, 2023
7af15f6
Test updates.
CodeBlanch Jun 26, 2023
6a8fbaa
Merge branch 'main' into merge-1.5.1-fixes
CodeBlanch Jun 28, 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
Prev Previous commit
Next Next commit
Updates for 1.6 logging changes.
  • Loading branch information
@CodeBlanch
CodeBlanch committed Jun 26, 2023
commit 1cf54f19f93c36c45e7397b89f964b2ed0f42f27
5 changes: 3 additions & 2 deletions src/OpenTelemetry.Api/Logs/LogRecordAttributeList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Copy link
Contributor

@utpilla utpilla Jun 27, 2023

Choose a reason for hiding this comment

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

How does this work with LoggerSdk.EmitLog? This would be changing the behavior of LoggerSdk.EmitLog to return an empty list instead of null when the attribute count is zero. Is that what we want?

opentelemetry-dotnet/src/OpenTelemetry/Logs/LoggerSdk.cs

Line 56 in 303a357

logRecord.Attributes = attributes.Export(ref logRecord.AttributeStorage);

LogRecord.Attributes description says that its value is set depending upon the options: IncludeAttributes or ParseStateValues. Would these options be considered when using LoggerSdk.EmitLog?

Copy link
Member Author

@CodeBlanch CodeBlanch Jun 27, 2023

Choose a reason for hiding this comment

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

There's a lot to unpack here 🤣

How does this work with LoggerSdk.EmitLog? This would be changing the behavior of LoggerSdk.EmitLog to return an empty list instead of null when the attribute count is zero. Is that what we want?

I don't think it is an issue. It is all net-new so up to us to define the behavior. When calling Logger.EmitLog (at the moment) there are always attributes. It is either user-supplied (may have 0 count or more) or it is default (0 count). It is a struct so there isn't a way to pass null. With this change the LogRecord will mirror that behavior. It will always have attributes either empty (0) or some list using the pool storage.

I made this change because the exporter which causes the 1.5.1 breakfix is doing...

var list = (IReadOnlyList<KeyValuePair<string, object>>)logRecord.State;

It isn't checking for null and it isn't looking at Attributes or StateValues. If some users take a newer SDK and starts using some Logger.EmitLog thing that exporter would break again. I needed to make sure this path also had a valid logRecord.State. Since State would have a value I decided Attributes / StateValues should as well. It felt confusing the other way around.

LogRecord.Attributes description says that its value is set depending upon the options: IncludeAttributes or ParseStateValues. Would these options be considered when using LoggerSdk.EmitLog?

OpenTelemetryLoggerOptions only applies to the ILogger integration. I'll open a follow-up PR to clarify the XML comments on LogRecord because you are right they are kind of misleading.

Now you can use LogRecordAttributeList with the ILogger integration! You can do logger.Log<LogRecordAttributeList>(...) which is using LogRecordAttributeList as TState in the ILogger API. This is really the only way at the moment to get truly allocation-free logging in OpenTelemetry .NET. If you set OpenTelemetryLoggerOptions.IncludeAttributes = false AND you use LogRecordAttributeList as the TState THEN IncludeAttributes is respected and the LogRecord ends up with null values for State, Attributes, and StateValues.

}

var overflowAttributes = this.OverflowAttributes;
Expand Down
9 changes: 8 additions & 1 deletion src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,14 @@ 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)
{
Expand Down