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

LogEmitter System.Diagnostics.Tracing extension project #3305

Closed
wants to merge 53 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
6d2f153
LogEmitter API.
CodeBlanch May 24, 2022
101ce19
Code review.
CodeBlanch May 25, 2022
7eb0006
Added LogRecordPool.
CodeBlanch May 26, 2022
20479f5
Nullable annotation updates.
CodeBlanch May 26, 2022
c64ca11
Merge branch 'main' into log-emitter
CodeBlanch May 26, 2022
53a6854
Cleanup.
CodeBlanch May 26, 2022
fd5a87e
Cleanup.
CodeBlanch May 26, 2022
035823b
Added reference counting into the log record pool.
CodeBlanch May 26, 2022
80cbdbb
Tweaks.
CodeBlanch May 28, 2022
744c399
Tweak.
CodeBlanch May 29, 2022
df93cf5
Test fix.
CodeBlanch May 29, 2022
1ba8afe
Merge branch 'main' into log-emitter
CodeBlanch May 29, 2022
9a0ee32
Test fix.
CodeBlanch May 29, 2022
888eb54
Rename.
CodeBlanch May 30, 2022
558e8d2
Trigger state buffering by processor inspection.
CodeBlanch May 31, 2022
4d76b87
Implement copy for in-memory log exporter.
CodeBlanch May 31, 2022
617d88d
Added GetDataRef.
CodeBlanch May 31, 2022
ca7e319
Tweaks.
CodeBlanch May 31, 2022
6fae76d
Merge branch 'main' into log-emitter
CodeBlanch May 31, 2022
a92e78e
Revert CompositeProcessor change.
CodeBlanch May 31, 2022
5e33474
Add log stress tests to solution.
CodeBlanch May 31, 2022
6b15b3b
Tweaks.
CodeBlanch May 31, 2022
2114dca
Code review.
CodeBlanch Jun 1, 2022
0d34533
Code review, example app, serilog + eventsource extensions.
CodeBlanch Jun 2, 2022
bb4293c
Rename.
CodeBlanch Jun 2, 2022
380f139
Typo.
CodeBlanch Jun 2, 2022
87597f8
New pool design + tests.
CodeBlanch Jun 6, 2022
345e1a2
Pool selection based on processor.
CodeBlanch Jun 6, 2022
ea63af4
Merge from main.
CodeBlanch Jun 7, 2022
a12d432
Update public api files.
CodeBlanch Jun 7, 2022
3b35268
Public api fix.
CodeBlanch Jun 7, 2022
57775a5
Lint and race comment.
CodeBlanch Jun 7, 2022
9ee18df
Comments in log emitter example app.
CodeBlanch Jun 7, 2022
f347e51
Switch to Volatile.Read.
CodeBlanch Jun 7, 2022
5537c73
Bump Microsoft.DotNet.ApiCompat.
CodeBlanch Jun 7, 2022
e0c2347
Typo fix.
CodeBlanch Jun 7, 2022
dd80cc9
Bump Microsoft.DotNet.ApiCompat.
CodeBlanch Jun 7, 2022
4f6b9e0
Attempting to fix ApiCompat failure.
CodeBlanch Jun 7, 2022
1dcd193
Tweak ApiCompatExcludeAttributeList path.
CodeBlanch Jun 7, 2022
226facf
Exclude NullableContextAttribute from ApiCompat.
CodeBlanch Jun 7, 2022
d3c5443
Merge from main.
CodeBlanch Jun 15, 2022
31f5a97
Merge from main.
CodeBlanch Jun 16, 2022
cb69e49
Fix merge.
CodeBlanch Jun 16, 2022
8693d93
Merge from main.
CodeBlanch Jun 17, 2022
b71006f
Updates.
CodeBlanch Jun 17, 2022
6269015
Merge from main.
CodeBlanch Jun 18, 2022
7647145
Revert OtlpLogExporter use of logRecord.GetDataRef because it is now …
CodeBlanch Jun 18, 2022
4f498af
Merge branch 'main' into log-emitter
cijothomas Jun 27, 2022
55537b5
Merge from main.
CodeBlanch Jun 30, 2022
d1a4b3b
Merge fix.
CodeBlanch Jun 30, 2022
ffda1ce
Merge from main.
CodeBlanch Jul 11, 2022
12c92ad
Merge from main.
CodeBlanch Jul 18, 2022
cf69b8c
Updates.
CodeBlanch Jul 18, 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
18 changes: 16 additions & 2 deletions examples/AspNetCore/Controllers/WeatherForecastController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,28 @@
namespace Examples.AspNetCore.Controllers;

