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

Mark LogRecord.CategoryName Obsolete #3493

Closed
wants to merge 2 commits into from
Closed

Mark LogRecord.CategoryName Obsolete #3493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4c19a2b
Mark LogRecord.CategoryName Obsolete
alanwest Jul 26, 2022
075aa31
Update changelog
alanwest Jul 26, 2022
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
2 changes: 2 additions & 0 deletions src/OpenTelemetry.Exporter.Console/ConsoleLogRecordExporter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,12 @@ public override ExportResult Export(in Batch<LogRecord> batch)
this.WriteLine($"{"LogRecord.TraceFlags:",-RightPaddingLength}{logRecord.TraceFlags}");
}

#pragma warning disable 618 // LogRecord.CategoryName is obsolete
if (logRecord.CategoryName != null)
{
this.WriteLine($"{"LogRecord.CategoryName:",-RightPaddingLength}{logRecord.CategoryName}");
}
#pragma warning restore 618 // LogRecord.CategoryName is obsolete

this.WriteLine($"{"LogRecord.LogLevel:",-RightPaddingLength}{logRecord.LogLevel}");

Expand Down
10 changes: 0 additions & 10 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 @@ -75,16 +75,6 @@ internal static OtlpLogs.LogRecord ToOtlpLog(this LogRecord logRecord)
SeverityText = LogLevels[(int)logRecord.LogLevel],
};

if (!string.IsNullOrEmpty(logRecord.CategoryName))
{
// TODO:
// 1. Track the following issue, and map CategoryName to Name
// if it makes it to log data model.
// https://github.com/open-telemetry/opentelemetry-specification/issues/2398
// 2. Confirm if this name for attribute is good.
otlpLogRecord.Attributes.AddStringAttribute("dotnet.ilogger.category", logRecord.CategoryName);
}

