[DYN-2680] Write logs to std out instead of creating new files for each test. #10651

reddyashish · 2020-05-13T17:32:54Z

Purpose

This PR is in reference to the task: https://jira.autodesk.com/browse/DYN-2680.

In the current design, a log file is created everytime dynamo is started and closed. So during a test run on the VM, around 5000-10000 logs are created. Over time, this is causing performance issues on the VM. We are not using these logs anyways so it is better to avoid it.

After this change, during tests, we write the logs only to std out instead of file or to dynamo console.

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.

Reviewers

@DynamoDS/dynamo

This is to avoid storing these large number of logs on the VM which is affecting its performance over time.

mmisol · 2020-05-13T17:40:25Z

src/DynamoCore/Logging/DynamoLogger.cs

@@ -157,7 +157,7 @@ public IEnumerable<NotificationMessage> StartupNotifications
        /// </summary>
        /// <param name="debugSettings">Debug settings</param>
        /// <param name="logDirectory">Directory path where log file will be written</param>
-        public DynamoLogger(DebugSettings debugSettings, string logDirectory)
+        public DynamoLogger(DebugSettings debugSettings, string logDirectory, Boolean IsTestMode = false)


Should we used chained constructors instead? Optional parameters require a client recompile, so technically this is a breaking change.

NIT: The parameter name should start with lower case: isTestMode

I finally found an official version of this:
https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/breaking-change-rules.md

but It does not seem as comprehensive as:
https://stackoverflow.com/questions/1456785/a-definitive-guide-to-api-breaking-changes-in-net

please review it whenever making changes to public apis.
relevant section:
Adding a parameter with a default value.
though it requires careful reading.

Yes will be changing this to an overloaded constructor. I tried to look if adding a optional parameter is a good design or not for public API's but that link is really comprehensive

mmisol · 2020-05-13T17:46:00Z

src/DynamoCore/Logging/DynamoLogger.cs

+                {
+                    Console.WriteLine(string.Format("{0} : {1}", DateTime.UtcNow.ToString("u"), message));
+                    return;
+                }


Why not just leverage on the existing code for the console logging by setting the level variable to LogLevel.Console if testMode is true?

The LogLevel.Console is configured to write to both the dynamo console and to the log file. So couldn't use it.

You are indeed right. The fact that it does both things having that name is confusing :)

These tests verify that the logs are written to dynamo console and to a file. When we changed the behaviour to write the logs to std out in test mode, there were failing. Fixed them accordingly.

reddyashish · 2020-05-14T13:26:10Z

Fixed a couple of existing tests. These tests verify that the logs are written to dynamo console and to a file. When we changed the behaviour to write the logs to std out in test mode, they were failing. Fixed them accordingly.

mmisol · 2020-05-14T14:08:17Z

test/DynamoCoreTests/Logging/DynamoLoggerTest.cs

-                CorePath = config.DynamoCorePath,
-                HostPath = config.DynamoHostPath,
-                PathResolver = config.PathResolver
-            });


Just to confirm, these params were not needed, right?

yeah, these were null values.

mmisol · 2020-05-14T14:11:49Z

test/DynamoCoreTests/Migration/WorkspaceMigrationsTests.cs

-            Assert.IsTrue(File.Exists(currentLoggerPath));//Check that the log was created successfully
-            Assert.IsTrue(CurrentDynamoModel.Logger.LogText.Contains("migration from 0.5.3.x to 0.6.1.x"));//Checks that the log text contains the migration info
+            //Checks that the console output text contains the migration info
+            Assert.IsTrue(consoleOutput.Contains("migration from 0.5.3.x to 0.6.1.x"));


Maybe for this test we actually want to use the non-test version of the logger? I'm thinking that having a persistent log after a migration is done is desired and should be tested to exist.

The thing is, the dynamo logger is created from the dynamo model when the dynamo model is initialized from the tests. It is a read only property and we cannot create an instance of logger in non test mode here, then add it to the model(only then this log message will be written to a file).

mjkkirschner

please check if this should be disposed. Since the logger is created with each test it could introduce a leak.

mjkkirschner · 2020-05-14T14:51:14Z

test/DynamoCoreTests/Migration/WorkspaceMigrationsTests.cs

+
+                consoleOutput = stringWriter.ToString();
+
+                StreamWriter sw = new StreamWriter(Console.OpenStandardOutput());


should you call dispose on StreamWriter? I believe it implements IDisposable!

sorry, looked too quickly, didn't see this was one test, not the logger!

We can avoid creating the StreamWriter object. Saving the std out beforehand and setting it back.

reddyashish · 2020-05-15T14:17:49Z

Merging this.

Write logs to std out instead of creating new files for each test.

6b4f125

This is to avoid storing these large number of logs on the VM which is affecting its performance over time.

reddyashish changed the title ~~Write logs to std out instead of creating new files for each test.~~ [DYN-2680] Write logs to std out instead of creating new files for each test. May 13, 2020

mmisol reviewed May 13, 2020

View reviewed changes

reddyashish added 2 commits May 13, 2020 17:19

Fix for API break change.

e4e69bc

Fixing the failing tests.

5d3f162

These tests verify that the logs are written to dynamo console and to a file. When we changed the behaviour to write the logs to std out in test mode, there were failing. Fixed them accordingly.

mmisol reviewed May 14, 2020

View reviewed changes

mmisol approved these changes May 14, 2020

View reviewed changes

mjkkirschner requested changes May 14, 2020

View reviewed changes

Avoid the stream writer object, no longer needed.

6a124a7

reddyashish merged commit 888face into DynamoDS:master May 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DYN-2680] Write logs to std out instead of creating new files for each test. #10651

[DYN-2680] Write logs to std out instead of creating new files for each test. #10651

reddyashish commented May 13, 2020 •

edited

Loading

mmisol May 13, 2020

mmisol May 13, 2020

mjkkirschner May 13, 2020

reddyashish May 13, 2020

mmisol May 13, 2020

reddyashish May 13, 2020 •

edited

Loading

mmisol May 13, 2020

reddyashish commented May 14, 2020

mmisol May 14, 2020

reddyashish May 14, 2020

mmisol May 14, 2020

reddyashish May 14, 2020 •

edited

Loading

mjkkirschner left a comment

mjkkirschner May 14, 2020

mjkkirschner May 14, 2020

reddyashish May 14, 2020

reddyashish commented May 15, 2020


		consoleOutput = stringWriter.ToString();

		StreamWriter sw = new StreamWriter(Console.OpenStandardOutput());

[DYN-2680] Write logs to std out instead of creating new files for each test. #10651

[DYN-2680] Write logs to std out instead of creating new files for each test. #10651

Conversation

reddyashish commented May 13, 2020 • edited Loading

Purpose

Declarations

Reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reddyashish May 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reddyashish commented May 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reddyashish May 14, 2020 • edited Loading

Choose a reason for hiding this comment

mjkkirschner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reddyashish commented May 15, 2020

reddyashish commented May 13, 2020 •

edited

Loading

reddyashish May 13, 2020 •

edited

Loading

reddyashish May 14, 2020 •

edited

Loading