using Microsoft.AspNetCore.Mvc;
using OpenTelemetry.Logs;

[ApiController]
[Route("[controller]")]
public class WeatherForecastController : ControllerBase
{
private static readonly string[] Summaries = new[]
{
"Freezing", "Bracing", "Chilly", "Cool", "Mild", "Warm", "Balmy", "Hot", "Sweltering", "Scorching",
"Freezing", "Bracing", "Chilly", "Cool", "Mild", "Warm", "Balmy", "Hot", "Sweltering", "Scorching",
};

private static readonly HttpClient HttpClient = new();

private readonly ILogger<WeatherForecastController> logger;
private readonly LogEmitter logEmitter;

public WeatherForecastController(ILogger<WeatherForecastController> logger)
public WeatherForecastController(
ILogger<WeatherForecastController> logger,
LogEmitter logEmitter)
{
this.logger = logger ?? throw new ArgumentNullException(nameof(logger));
this.logEmitter = logEmitter ?? throw new ArgumentNullException(nameof(logEmitter));
}

[HttpGet]
Expand All @@ -54,11 +59,20 @@ public IEnumerable<WeatherForecast> Get()
})
.ToArray();

// Log using ILogger API.
this.logger.LogInformation(
"WeatherForecasts generated {count}: {forecasts}",
forecast.Length,
forecast);

// Log using LogEmitter API.
this.logEmitter.Log(new(
categoryName: "WeatherForecasts",
timestamp: DateTime.UtcNow,
logLevel: LogLevel.Information,
message: "WeatherForecasts generated.",
stateValues: new List<KeyValuePair<string, object?>>() { new KeyValuePair<string, object?>("count", forecast.Length) }));

return forecast;
}
}
5 changes: 5 additions & 0 deletions src/OpenTelemetry/.publicApi/net462/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
#nullable enable
OpenTelemetry.Logs.LogEmitter
Copy link
Member

Choose a reason for hiding this comment

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

Would we want to consider putting this in a separate SDK package specifically to highlight that this is meant for logging extension authors?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm down for whatever. It is part of the SDK spec so I feel like it should be part of the SDK assembly, but I don't feel passionately about it. If it does live in some other assembly we won't be able to do this:

builder.Services.TryAddSingleton<LogEmitter>();

Users will have to find some way to access their OpenTelemetryLoggerProvider to pass to whatever library so that it can construct a LogEmitter. I'll mess with what that might look like.

Copy link
Member

Choose a reason for hiding this comment

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

That's what I was trying to wrap my head around. Would regular users need to be able to do this? What's are the intended use cases for an end user using a LogEmitter?

The kind of use case I'm imagining is one where the user might not need to interact directly with the LogEmitter class and instead is using serilog or something. So, I was thinking the need to get a handle on the LogEmitter would be the responsibility of the serilog extension.

Copy link
Member

@reyang reyang Jun 1, 2022

Choose a reason for hiding this comment

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

Here is my suggestion:

  1. Use this PR to focus on the LogRecord improvements (including pooling, ref count, setters) - this would give good perf benefits, and set a cornerstone for LogEmitter or any bridge with Serilog/NLog/EventSource/etc.
  2. LogEmitter can either be a separate PR or included here, as long as the scope of the change is not too big + is self-contained (e.g. if LogEmitter is need to make meaningful perf tests, probably fine to keep it, but it has to be kept internal)
  3. Before the spec becomes stable, we can either keep LogEmitter internal (for our own testing purpose), or making it public AND having it in a separate package (similar to OTLP Log Exporter) - if we know someone who is interested in trying it out.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. 👍 to making LogEmitter internal and leaving it where it is in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

  • Let's make it internal and keep where it is. We can figure out where to move it if needed before it goes public?

  • What shall I do with LogRecordData + LogRecordAttributeList?

    • If LogRecordData goes internal, so to will LogRecord.GetDataRef() which gives exporters a way to get at the data more quickly and also gives users a way to mutate everything on LogRecord (which has been requested).

    • If LogRecordAttributeList goes internal, no real downside without LogEmitter. I do have this in there...

      if (state is LogRecordAttributeList logRecordAttributes)

      What that allows you to do is pass LogRecordAttributeList as TState to ILogger.Log<TState> and get boxing/allocation-free tags. If it goes internal, users won't be able to easily build upon that perf-path.

