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.

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

[sdk-logs] Update LogRecord to keep CategoryName and Logger in sync #5317

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ internal sealed class OtlpLogRecordTransformer
{
internal static readonly ConcurrentBag<OtlpLogs.ScopeLogs> LogListPool = new();

private const string DefaultScopeName = "";

private readonly SdkLimitOptions sdkLimitOptions;
private readonly ExperimentalOptions experimentalOptions;

Expand Down Expand Up @@ -49,7 +47,7 @@ internal OtlpCollector.ExportLogsServiceRequest BuildExportRequest(
var otlpLogRecord = this.ToOtlpLog(logRecord);
if (otlpLogRecord != null)
{
var scopeName = logRecord.CategoryName ?? logRecord.Logger?.Name ?? DefaultScopeName;
var scopeName = logRecord.Logger.Name;
Copy link
Contributor

Choose a reason for hiding this comment

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

In the tests, we are proving that the values of logRecord.Logger.Name and logRecord.CategoryName are the same. Is there any difference in using one over the other in this context?

Copy link
Member

Choose a reason for hiding this comment

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

OTLPExporter can remain agnostic to ILogger specific CategoryName.

Copy link
Member Author

Choose a reason for hiding this comment

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

No practical difference between the two. This is more of a philosophical change which brings OtlpExporter closer to the spec. The goal being it doesn't know anything about any specific logging framework (ILogger being one of those).

if (!logsByCategory.TryGetValue(scopeName, out var scopeLogs))
{
scopeLogs = this.GetLogListFromPool(scopeName);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
OpenTelemetry.Logs.LoggerProviderBuilderExtensions
OpenTelemetry.Logs.LoggerProviderExtensions
OpenTelemetry.Logs.LogRecord.Logger.get -> OpenTelemetry.Logs.Logger?
OpenTelemetry.Logs.LogRecord.Logger.get -> OpenTelemetry.Logs.Logger!
OpenTelemetry.Logs.LogRecord.Severity.get -> OpenTelemetry.Logs.LogRecordSeverity?
OpenTelemetry.Logs.LogRecord.Severity.set -> void
OpenTelemetry.Logs.LogRecord.SeverityText.get -> string?
Expand Down
4 changes: 4 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@
when configuring a view.
([#5312](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5312))

* Updated `LogRecord` to keep `CategoryName` and `Logger` in sync when using the
experimental Log Bridge API.
[#5317](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5317)

## 1.7.0

Released 2023-Dec-08
Expand Down
29 changes: 29 additions & 0 deletions src/OpenTelemetry/Internal/InstrumentationScopeLogger.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

using System.Collections.Concurrent;
using OpenTelemetry.Logs;

namespace OpenTelemetry.Internal;

internal sealed class InstrumentationScopeLogger : Logger
{
private static readonly ConcurrentDictionary<string, InstrumentationScopeLogger> Cache = new();

private InstrumentationScopeLogger(string name)
: base(name)
{
}

public static InstrumentationScopeLogger Default { get; } = new(string.Empty);

public static InstrumentationScopeLogger GetInstrumentationScopeLoggerForName(string? name)
{
return string.IsNullOrWhiteSpace(name)
? Default
: Cache.GetOrAdd(name!, static n => new(n));
}

public override void EmitLog(in LogRecordData data, in LogRecordAttributeList attributes)
=> throw new NotSupportedException();
}
22 changes: 3 additions & 19 deletions src/OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ internal sealed class OpenTelemetryLogger : ILogger

private readonly LoggerProviderSdk provider;
private readonly OpenTelemetryLoggerOptions options;
private readonly string categoryName;
private readonly InstrumentationScopeLogger instrumentationScope;

internal OpenTelemetryLogger(
LoggerProviderSdk provider,
Expand All @@ -37,7 +37,7 @@ internal OpenTelemetryLogger(

this.provider = provider!;
this.options = options!;
this.categoryName = categoryName!;
this.instrumentationScope = InstrumentationScopeLogger.GetInstrumentationScopeLoggerForName(categoryName);
}

internal IExternalScopeProvider? ScopeProvider { get; set; }
Expand Down Expand Up @@ -65,7 +65,6 @@ public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Except
iloggerData.TraceState = this.options.IncludeTraceState && activity != null
? activity.TraceStateString
: null;
iloggerData.CategoryName = this.categoryName;
iloggerData.EventId = eventId;
iloggerData.Exception = exception;
iloggerData.ScopeProvider = this.options.IncludeScopes ? this.ScopeProvider : null;
Expand Down Expand Up @@ -97,7 +96,7 @@ public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Except
: null;
}

record.Logger = LoggerInstrumentationScope.Instance;
record.Logger = this.instrumentationScope;

processor.OnEnd(record);

Expand Down Expand Up @@ -239,19 +238,4 @@ public void Dispose()
{
}
}

private sealed class LoggerInstrumentationScope : Logger
{
private LoggerInstrumentationScope(string name, string version)
: base(name)
{
this.SetInstrumentationScope(version);
}

public static LoggerInstrumentationScope Instance { get; }
= new("OpenTelemetry", Sdk.InformationalVersion);

public override void EmitLog(in LogRecordData data, in LogRecordAttributeList attributes)
=> throw new NotSupportedException();
}
}
44 changes: 33 additions & 11 deletions src/OpenTelemetry/Logs/LogRecord.cs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ internal LogRecord(
this.ILoggerData = new()
{
TraceState = activity?.TraceStateString,
CategoryName = categoryName,
FormattedMessage = formattedMessage,
EventId = eventId,
Exception = exception,
Expand All @@ -79,6 +78,8 @@ internal LogRecord(

this.AttributeData = stateValues;
}

this.Logger = InstrumentationScopeLogger.GetInstrumentationScopeLoggerForName(categoryName);
}

internal enum LogRecordSource
Expand Down Expand Up @@ -153,16 +154,31 @@ public string? TraceState
set => this.ILoggerData.TraceState = value;
}

#if EXPOSE_EXPERIMENTAL_FEATURES
/// <summary>
/// Gets or sets the log category name.
/// </summary>
/// <remarks>
/// Note: <see cref="CategoryName"/> is only set when emitting logs through <see cref="ILogger"/>.
/// Note: <see cref="CategoryName"/> is an alias for the <see
/// cref="Logger.Name"/> accessed via the <see cref="Logger"/> property.
/// Setting a new value for <see cref="CategoryName"/> will result in a new
/// <see cref="Logger"/> being set.
/// </remarks>
#else
/// <summary>
/// Gets or sets the log category name.
/// </summary>
#endif
public string? CategoryName
{
get => this.ILoggerData.CategoryName;
set => this.ILoggerData.CategoryName = value;
get => this.Logger.Name;
set
{
if (this.Logger.Name != value)
{
this.Logger = InstrumentationScopeLogger.GetInstrumentationScopeLoggerForName(value);
}
}
}

/// <summary>
Expand Down Expand Up @@ -379,18 +395,26 @@ public Exception? Exception

#if EXPOSE_EXPERIMENTAL_FEATURES
/// <summary>
/// Gets the <see cref="Logs.Logger"/> which emitted the <see cref="LogRecord"/>.
/// Gets the <see cref="Logs.Logger"/> associated with the <see
/// cref="LogRecord"/>.
/// </summary>
/// <remarks><inheritdoc cref="Sdk.CreateLoggerProviderBuilder" path="/remarks"/></remarks>
/// <remarks>
/// <para><inheritdoc cref="Sdk.CreateLoggerProviderBuilder" path="/remarks"/></para>
/// Note: When using the Log Bridge API (for example <see
/// cref="Logger.EmitLog(in LogRecordData)"/>) <see cref="Logger"/> is
/// typically the <see cref="Logs.Logger"/> which emitted the <see
/// cref="LogRecord"/> however the value may be different if <see
/// cref="CategoryName"/> is modified.</remarks>
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.LogsBridgeExperimentalApi, UrlFormat = DiagnosticDefinitions.ExperimentalApiUrlFormat)]
#endif
public Logger? Logger { get; internal set; }
public Logger Logger { get; internal set; } = InstrumentationScopeLogger.Default;
#else
/// <summary>
/// Gets or sets the <see cref="Logs.Logger"/> which emitted the <see cref="LogRecord"/>.
/// Gets or sets the <see cref="Logs.Logger"/> associated with the <see
/// cref="LogRecord"/>.
/// </summary>
internal Logger? Logger { get; set; }
internal Logger Logger { get; set; } = InstrumentationScopeLogger.Default;
#endif

/// <summary>
Expand Down Expand Up @@ -523,7 +547,6 @@ private void BufferLogScopes()
internal struct LogRecordILoggerData
{
public string? TraceState;
public string? CategoryName;
public EventId EventId;
public string? FormattedMessage;
public Exception? Exception;
Expand All @@ -536,7 +559,6 @@ public LogRecordILoggerData Copy()
var copy = new LogRecordILoggerData
{
TraceState = this.TraceState,
CategoryName = this.CategoryName,
EventId = this.EventId,
FormattedMessage = this.FormattedMessage,
Exception = this.Exception,
Expand Down
37 changes: 35 additions & 2 deletions test/OpenTelemetry.Tests/Logs/LogRecordTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -979,8 +979,41 @@ public void LogRecordInstrumentationScopeTest()

Assert.NotNull(logRecord);
Assert.NotNull(logRecord.Logger);
Assert.Equal("OpenTelemetry", logRecord.Logger.Name);
Assert.Equal(Sdk.InformationalVersion, logRecord.Logger.Version);
Assert.Equal("OpenTelemetry.Logs.Tests.LogRecordTest", logRecord.Logger.Name);
Assert.Null(logRecord.Logger.Version);
}

[Fact]
public void LogRecordCategoryNameAliasForInstrumentationScopeTests()
{
LogRecord logRecord = new();

Assert.Equal(string.Empty, logRecord.CategoryName);
Assert.Equal(logRecord.CategoryName, logRecord.Logger.Name);

logRecord.CategoryName = "Testing";

Assert.Equal("Testing", logRecord.CategoryName);
Assert.Equal(logRecord.CategoryName, logRecord.Logger.Name);

logRecord.CategoryName = null;

Assert.Equal(string.Empty, logRecord.CategoryName);
Assert.Equal(logRecord.CategoryName, logRecord.Logger.Name);

var exportedItems = new List<LogRecord>();
using (var loggerProvider = Sdk.CreateLoggerProviderBuilder()
.AddProcessor(new BatchLogRecordExportProcessor(new InMemoryExporter<LogRecord>(exportedItems)))
.Build())
{
var logger = loggerProvider.GetLogger("TestName");
logger.EmitLog(default);
}

Assert.Single(exportedItems);

Assert.Equal("TestName", exportedItems[0].CategoryName);
Assert.Equal(exportedItems[0].CategoryName, exportedItems[0].Logger.Name);
}

private static ILoggerFactory InitializeLoggerFactory(out List<LogRecord> exportedItems, Action<OpenTelemetryLoggerOptions> configure = null)
Expand Down
Loading