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

[otlp] Remove ilogger and exception attributes #4781

Merged
merged 12 commits into from
Aug 21, 2023
Merged

[otlp] Remove ilogger and exception attributes #4781

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
87980ac
Temporarily remove ilogger and ex attributes
vishweshbankwar Aug 17, 2023
65aa7ef
add changelog
vishweshbankwar Aug 17, 2023
619d2bd
Update CHANGELOG.md
vishweshbankwar Aug 17, 2023
e5c892f
format
vishweshbankwar Aug 17, 2023
83ea7fe
Merge branch 'vibankwa/remove-experimental-attributes' of https://git…
vishweshbankwar Aug 17, 2023
6ca7e06
rmv using
vishweshbankwar Aug 17, 2023
3325b40
format
vishweshbankwar Aug 17, 2023
79295b4
format test
vishweshbankwar Aug 17, 2023
576429c
pr feedback
vishweshbankwar Aug 18, 2023
e6b314c
address feedback
vishweshbankwar Aug 18, 2023
1fbc66d
addres feedback
vishweshbankwar Aug 21, 2023
559f26f
Merge branch 'main' into vibankwa/remove-experimental-attributes
vishweshbankwar Aug 21, 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
5 changes: 5 additions & 0 deletions src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

## Unreleased

* Excluded attributes corresponding to `LogRecord.EventId`,
Copy link
Member

Choose a reason for hiding this comment

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

its better if we add a stronger message here, as this is a significant breaking change for users who are relying on these features.

Breaking/Please Note or something to catch attention.

Copy link

Choose a reason for hiding this comment

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

Not having the EventId & Category properties makes it harder to filter log messages to the ones that you care about. They could go in the attributes as they are SDK specific.

Copy link

Choose a reason for hiding this comment

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

Not having the EventId & Category properties makes it harder to filter log messages to the ones that you care about. They could go in the attributes as they are SDK specific.

+1

Copy link

Choose a reason for hiding this comment

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

Not having the EventId & Category properties makes it harder to filter log messages to the ones that you care about. They could go in the attributes as they are SDK specific.

Indeed. Missed this in last week's release notes and ran into this today. Was a real head-scratcher for a few hours because this is the last place I expected these (fairly ubiquitous) dotnet attrs to be getting dropped ☹️