Copy link
Member

Choose a reason for hiding this comment

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

👍 to LogRecordData/GetDataRef being public since that seems to be the main story regarding perf improvement.

I do not have a strong opinion about whether to make LogRecordAttributeList public at this moment. Maybe just leave it internal for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated LogRecordAttributeList is now internal. Left LogRecordData & LogRecord.GetDataRef() public for now.

The primary goal of LogRecordData wasn't really performance. Originally I had LogEmitter.Log(LogRecord logRecord) as the API. But that created some problems. It meant user had to interact with the pool correctly. Switching the API to LogEmitter.Log(in LogRecordData) allowed for better encapsulation/safety. I think having struct LogRecordData is providing some perf benefit, but it wasn't the primary driver. There is a comment below specifically about the perf, I'm going to add more detail there.

We could make LogRecordData internal, remove or make LogRecord.GetDataRef internal, and just expose public setters for everything on LogRecord (or not, but I think users want to be able to mutate everything). The only downside to removing GetDataRef is exporters will take a perf hit because most things on LogRecord are now accessed via the struct. Handing back a ref to the struct is just a faster way to get all the data at time of export. It also allows users to mutate everything without also needing public setters on LogRecord.

OpenTelemetry.Logs.LogEmitter.Log(OpenTelemetry.Logs.LogRecord! logRecord) -> void
OpenTelemetry.Logs.LogEmitter.LogEmitter(OpenTelemetry.Logs.OpenTelemetryLoggerProvider! loggerProvider) -> void
OpenTelemetry.Logs.LogRecord.FormattedMessage.set -> void
OpenTelemetry.Logs.LogRecord.LogRecord(string! categoryName, System.DateTime timestamp, Microsoft.Extensions.Logging.LogLevel logLevel, string! message, Microsoft.Extensions.Logging.EventId eventId = default(Microsoft.Extensions.Logging.EventId), System.Exception? exception = null, System.Collections.Generic.IReadOnlyList<System.Collections.Generic.KeyValuePair<string!, object?>>? stateValues = null) -> void
OpenTelemetry.Logs.LogRecord.State.set -> void
OpenTelemetry.Logs.LogRecord.StateValues.set -> void
OpenTelemetry.Logs.OpenTelemetryLoggerProvider.CreateEmitter() -> OpenTelemetry.Logs.LogEmitter!
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
#nullable enable
OpenTelemetry.Logs.LogEmitter
OpenTelemetry.Logs.LogEmitter.Log(OpenTelemetry.Logs.LogRecord! logRecord) -> void
OpenTelemetry.Logs.LogEmitter.LogEmitter(OpenTelemetry.Logs.OpenTelemetryLoggerProvider! loggerProvider) -> void
OpenTelemetry.Logs.LogRecord.FormattedMessage.set -> void
OpenTelemetry.Logs.LogRecord.LogRecord(string! categoryName, System.DateTime timestamp, Microsoft.Extensions.Logging.LogLevel logLevel, string! message, Microsoft.Extensions.Logging.EventId eventId = default(Microsoft.Extensions.Logging.EventId), System.Exception? exception = null, System.Collections.Generic.IReadOnlyList<System.Collections.Generic.KeyValuePair<string!, object?>>? stateValues = null) -> void
OpenTelemetry.Logs.LogRecord.State.set -> void
OpenTelemetry.Logs.LogRecord.StateValues.set -> void
OpenTelemetry.Logs.OpenTelemetryLoggerProvider.CreateEmitter() -> OpenTelemetry.Logs.LogEmitter!
67 changes: 67 additions & 0 deletions src/OpenTelemetry/Logs/LogEmitter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// <copyright file="LogEmitter.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

