Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DYN-5755 : restrict logging in service mode #13860

Merged
merged 2 commits into from
Apr 4, 2023
Merged

DYN-5755 : restrict logging in service mode #13860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c051a9
restrict logging in service mode
zavub Mar 30, 2023
fdd242f
add todo
zavub Apr 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/DynamoCLI/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,9 @@ private static void RunKeepAlive(StartupUtils.CommandLineArguments cmdLineArgs)
Console.WriteLine("Press Enter to shutdown...");
}
}
catch
catch(Exception ex)
{
Console.WriteLine("Server is shutting down due to an error");
Console.WriteLine("Server is shutting down due to an error : " + ex.ToString());
}
}

Expand Down
50 changes: 48 additions & 2 deletions src/DynamoCore/Logging/DynamoLogger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ public class DynamoLogger: NotificationObject, ILogger, IDisposable
private bool _isDisposed;
private readonly bool testMode;
private readonly bool cliMode;
private readonly bool serviceMode;

private TextWriter FileWriter { get; set; }
private StringBuilder ConsoleWriter { get; set; }
Expand Down Expand Up @@ -159,7 +160,7 @@ public IEnumerable<NotificationMessage> StartupNotifications
/// </summary>
/// <param name="debugSettings">Debug settings</param>
/// <param name="logDirectory">Directory path where log file will be written</param>
[Obsolete("This will be removed in 3.0, please use DynamoLogger(debugSettings, logDirectory, isTestMode) instead.")]
[Obsolete("This will be removed in 3.0, please use DynamoLogger(debugSettings, logDirectory, isTestMode, isCLIMode, isServiceMode) instead.")]
public DynamoLogger(DebugSettings debugSettings, string logDirectory) : this(debugSettings, logDirectory, false)
{

Expand All @@ -172,6 +173,7 @@ public DynamoLogger(DebugSettings debugSettings, string logDirectory) : this(deb
/// <param name="debugSettings">Debug settings</param>
/// <param name="logDirectory">Directory path where log file will be written</param>
/// <param name="isTestMode">Test mode is true or false.</param>
[Obsolete("This will be removed in 3.0, please use DynamoLogger(debugSettings, logDirectory, isTestMode, isCLIMode, isServiceMode) instead.")]
public DynamoLogger(DebugSettings debugSettings, string logDirectory, Boolean isTestMode)
{
lock (guardMutex)
Expand Down Expand Up @@ -203,6 +205,7 @@ public DynamoLogger(DebugSettings debugSettings, string logDirectory, Boolean is
/// <param name="logDirectory">Directory path where log file will be written</param>
/// <param name="isTestMode">Test mode is true or false.</param>
/// <param name="isCLIMode">We want to allow logging when CLI mode is true even if we are in test mode.</param>
[Obsolete("This will be removed in 3.0, please use DynamoLogger(debugSettings, logDirectory, isTestMode, isCLIMode, isServiceMode) instead.")]
public DynamoLogger(DebugSettings debugSettings, string logDirectory, Boolean isTestMode, Boolean isCLIMode)
:this(debugSettings, logDirectory, isTestMode)
{
Expand All @@ -213,6 +216,42 @@ public DynamoLogger(DebugSettings debugSettings, string logDirectory, Boolean is
}
}

/// <summary>
/// Initializes a new instance of <see cref="DynamoLogger"/> class
/// with specified debug settings and directory where to write logs
/// </summary>
/// <param name="debugSettings">Debug settings</param>
/// <param name="logDirectory">Directory path where log file will be written</param>
/// <param name="isTestMode">Test mode is true or false.</param>
/// <param name="isCLIMode">We want to allow logging when CLI mode is true even if we are in test mode.</param>
/// <param name="isServiceMode">We want restrict logging in service mode to console only due to lambda limitations.</param>
/// TODO(DYN-5757): Review usage of isTestMode,isTestMode,isServiceMode across Dynamo and see how we can consildate all these flags.
public DynamoLogger(DebugSettings debugSettings, string logDirectory, Boolean isTestMode, Boolean isCLIMode, Boolean isServiceMode)
: this(debugSettings, logDirectory, isTestMode)
{
lock (guardMutex)
{
this.debugSettings = debugSettings;
_isDisposed = false;

WarningLevel = WarningLevel.Mild;
Warning = "";

notifications = new List<NotificationMessage>();

testMode = isTestMode;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should merge all these flags somehow at some point

Copy link
Member

@mjkkirschner mjkkirschner Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you throw in an obsolete message and a TODO + Dynamo 3.0 it will be easier to find later when we want to refactor all these signatures throughout the code base.

cliMode = isCLIMode;
serviceMode = isServiceMode;

if (!testMode && !isServiceMode)
{
StartLoggingToConsoleAndFile(logDirectory);
}

XmlDocumentationExtensions.LogToConsole += Log;
}
}

/// <summary>
/// Logs the specified message.
/// </summary>
Expand Down Expand Up @@ -244,6 +283,12 @@ private void Log(string message, LogLevel level, bool reportModification)
return;
}

if (serviceMode && (level == LogLevel.Console || level == LogLevel.File))
{
ConsoleWriter.AppendLine("LogLevel switched to ConsoleOnly in service mode");
level = LogLevel.ConsoleOnly;
}

switch (level)
{
//write to the console only
Expand Down Expand Up @@ -444,7 +489,8 @@ private void StartLoggingToConsoleAndFile(string logDirectory)
{
lock (this.guardMutex)
{
if (FileWriter != null && ConsoleWriter != null)
if (serviceMode ||
(FileWriter != null && ConsoleWriter != null))
{
return;
}
Expand Down
8 changes: 7 additions & 1 deletion src/DynamoCore/Models/DynamoModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,7 @@ protected DynamoModel(IStartConfiguration config)
IsHeadless = config.IsHeadless;

DebugSettings = new DebugSettings();
Logger = new DynamoLogger(DebugSettings, pathManager.LogDirectory, IsTestMode, CLIMode);
Logger = new DynamoLogger(DebugSettings, pathManager.LogDirectory, IsTestMode, CLIMode, IsServiceMode);

if (!IsServiceMode)
{
Expand Down Expand Up @@ -2418,6 +2418,12 @@ private void RegisterHomeWorkspace(HomeWorkspaceModel newWorkspace)
/// </summary>
protected void SaveBackupFiles(object state)
{
//No backup files in ServiceMode due to Lambda restrictions
if (IsServiceMode)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice to have a test for this if there are already backup tests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do

{
return;
}

OnRequestDispatcherBeginInvoke(() =>
{
// tempDict stores the list of backup files and their corresponding workspaces IDs
Expand Down