-
Notifications
You must be signed in to change notification settings - Fork 786
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
Changes from 1 commit
6d2f153
101ce19
7eb0006
20479f5
c64ca11
53a6854
fd5a87e
035823b
80cbdbb
744c399
df93cf5
1ba8afe
9a0ee32
888eb54
558e8d2
4d76b87
617d88d
ca7e319
6fae76d
a92e78e
5e33474
6b15b3b
2114dca
0d34533
bb4293c
380f139
87597f8
345e1a2
ea63af4
a12d432
3b35268
57775a5
9ee18df
f347e51
5537c73
e0c2347
dd80cc9
4f6b9e0
1dcd193
226facf
d3c5443
31f5a97
cb69e49
8693d93
b71006f
6269015
7647145
4f498af
55537b5
d1a4b3b
ffda1ce
12c92ad
cf69b8c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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! |
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! |
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; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
opentelemetry-dotnet/src/OpenTelemetry/Logs/OpenTelemetryLoggingExtensions.cs
Line 46 in 2114dca
Users will have to find some way to access their
OpenTelemetryLoggerProvider
to pass to whatever library so that it can construct aLogEmitter
. I'll mess with what that might look like.There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is my suggestion:
internal
)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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 willLogRecord.GetDataRef()
which gives exporters a way to get at the data more quickly and also gives users a way to mutate everything onLogRecord
(which has been requested).If
LogRecordAttributeList
goes internal, no real downside withoutLogEmitter
. I do have this in there...opentelemetry-dotnet/src/OpenTelemetry/Logs/OpenTelemetryLogger.cs
Line 93 in 2114dca
What that allows you to do is pass
LogRecordAttributeList
asTState
toILogger.Log<TState>
and get boxing/allocation-free tags. If it goes internal, users won't be able to easily build upon that perf-path.There was a problem hiding this comment.
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?There was a problem hiding this comment.
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. LeftLogRecordData
&LogRecord.GetDataRef()
public for now.The primary goal of
LogRecordData
wasn't really performance. Originally I hadLogEmitter.Log(LogRecord logRecord)
as the API. But that created some problems. It meant user had to interact with the pool correctly. Switching the API toLogEmitter.Log(in LogRecordData)
allowed for better encapsulation/safety. I think havingstruct 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 makeLogRecord.GetDataRef
internal, and just expose public setters for everything onLogRecord
(or not, but I think users want to be able to mutate everything). The only downside to removingGetDataRef
is exporters will take a perf hit because most things onLogRecord
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 onLogRecord
.