Fix logs still appearing even when LogLevel is set to none bug#3318
Fix logs still appearing even when LogLevel is set to none bug#3318RubenCerna2079 wants to merge 15 commits intomainfrom
none bug#3318Conversation
|
/azp run |
|
/azp run |
|
/azp run |
|
|
||
| //Flush all logs that were buffered before setting the LogLevel. | ||
| // Important: All logs set before this point should use _logBuffer. | ||
| _logBuffer.FlushToLogger(_logger); |
There was a problem hiding this comment.
If there is an exception raised before we flush the logger here, then the buffer never flushes and any errors are skipped from being logged. similarly if TryGetConfig returns false I dont think we flush the logger for the late configured path.
I think we should use a try/finally block here to ensure that we always flush the logger.
There was a problem hiding this comment.
also, after this is fixed (or if you make it into a followup issue), there will be another issue which is we still have some usage of the console to account for this, for example, errors during deserialization. We will need to adjust console usage to rely on the logger buffer once we know it will flush properly even during a halted startup.
| Environment.SetEnvironmentVariable(RUNTIME_ENVIRONMENT_VAR_NAME, "Test"); | ||
|
|
||
| Assert.IsTrue(ConfigMerger.TryMergeConfigsIfAvailable(fileSystem, loader, new StringLogger(), out string? mergedConfig), "Failed to merge config files"); | ||
| Assert.IsTrue(ConfigMerger.TryMergeConfigsIfAvailable(fileSystem, loader, new StringLogger(), null, out string? mergedConfig), "Failed to merge config files"); |
There was a problem hiding this comment.
name the parameter that is null for better readability
| public static bool TryStartEngineWithOptions(StartOptions options, FileSystemRuntimeConfigLoader loader, IFileSystem fileSystem) | ||
| { | ||
| if (!TryGetConfigForRuntimeEngine(options.Config, loader, fileSystem, out string runtimeConfigFile)) | ||
| if (!TryGetConfigForRuntimeEngine(options.Config, loader, fileSystem, out string runtimeConfigFile, false)) |
There was a problem hiding this comment.
nit: name the argument that is false
| /// Else, it will send the log to the logger. | ||
| /// </summary> | ||
| /// <param name="logLevel">LogLevel of the log.</param> | ||
| /// <param name="message">Message that will be printed in the log.</param> |
There was a problem hiding this comment.
missing description of exception
Aniruddh25
left a comment
There was a problem hiding this comment.
Not clear how using different LogBuffer is helping to fix CLI logs being outputed even after LogLevel is set to None.
| { | ||
| if (cliBuffer is not null) | ||
| { | ||
| cliBuffer.BufferLog(logLevel, message, ex); |
There was a problem hiding this comment.
Why do we need this helper method? The caller is anyway passing both the logger and the logbuffer to this function. Its not like the caller is unaware, if the CliBuffer is null or not, if its a static member that you initialize then it is always going to be valid. You can simply call the BufferLog in the caller itself.
There was a problem hiding this comment.
There are some places where we use it where it is not always necessary as the loggers have already been initialized properly, so we pass the LogBuffer as a null so that it directly logs the message. I will also add this to the description to make it easier to understand.
There was a problem hiding this comment.
In those other callers, where the logger directly logs - you can simply use logger.Log. There you dont need the buffer.
This wrapper is unnecessary, we can simply either flush to buffer or do logger.log in the respective callers.
That way, its easier to read as well - otherwise, in the various callers - the reader is unaware what is going to happen unless they look into the function definition.
none bug
I do not understand this. |
Why make this change?
Closes issue [Bug]: Startup class logger is not properly initialized #3262
The logger for the Startup class is not initialized properly, since this logger is special due to the nature of the Startup class it needs to be continuously updated as DAB initializes. This causes two problems:
Closes issue [Bug]: CLI logs being outputed even after LogLevel is set to none #3256 & [Bug]:
log-levelseems like it is not working when two in config. #3255The CLI logger still outputs some logs even when the LogLevel is set to
none. It is expected that if the LogLevel set isnoneor some other level that shouldn't output theinformationlevel, the logs will not appear.What is this change?
We used the LogBuffer to ensure that all the logs are saved before the loggers are initialized with the proper minimum LogLevel to then flush them. Each of the classes has its own LogBuffer to ensure that all the logs related to that class are not mixed with the logs from other classes.
For the CLI we first check if the LogBuffer is null, to see if we need to save the log inside the buffer or send directly through the logger, we do this since some of the functions that are used before the loggers are initialized are also used after the logger initialization, so we need a way to ensure that the logs aren't saved after we flush the logger as we are not expecting to use it anymore, which means the logs would be lost.
For the Startup section we properly initialized the Startup logger as it was not being initialized properly. Which allows the logs that are saved in the buffer to be properly logged once we flush them.
Startup.cs: Removed LogBuffer from the services, this is necessary since we want each class to have its own LogBuffers to be a separate object so that the logs from the different classes don't get mixed together. Correctly initialized the logger for startup by creating its ownLogLevelInitializerand added the logs that are generated before the startup logger is initialized correctly to a log buffer, so they can be flushed out once the logger is completely initialized.Service/Program.cs: Fixed grammar mistake inisLogLevelOverriddenByCliDynamicLogLevelProvider.cs: Added summary commentsLogBuffer.cs: Changed name fromStartupLogBuffer.csas it is now also used in the CLI. Also added theExceptionsparameter in order to allow for more types of logs to be saved in the buffer.FileSystemRuntimeConfigLoader.cs: MovedLogBufferso that it always starts on its own, and the parameter doesn't need to be passed to it.StartOptions.cs: Add its ownLogBufferand add logs to buffer.ConfigGenerator.cs: SetLogBufferand add all the logs that are generated into the buffer and flush them once the LogLevel is set.CustomLoggerProvider.cs: Allow the minimum LogLevel to be changed.ConfigMerger.cs: Add logs to buffer.How was this tested?
Sample Request(s)