Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
LogEmitter System.Diagnostics.Tracing extension project #3305
Changes from 8 commits
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
There are no files selected for viewing
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
.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.
Can LogRecordPool be internal for now too if we're keeping LogEmitter and friends internal?
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.
Could be. Only question is do we want to allow users to configure the pool size? The only thing exposed is currently
LogRecordPool.Resize(int size)
.