`LogRecord.CategoryName` and `LogRecord.Exception` from the exported data. This
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
is done as the semantic conventions for these attributes are not yet stable.
([#4781](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4781))

* Added extension method for configuring export processor options for otlp log
exporter.
([#4733](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4733))
Expand Down
7 changes: 5 additions & 2 deletions src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/LogRecordExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,7 @@

using System.Runtime.CompilerServices;
using Google.Protobuf;
using OpenTelemetry.Internal;
using OpenTelemetry.Logs;
using OpenTelemetry.Trace;
using OtlpCollector = OpenTelemetry.Proto.Collector.Logs.V1;
using OtlpCommon = OpenTelemetry.Proto.Common.V1;
using OtlpLogs = OpenTelemetry.Proto.Logs.V1;
Expand Down Expand Up @@ -80,6 +78,10 @@ internal static OtlpLogs.LogRecord ToOtlpLog(this LogRecord logRecord, SdkLimitO
var attributeValueLengthLimit = sdkLimitOptions.AttributeValueLengthLimit;
var attributeCountLimit = sdkLimitOptions.AttributeCountLimit ?? int.MaxValue;

/*
// Removing this temporarily for stable release
// https://github.com/open-telemetry/opentelemetry-dotnet/issues/4776
// https://github.com/open-telemetry/opentelemetry-dotnet/issues/3491
alanwest marked this conversation as resolved.
Show resolved Hide resolved
// First add the generic attributes like Category, EventId and Exception,
// so they are less likely being dropped because of AttributeCountLimit.

Expand Down Expand Up @@ -109,6 +111,7 @@ internal static OtlpLogs.LogRecord ToOtlpLog(this LogRecord logRecord, SdkLimitO
otlpLogRecord.AddStringAttribute(SemanticConventions.AttributeExceptionMessage, logRecord.Exception.Message, attributeValueLengthLimit, attributeCountLimit);
otlpLogRecord.AddStringAttribute(SemanticConventions.AttributeExceptionStacktrace, logRecord.Exception.ToInvariantString(), attributeValueLengthLimit, attributeCountLimit);
}
*/

bool bodyPopulatedFromFormattedMessage = false;
if (logRecord.FormattedMessage != null)
Expand Down
88 changes: 69 additions & 19 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 @@ -23,7 +23,6 @@
using Moq;
using OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation;
using OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.ExportClient;
using OpenTelemetry.Internal;
using OpenTelemetry.Logs;
using OpenTelemetry.Tests;
using OpenTelemetry.Trace;
Expand Down Expand Up @@ -184,25 +183,29 @@ public void OtlpLogRecordTestWhenStateValuesArePopulated()

Assert.NotNull(otlpLogRecord);
Assert.Equal("Hello from tomato 2.99.", otlpLogRecord.Body.StringValue);
Assert.Equal(4, otlpLogRecord.Attributes.Count);
Assert.Equal(3, otlpLogRecord.Attributes.Count);
var index = 0;
var attribute = otlpLogRecord.Attributes[index];

var attribute = otlpLogRecord.Attributes[0];
/*
Assert.Equal("dotnet.ilogger.category", attribute.Key);
Assert.Equal("OtlpLogExporterTests", attribute.Value.StringValue);
attribute = otlpLogRecord.Attributes[++index];
*/

attribute = otlpLogRecord.Attributes[1];
Assert.Equal("name", attribute.Key);
Assert.Equal("tomato", attribute.Value.StringValue);

attribute = otlpLogRecord.Attributes[2];
attribute = otlpLogRecord.Attributes[++index];
Assert.Equal("price", attribute.Key);
Assert.Equal(2.99, attribute.Value.DoubleValue);

attribute = otlpLogRecord.Attributes[3];
attribute = otlpLogRecord.Attributes[++index];
Assert.Equal("{OriginalFormat}", attribute.Key);
Assert.Equal("Hello from {name} {price}.", attribute.Value.StringValue);
}

/*
[Fact]
public void CheckToOtlpLogRecordLoggerCategory()
{
Expand Down Expand Up @@ -287,6 +290,7 @@ public void CheckToOtlpLogRecordEventId()
Assert.Contains("Name", otlpLogRecordAttributes);
Assert.Contains("MyEvent10", otlpLogRecordAttributes);
}
*/

[Fact]
public void CheckToOtlpLogRecordTimestamps()
Expand Down Expand Up @@ -485,6 +489,7 @@ public void CheckToOtlpLogRecordBodyIsPopulated(bool includeFormattedMessage)
Assert.Equal("state", otlpLogRecord.Body.StringValue);
}

/*
[Fact]
public void CheckToOtlpLogRecordExceptionAttributes()
{
Expand Down Expand Up @@ -521,7 +526,7 @@ public void CheckToOtlpLogRecordRespectsAttributeLimits()
{
var sdkLimitOptions = new SdkLimitOptions
{
AttributeCountLimit = 3, // 3 => LogCategory, exception.type and exception.message
AttributeCountLimit = 2,
AttributeValueLengthLimit = 8,
};

Expand Down Expand Up @@ -557,6 +562,51 @@ public void CheckToOtlpLogRecordRespectsAttributeLimits()
var exceptionStackTraceAtt = TryGetAttribute(otlpLogRecord, SemanticConventions.AttributeExceptionStacktrace);
Assert.Null(exceptionStackTraceAtt);
}
*/

// Remove this when adding back the category, eventid and exception attributes
[Fact]
public void CheckToOtlpLogRecordRespectsAttributeLimits()
vishweshbankwar marked this conversation as resolved.
Show resolved Hide resolved
{
var sdkLimitOptions = new SdkLimitOptions
{
AttributeCountLimit = 2,
AttributeValueLengthLimit = 8,
};

var logRecords = new List<LogRecord>();
using var loggerFactory = LoggerFactory.Create(builder =>
{
builder.AddOpenTelemetry(options =>
{
options.ParseStateValues = true;
options.AddInMemoryExporter(logRecords);
});
});

var logger = loggerFactory.CreateLogger(string.Empty);
logger.LogInformation("OpenTelemetry {AttributeOne} {AttributeTwo} {AttributeThree}!", "I'm an attribute", "I too am an attribute", "I get dropped :(");

var logRecord = logRecords[0];
var otlpLogRecord = logRecord.ToOtlpLog(sdkLimitOptions);

Assert.NotNull(otlpLogRecord);
Assert.Equal(1u, otlpLogRecord.DroppedAttributesCount);

var attribute = TryGetAttribute(otlpLogRecord, "AttributeOne");
Assert.NotNull(attribute);

// "I'm an a" == first 8 chars from the first attribute "I'm an attribute"
Assert.Equal("I'm an a", attribute.Value.StringValue);
attribute = TryGetAttribute(otlpLogRecord, "AttributeTwo");
Assert.NotNull(attribute);

// "I too am" == first 8 chars from the second attribute "I too am an attribute"
Assert.Equal("I too am", attribute.Value.StringValue);

attribute = TryGetAttribute(otlpLogRecord, "AttributeThree");
Assert.Null(attribute);
}

[Fact]
public void Export_WhenExportClientIsProvidedInCtor_UsesProvidedExportClient()
Expand Down Expand Up @@ -686,7 +736,7 @@ public void ToOtlpLog_WhenOptionsIncludeScopesIsTrue_ContainsScopeAttributeStrin
// Assert.
var logRecord = logRecords.Single();
var otlpLogRecord = logRecord.ToOtlpLog(DefaultSdkLimitOptions);
Assert.Equal(2, otlpLogRecord.Attributes.Count);
Assert.Single(otlpLogRecord.Attributes);
var actualScope = TryGetAttribute(otlpLogRecord, scopeKey);
Assert.NotNull(actualScope);
Assert.Equal(scopeKey, actualScope.Key);
Expand Down Expand Up @@ -725,7 +775,7 @@ public void ToOtlpLog_WhenOptionsIncludeScopesIsTrue_ContainsScopeAttributeBoolV
// Assert.
var logRecord = logRecords.Single();
var otlpLogRecord = logRecord.ToOtlpLog(DefaultSdkLimitOptions);
Assert.Equal(2, otlpLogRecord.Attributes.Count);
Assert.Single(otlpLogRecord.Attributes);
var actualScope = TryGetAttribute(otlpLogRecord, scopeKey);
Assert.NotNull(actualScope);
Assert.Equal(scopeKey, actualScope.Key);
Expand Down Expand Up @@ -776,7 +826,7 @@ public void ToOtlpLog_WhenOptionsIncludeScopesIsTrue_ContainsScopeAttributeIntVa
// Assert.
var logRecord = logRecords.Single();
var otlpLogRecord = logRecord.ToOtlpLog(DefaultSdkLimitOptions);
Assert.Equal(2, otlpLogRecord.Attributes.Count);
Assert.Single(otlpLogRecord.Attributes);
var actualScope = TryGetAttribute(otlpLogRecord, scopeKey);
Assert.NotNull(actualScope);
Assert.Equal(scopeKey, actualScope.Key);
Expand Down Expand Up @@ -815,7 +865,7 @@ public void ToOtlpLog_WhenOptionsIncludeScopesIsTrue_ContainsScopeAttributeDoubl
// Assert.
var logRecord = logRecords.Single();
var otlpLogRecord = logRecord.ToOtlpLog(DefaultSdkLimitOptions);
Assert.Equal(2, otlpLogRecord.Attributes.Count);
Assert.Single(otlpLogRecord.Attributes);
var actualScope = TryGetAttribute(otlpLogRecord, scopeKey);
Assert.NotNull(actualScope);
Assert.Equal(scopeKey, actualScope.Key);
Expand Down Expand Up @@ -854,7 +904,7 @@ public void ToOtlpLog_WhenOptionsIncludeScopesIsTrue_ContainsScopeAttributeDoubl
// Assert.
var logRecord = logRecords.Single();
var otlpLogRecord = logRecord.ToOtlpLog(DefaultSdkLimitOptions);
Assert.Equal(2, otlpLogRecord.Attributes.Count);
Assert.Single(otlpLogRecord.Attributes);
var actualScope = TryGetAttribute(otlpLogRecord, scopeKey);
Assert.NotNull(actualScope);
Assert.Equal(scopeKey, actualScope.Key);
Expand Down Expand Up @@ -888,7 +938,7 @@ public void ToOtlpLog_WhenOptionsIncludeScopesIsTrue_AndScopeStateIsOfTypeString
var logRecord = logRecords.Single();
var otlpLogRecord = logRecord.ToOtlpLog(DefaultSdkLimitOptions);
Assert.NotNull(otlpLogRecord);
Assert.Single(otlpLogRecord.Attributes);
Assert.Empty(otlpLogRecord.Attributes);
}

[Theory]
Expand Down Expand Up @@ -923,7 +973,7 @@ public void ToOtlpLog_WhenOptionsIncludeScopesIsTrue_AndScopeStateIsOfPrimitiveT
var logRecord = logRecords.Single();
var otlpLogRecord = logRecord.ToOtlpLog(DefaultSdkLimitOptions);
Assert.NotNull(otlpLogRecord);
Assert.Single(otlpLogRecord.Attributes);
Assert.Empty(otlpLogRecord.Attributes);
}

[Fact]
Expand Down Expand Up @@ -954,7 +1004,7 @@ public void ToOtlpLog_WhenOptionsIncludeScopesIsTrue_AndScopeStateIsOfDictionary
// Assert.
var logRecord = logRecords.Single();
var otlpLogRecord = logRecord.ToOtlpLog(DefaultSdkLimitOptions);
Assert.Equal(2, otlpLogRecord.Attributes.Count);
Assert.Single(otlpLogRecord.Attributes);
var actualScope = TryGetAttribute(otlpLogRecord, scopeKey);
Assert.NotNull(actualScope);
Assert.Equal(scopeKey, actualScope.Key);
Expand Down Expand Up @@ -993,7 +1043,7 @@ public void ToOtlpLog_WhenOptionsIncludeScopesIsTrue_AndScopeStateIsOfEnumerable
// Assert.
var logRecord = logRecords.Single();
var otlpLogRecord = logRecord.ToOtlpLog(DefaultSdkLimitOptions);
Assert.Equal(2, otlpLogRecord.Attributes.Count);
Assert.Single(otlpLogRecord.Attributes);
var actualScope = TryGetAttribute(otlpLogRecord, scopeKey);
Assert.NotNull(actualScope);
Assert.Equal(scopeKey, actualScope.Key);
Expand Down Expand Up @@ -1036,7 +1086,7 @@ public void ToOtlpLog_WhenOptionsIncludeScopesIsTrue_AndMultipleScopesAreAdded_C
var allScopeValues = otlpLogRecord.Attributes
.Where(_ => _.Key == scopeKey1 || _.Key == scopeKey2)
.Select(_ => _.Value.StringValue);
Assert.Equal(3, otlpLogRecord.Attributes.Count);
Assert.Equal(2, otlpLogRecord.Attributes.Count);
Assert.Equal(2, allScopeValues.Count());
Assert.Contains(scopeValue1, allScopeValues);
Assert.Contains(scopeValue2, allScopeValues);
Expand Down Expand Up @@ -1077,7 +1127,7 @@ public void ToOtlpLog_WhenOptionsIncludeScopesIsTrue_AndMultipleScopeLevelsAreAd
var allScopeValues = otlpLogRecord.Attributes
.Where(_ => _.Key == scopeKey1 || _.Key == scopeKey2)
.Select(_ => _.Value.StringValue);
Assert.Equal(3, otlpLogRecord.Attributes.Count);
Assert.Equal(2, otlpLogRecord.Attributes.Count);
Assert.Equal(2, allScopeValues.Count());
Assert.Contains(scopeValue1, allScopeValues);
Assert.Contains(scopeValue2, allScopeValues);
Expand Down Expand Up @@ -1123,7 +1173,7 @@ public void ToOtlpLog_WhenOptionsIncludeScopesIsTrue_AndScopeIsUsedInLogMethod_C
var allScopeValues = otlpLogRecord.Attributes
.Where(_ => _.Key == scopeKey1 || _.Key == scopeKey2)
.Select(_ => _.Value.StringValue);
Assert.Equal(7, otlpLogRecord.Attributes.Count);
Assert.Equal(2, otlpLogRecord.Attributes.Count);
Assert.Equal(2, allScopeValues.Count());
Assert.Contains(scopeValue1, allScopeValues);
Assert.Contains(scopeValue2, allScopeValues);
Expand Down