-
Notifications
You must be signed in to change notification settings - Fork 447
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
Deferred Log Dispatcher #10530
Deferred Log Dispatcher #10530
Conversation
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.
Some comments which hopefully remove the need to touch the other loggers.
Also, this approach will not capture log scopes. Is that okay? Or do you want to buffer scope data as well? Just make sure to capture scope data as a string and not their raw types (they may capture async state which we don't want)
src/WebJobs.Script.WebHost/Diagnostics/DeferredLogDispatcher.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script.WebHost/Diagnostics/DeferredLogDispatcher.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script.WebHost/Diagnostics/DeferredLogDispatcher.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script.WebHost/Diagnostics/IDeferredLogDispatcher.cs
Outdated
Show resolved
Hide resolved
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.
ILoggerProvider
approach looks good. However, lets worry about capturing and re-emitting scope in a separate PR.
BUT I still recommend using the ISupportExternalScope
for the ILoggerProvider
and ILogger
implementation in this PR.
src/WebJobs.Script.WebHost/Diagnostics/DeferredLoggerProvider.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script.WebHost/Diagnostics/DeferredLoggerProvider.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script.WebHost/Diagnostics/DeferredLoggerProvider.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script.WebHost/Diagnostics/WebHostSystemLoggerProvider.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script.WebHost/Diagnostics/DeferredLoggerProvider.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Script.WebHost/Diagnostics/DeferredLoggerProvider.cs
Outdated
Show resolved
Hide resolved
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.
Thank you, @RohitRanjanMS
The new iteration looks good, but I'm still concerned about the placeholder logs question.
Have you validated this functionality in placeholder? I want to make sure warmup site logs won't end up in customer facing Application Insights logs.
The initial telemetry from the host startup is missing in Application Insights because the ApplicationInsightsLoggerProvider is added later in the process. As a result, important startup logs are not captured in App Insights. The proposed solution addresses this by temporarily storing logs in a channel and forwarding them to Application Insights and/or OpenTelemetry once these providers are added to the logging system.
resolves #9679 and #9361
Pull request checklist
IMPORTANT: Currently, changes must be backported to the
in-proc
branch to be included in Core Tools and non-Flex deployments.in-proc
branch is not requiredrelease_notes.md