A logger for Azure WebApps #468
Conversation
| } | ||
| else | ||
| { | ||
| _disposable[index - 2] = disposable; |
There was a problem hiding this comment.
Check index or it will nullref is constructor had 1 or 2 and SetDisposable is called for index >2
|
Updated |
| PassLogEventToInnerSink(logEvent); | ||
| } | ||
| } | ||
| catch(OperationCanceledException) |
|
Updated with some tests |
|
|
||
| namespace Microsoft.Extensions.Logging.Abstractions.Internal | ||
| { | ||
| public sealed class NullLogger : ILogger |
There was a problem hiding this comment.
We shouldn't encourage APIs in the .Internal folder referenced by other projects in the same solution. This either needs to be proper public API or shared source
victorhurdugaci
force-pushed
the
victorhu/azure-logger
branch
from
ff8a120 to
7dc5021
Compare
|
Updated. I would like to push this before my vacation. If you have more comments, please add them, otherwise sign off |
| } | ||
|
|
||
| // Wait for the thread to complete before disposing the resources | ||
| _workerThread.Join(); |
There was a problem hiding this comment.
You might want to be defensive here and add a timeout.
victorhurdugaci
force-pushed
the
victorhu/azure-logger
branch
from
7dc5021 to
e5bf597
Compare
|
Updated |
| /// </summary> | ||
| /// <param name="factory">The extension method argument</param> | ||
| /// <param name="fileSizeLimitMb">A strictly positive value representing the maximum log size in megabytes. Once the log is full, no more message will be appended</param> | ||
| public static ILoggerFactory AddAzureWebAppDiagnostics(this ILoggerFactory factory, int fileSizeLimitMb = FileLoggerProvider.DefaultFileSizeLimitMb) |
There was a problem hiding this comment.
The code configuration will be addressed in #470
|
@nblumhardt can you review? |
victorhurdugaci
force-pushed
the
victorhu/azure-logger
branch
from
e5bf597 to
5d10260
Compare
| if (!string.IsNullOrEmpty(config.BlobContainerUrl)) | ||
| { | ||
| // TODO: Add the blob logger by creating a composite inner logger which calls | ||
| // both loggers |
There was a problem hiding this comment.
Might be good to reference another ticket somewhere for the TODO.
|
Thanks for the loop-in guys. I had a quick run through it (sunny Saturday morning here, so on my way out :-)) and everything looks fine. Added what comments I could. One thing I'd give serious thought to - when the buffer in Given events are already going to be dropped at shut-down if they can't be flushed within 5 seconds, dropping them proactively when the buffer fills up seems reasonable. It may not always make a difference, but seems like it might help save an app that's under heavy load (or faulty I/O) once in a while. I had a shot at implementing this strategy here. Let me know if I'm off the mark. Have a great weekend, |
victorhurdugaci
force-pushed
the
victorhu/azure-logger
branch
from
5d10260 to
b7bcb77
Compare
|
@nblumhardt thanks for taking a look! |
|
Merging this as it is - it works. Opened a few bugs that we'll address but I don't have time to do it now. I can do it when I return form vacation. Here are the issues:
cc @muratg |
WIP implementation for #466. Sending to get some early feedback. Tests are on the way.
Please review @pakrym @kichalla @davidfowl