bool bodyPopulatedFromFormattedMessage = false;
if (logRecord.FormattedMessage != null)
{
Expand Down
2 changes: 1 addition & 1 deletion src/OpenTelemetry.Extensions.Serilog/OpenTelemetrySerilogSink.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public void Emit(LogEvent logEvent)
if (property.Key == Constants.SourceContextPropertyName
&& property.Value is ScalarValue sourceContextValue)
{
data.CategoryName = sourceContextValue.Value as string;
attributes.Add("serilog.source_context", sourceContextValue.Value as string);
}
else if (property.Value is ScalarValue scalarValue)
{
Expand Down
3 changes: 3 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@
* Fix exact match of activity source name when `wildcard` is used.
([#3446](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3446))

* `LogRecord.CategoryName` has been marked obsolete.
([#3493](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3493))

## 1.3.0

Released 2022-Jun-03
Expand Down
1 change: 1 addition & 0 deletions src/OpenTelemetry/Logs/LogRecord.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ public string? TraceState
/// <summary>
/// Gets or sets the log category name.
/// </summary>
[Obsolete("CategoryName may not be exported. Consider adding an attribute to the LogRecord.")]
public string? CategoryName
{
get => this.Data.CategoryName;
Expand Down
6 changes: 5 additions & 1 deletion src/OpenTelemetry/Logs/OpenTelemetryLogger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,18 @@ public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Except
ref LogRecordData data = ref record.Data;

data.TimestampBacking = DateTime.UtcNow;
data.CategoryName = this.categoryName;
data.LogLevel = logLevel;
data.EventId = eventId;
data.Message = provider.IncludeFormattedMessage ? formatter?.Invoke(state, exception) : null;
data.Exception = exception;

LogRecordData.SetActivityContext(ref data, Activity.Current);

LogRecordAttributeList attributes = default;

// TODO: Confirm if this name for attribute is good.
attributes.Add("dotnet.ilogger.category", this.categoryName);
Copy link
Member

Choose a reason for hiding this comment

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

What's the perf drop (from both writer and reader perspective)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea agreed gotta measure this. Before I spend time on that though, I would like to first get a sense whether folks agree that obsoleting CategoryName is the route we want to take.

Copy link
Member

Choose a reason for hiding this comment

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

I put my quick thinking here #3491 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

Is this doing anything? 🤣 Seems to be adding the attribute to the stack and then dropping it on the floor. If we wanted to drop CategoryName & EventId here we would have to turn on the buffering feature so we could add new tags to AttributeStorage. Perf hit would be minor for users already doing buffering/batching but for users doing re-entrant processor to something like ETW (GenevaLogExporter) the perf hit would be significant!

I like @reyang's idea to just wait and see what else is coming and then tackle the weirdness. I would also love to drop LogRecord.State and only expose LogRecord.StateValues. Having the two is super confusing and it is kind of undefined what exporters should do with them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't say I feel all that bad about my application of Cunningham's Law in this PR 😆. I am closing it because you've convinced me of the better option for now: #3491 (comment)


processor.OnEnd(record);

record.ScopeProvider = null;
Expand Down
8 changes: 8 additions & 0 deletions test/OpenTelemetry.Extensions.EventSource.Tests/OpenTelemetryEventSourceLogEmitterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,11 @@ public void OpenTelemetryEventSourceLogEmitterSimpleEventTest()
Assert.Equal(TestEventSource.SimpleEventId, logRecord.EventId.Id);
Assert.Equal(nameof(TestEventSource.SimpleEvent), logRecord.EventId.Name);
Assert.Equal(LogLevel.Warning, logRecord.LogLevel);

#pragma warning disable 618 // LogRecord.CategoryName is obsolete
Assert.Null(logRecord.CategoryName);
#pragma warning restore 618 // LogRecord.CategoryName is obsolete

Assert.Null(logRecord.Exception);

Assert.Equal(default, logRecord.TraceId);
Expand Down Expand Up @@ -222,7 +226,11 @@ public void OpenTelemetryEventSourceLogEmitterComplexEventTest(bool formatMessag
Assert.Equal(TestEventSource.ComplexEventId, logRecord.EventId.Id);
Assert.Equal(nameof(TestEventSource.ComplexEvent), logRecord.EventId.Name);
Assert.Equal(LogLevel.Information, logRecord.LogLevel);

#pragma warning disable 618 // LogRecord.CategoryName is obsolete
Assert.Null(logRecord.CategoryName);
#pragma warning restore 618 // LogRecord.CategoryName is obsolete

Assert.Null(logRecord.Exception);

Assert.Equal(default, logRecord.TraceId);
Expand Down
11 changes: 9 additions & 2 deletions test/OpenTelemetry.Extensions.Serilog.Tests/OpenTelemetrySerilogSinkTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,10 @@ public void SerilogBasicLogTests(bool includeFormattedMessage)
Assert.NotEqual(DateTime.MinValue, logRecord.Timestamp);
Assert.Equal(DateTimeKind.Utc, logRecord.Timestamp.Kind);
Assert.Equal(LogLevel.Information, logRecord.LogLevel);

#pragma warning disable 618 // LogRecord.CategoryName is obsolete
Assert.Null(logRecord.CategoryName);
#pragma warning restore 618 // LogRecord.CategoryName is obsolete

Assert.NotNull(logRecord.StateValues);
Assert.Single(logRecord.StateValues);
Expand Down Expand Up @@ -146,7 +149,7 @@ public void SerilogBasicLogWithActivityTest()
}

[Fact]
public void SerilogCategoryNameTest()
public void SerilogSourceContextTest()
{
List<LogRecord> exportedItems = new();

Expand All @@ -172,7 +175,11 @@ public void SerilogCategoryNameTest()

LogRecord logRecord = exportedItems[0];

Assert.Equal("OpenTelemetry.Extensions.Serilog.Tests.OpenTelemetrySerilogSinkTests", logRecord.CategoryName);
#pragma warning disable 618 // LogRecord.CategoryName is obsolete
Assert.Null(logRecord.CategoryName);
#pragma warning restore 618 // LogRecord.CategoryName is obsolete

Assert.Contains(logRecord.StateValues, kvp => kvp.Key == "serilog.source_context" && (string?)kvp.Value == "OpenTelemetry.Extensions.Serilog.Tests.OpenTelemetrySerilogSinkTests");
}

[Fact]
Expand Down