Conversation
noahfalk
left a comment
noahfalk
left a comment
There was a problem hiding this comment.
I like the overall direction. I didn't look too closely at the implementation details but nothing jumped out as being problematic. If the dotnet-monitor crew want to drill into the details go for it, but looks fine to me.
|
(Let me know if you need me to mash the merge button) |
|
|
||
| private readonly string _formattedMessage; | ||
| private List<KeyValuePair<string, object>> _items = new(); | ||
| private KeyValuePair<string, object>[] _items = new KeyValuePair<string, object>[8]; |
There was a problem hiding this comment.
Would CollectionsMarshal.AsSpan work here?
There was a problem hiding this comment.
Issue is we only have netstandard2.1 target here. CollectionsMarshal needs something newer. Probably not worth messing with the targets for this?
| { | ||
| eventSource.Dynamic.AddCallbackForProviderEvent(LoggingSourceConfiguration.MicrosoftExtensionsLoggingProviderName, "MessageJson", (traceEvent) => { | ||
| if (!activityIdToScope.TryGetValue(traceEvent.ActivityID, out LogScopeItem scopeItem)) | ||
| { |
There was a problem hiding this comment.
I'm assuming you kept the two code paths separate because ultimately we will remove the _logger == null path. However, I'm assuming there's a lot of overlap between our reading of the log events and the new code.
There was a problem hiding this comment.
Ya there is some overlap. But would prefer to leave as-is and spend the time updating consumers to use the new pipeline, remove the old one instead of spending time to make the side-by-side really slick. Thoughts?
src/Microsoft.Diagnostics.Monitoring.EventPipe/Logs/LogRecordException.cs
Outdated
Show resolved
Hide resolved
| { | ||
| private static readonly UTF8Encoding s_Utf8Encoding = new(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true); | ||
| [ThreadStatic] | ||
| private static LogObject[]? s_ScopeStorage; |
There was a problem hiding this comment.
Does this state get cleaned up after each session?
There was a problem hiding this comment.
Define cleaned up 😄 It gets reused over and over. Never given back. So if you listen to logs for 30 seconds and never again the process will have some memory held it doesn't technically need. It isn't going to be a whole bunch of memory, I don't think. Issue or no?
3 participants
The existing
EventLogsPipelineroutes log messages retrieved usingMicrosoft-Extensions-Loggingthrough anILoggershim. There isn't anything necessarily wrong with this, but it adds a lot of extra work and memory pressure to the monitoring host. There also really isn't a way to add data such as Timestamp, TraceId, SpanId, etc., without having to hack ILogger.Working through a prototype of a
dotnet-monitorwhich exports OpenTelemetry format (OTLP) we (@samsp-msft @wiktork and @rajkumar-rangaraj) felt it was better to treat logging as we do the other pipelines and have a simple DTO to store all the log information (independent of ILogger) consumers can use to process logs.What this PR does is add a new interface into
EventLogsPipeline(ILogRecordLogger) which can be used to opt-into this new pipeline behavior.Once consumers have been updated we should be able to remove the old one. But they exist side-by-side for now so nothing currently reliant on the internals breaks.