#nullable enable

using OpenTelemetry.Internal;

namespace OpenTelemetry.Logs
{
/// <summary>
/// LogEmitter implementation.
/// </summary>
/// <remarks>
/// Spec reference: <a href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/logging-library-sdk.md#logemitter">LogEmitter</a>.
/// </remarks>
public sealed class LogEmitter
{
private readonly OpenTelemetryLoggerProvider loggerProvider;

/// <summary>
/// Initializes a new instance of the <see cref="LogEmitter"/> class.
/// </summary>
/// <param name="loggerProvider"><see cref="OpenTelemetryLoggerProvider"/>.</param>
public LogEmitter(OpenTelemetryLoggerProvider loggerProvider)
{
Guard.ThrowIfNull(loggerProvider);

this.loggerProvider = loggerProvider;
}

/// <summary>
/// Emit a <see cref="LogRecord"/>.
/// </summary>
/// <param name="logRecord"><see cref="LogRecord"/>.</param>
public void Log(LogRecord logRecord)
{
Guard.ThrowIfNull(logRecord);

var provider = this.loggerProvider;
var processor = provider.Processor;
if (processor != null)
{
if (provider.IncludeScopes)
{
logRecord.ScopeProvider = provider.ScopeProvider;
}

processor.OnEnd(logRecord);

logRecord.ScopeProvider = null;
}
}
}
}
23 changes: 23 additions & 0 deletions src/OpenTelemetry/Logs/LogRecord.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,29 @@ public sealed class LogRecord

private List<object?>? bufferedScopes;

public LogRecord(
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved
string categoryName,
DateTime timestamp,
LogLevel logLevel,
string message,
EventId eventId = default,
Exception? exception = null,
IReadOnlyList<KeyValuePair<string, object?>>? stateValues = null)
: this(
scopeProvider: null,
timestamp.Kind == DateTimeKind.Local ? timestamp.ToUniversalTime() : timestamp,
categoryName,
logLevel,
eventId,
message,
state: null,
exception,
stateValues)
{
Guard.ThrowIfNullOrEmpty(categoryName);
Guard.ThrowIfNullOrEmpty(message);
}

internal LogRecord(
IExternalScopeProvider? scopeProvider,
DateTime timestamp,
Expand Down
6 changes: 6 additions & 0 deletions src/OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,12 @@ public ILogger CreateLogger(string categoryName)
return logger;
}

/// <summary>
/// Create a <see cref="LogEmitter"/>.
/// </summary>
/// <returns><see cref="LogEmitter"/>.</returns>
public LogEmitter CreateEmitter() => new(this);
Copy link
Member

Choose a reason for hiding this comment

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

Do you consider LogEmitter as a pub-sub model? (similar like ActivitySource - which can be subscribed by multiple listeners)

Copy link
Member Author

Choose a reason for hiding this comment

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

I hadn't thought about that. There isn't much in the spec, but it seems mainly to be a way to just throw logs at a processor/exporter without using a log library. I think it would be nice/useful to do this, but should we pursue through the spec first?


internal OpenTelemetryLoggerProvider AddProcessor(BaseProcessor<LogRecord> processor)
{
Guard.ThrowIfNull(processor);
Expand Down
6 changes: 5 additions & 1 deletion src/OpenTelemetry/Logs/OpenTelemetryLoggingExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@ public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, Act
Guard.ThrowIfNull(builder);

builder.AddConfiguration();
builder.Services.TryAddEnumerable(ServiceDescriptor.Singleton<ILoggerProvider, OpenTelemetryLoggerProvider>());
builder.Services.TryAddSingleton<OpenTelemetryLoggerProvider>();
builder.Services.TryAddSingleton<LogEmitter>();
builder.Services.TryAddEnumerable(
ServiceDescriptor.Singleton<ILoggerProvider, OpenTelemetryLoggerProvider>(
sp => sp.GetRequiredService<OpenTelemetryLoggerProvider>()));

if (configure != null)
{
Expand Down