Updated InternalLogger to Create Directories If Needed #818

kevindaub · 2015-07-26T16:13:44Z

Updated the InternalLogger to create directories if they are needed.

Resolves #802

Updated the InternalLogger to create directories if they are needed.

304NotModified · 2015-07-26T17:27:35Z

src/NLog/Common/InternalLogger.cs

@@ -320,6 +320,7 @@ private static void Write(LogLevel level, string message, object[] args)
                var logFile = LogFile;
                if (!string.IsNullOrEmpty(logFile))
                {
+                    CreateDirectoriesIfNeeded(logFile);


Please move to constructor (performance)

Unfortunately, that will make my unit test moot, but agree with the performance.

304NotModified · 2015-07-26T17:29:13Z

Thanks! Please check the first note.

304NotModified · 2015-07-26T18:42:23Z

That's a lot of failed tests :/

kevindaub · 2015-07-26T18:54:01Z

That's a lot better. I forgot to remove out the unit test. Thoughts on moving all the static logic to an instance class that is initialized as a singleton. I think that will give you the performance and the unit testibility.

304NotModified · 2015-07-26T21:46:11Z

I really liked the unit test. I forgot that calling the method in a static ctor is difficult to mock. But we can fix this :)

New proposal:

move the code to the setter (InternalLogger.LogFile)
restore unit test
please change the XML comments of LogFile to: (it's a path, not a file name)

        /// <summary>
        /// Gets or sets the file path of the internal log file.
        /// </summary>
        /// <remarks>A value of <see langword="null" /> value disables internal logging to a file.</remarks>
        public static string LogFile { get; set; }

This is even better then, because we also need the directories when changing the LogFile setting (and I see that the XML config is calling: InternalLogger.LogFile = nlogElement.GetOptionalAttribute("internalLogFile", InternalLogger.LogFile);

Thanks in advance!

Per the new proposal, moved the directory creation to the LogFile property setter. Added back the unit test and moved the asserts a bit.

304NotModified · 2015-07-26T22:25:28Z

I see the failing tests. Dunno why they fail. Maybe you can change Assert.True with Assert.Equal in those cases. (which is better)

kevindaub · 2015-07-27T00:33:03Z

The failed unit test was not my unit test. However, I do notice that the tests in there seem to randomly fail. It varies on my computer which one fails. My changes seems to have made the tests unstable. It might be threading since it's a static class. I'll dig in a bit more.

304NotModified · 2015-07-27T07:41:04Z

yes, there are some tests which has race conditions. But that aren't those two failing tests. Do those tests successfully run on your local computer?

kevindaub · 2015-07-27T09:35:48Z

My test seems to make the race conditions worse. As long as my test doesn't
get executed, things are fine. Perhaps making my unit test a skip?

On Monday, July 27, 2015, Julian Verdurmen notifications@github.com wrote:

yes, there are some tests which has race conditions. But that aren't those
two failing tests. Do those tests successfully run on your local computer?

—
Reply to this email directly or view it on GitHub
#818 (comment).

Sent from Gmail Mobile

304NotModified · 2015-07-27T10:40:07Z

I think you should restore the original values of InternalLogger at then end of your unit test.

InternalLogger.LogLevel
InternalLogger.IncludeTimestamp
InternalLogger.LogFile

it's not a race condition, but InternalLogger is a static "global" and so the new unit test has (unwanted) side effects

The CreateDirectoriesIfNeededTests causes unwanted side effect by causing the other tests (or this one) to randomly fail. To get around this, we set the InternalLogger.LogFile back to the previous log file, which resolves the issue.

kevindaub · 2015-07-28T00:17:23Z

Resetting the LogFile back to the previous value resolved the issue. It worked repeatedly on my machine, which it was not doing before. Thanks for the help.

kevindaub · 2015-08-07T00:24:31Z

Are we good with this pull request?

304NotModified · 2015-08-07T15:02:15Z

src/NLog/Common/InternalLogger.cs

+            }
+            catch (Exception exception)
+            {
+                if (exception.MustBeRethrown())


We need an log to the internallog here, because the internal log can also be to console.

Do you mean something like this?

try { Directory.CreateDirectory(Path.GetDirectoryName(filename)); } catch (Exception exception) { InternalLogger.Error("Cannot create needed directories {0}, Exception : {1}", Path.GetDirectoryName(filename), exception); if (exception.MustBeRethrown()) { throw; } }

304NotModified · 2015-08-07T15:02:54Z

Looks good! one small thing (because 'empty' catches are confusing) - see the code note. Can you fix that?

Added logging to internal logging. Unfortunately, this will cause a lot of logs because the Write method calls LogFile, which is the source call.

This is needed because this functionality does not exist for Silverlight.

kevindaub · 2015-08-09T23:17:56Z

Added a log if we are not rethrowing the exception. But I think this actually might cause issues unless we log only to the console. Whether we rethrow or not, the Write method uses the LogFile, which will probably not let that method finish. Thoughts?

304NotModified · 2015-08-10T21:55:23Z

You're worried about infinite loops? I don't think this is a problem, but maybe we should unit test it to be sure.

304NotModified · 2015-08-10T21:56:46Z

tests/NLog.UnitTests/Common/InternalLoggerTests.cs

+            {
+                // Reset LogFile to the previous value
+                InternalLogger.LogFile = previousLogFile;
+


please revert all the InternalLogger properties, to be sure. Maybe we should create a helper for that.

See #839

kevindaub · 2015-08-10T23:08:58Z

I did a small test myself and my fear was lacking any solid basis. I did a small update. Besides your last comment. I think it's good.

Unsure what that last error is. Was there something new that was added?

The compiler was generating a type that was a private nested type, which was missing the ThreadAgnosticAttribute. Because it was auto-generated there was not a way to add it.

kevindaub · 2015-08-10T23:58:36Z

Added an additional check for the failing test. It looked like there was inner type that was auto generated by the compiler for the ReplaceLayoutRendererWrapper. Running the test again with the additional check of t.IsPrivateNested showed that this was the only generated class.

304NotModified · 2015-08-12T21:34:27Z

I will fix #839 soon and then we can use it in this PR.

304NotModified · 2015-08-12T21:35:12Z

I recently enabled the Silverlight builds on appveyor. Probably this will cause the build errors

304NotModified · 2015-08-12T21:44:55Z

@kevindaub

I created a separate PR for your fix. (#844). Thanks for the fix!

It should not lead to merge conflicts in this PR.

kevindaub · 2015-08-12T21:48:04Z

Great. Let me know if you need anything after the other pull request is
merged.

On Wednesday, August 12, 2015, Julian Verdurmen notifications@github.com
wrote:

@kevindaub https://github.com/kevindaub

I created a separate PR for your fix. (#844
#844). Thanks for the fix!

It should not lead to merge conflicts in this PR.

—
Reply to this email directly or view it on GitHub
#818 (comment).

Sent from Gmail Mobile

304NotModified · 2015-08-12T21:49:02Z

Yes I let you know. :)

it's almost fixed :)

304NotModified · 2015-08-12T22:34:40Z

#844 isn't working as expected. So we don;t have to change something in this PR for now. .

Updated InternalLogger to Create Directories If Needed

Updated InternalLogger to Create Directories If Needed

474c9ff

Updated the InternalLogger to create directories if they are needed.

304NotModified reviewed Jul 26, 2015
View reviewed changes

304NotModified added the enhancement Improvement on existing feature label Jul 26, 2015

304NotModified added this to the 4.1 milestone Jul 26, 2015

304NotModified added the could Have label Jul 26, 2015

304NotModified self-assigned this Jul 26, 2015

Moved Call to CreateDirectoriesIfNeeded to Constructor

09ddc0f

Reverted Out CreateDirectoriesIfNeeded Test

83a3aba

kevindaub added 2 commits July 26, 2015 18:02

Moved Needed Directory Creation to LogFile Property Setter

1be8f41

Per the new proposal, moved the directory creation to the LogFile property setter. Added back the unit test and moved the asserts a bit.

Updated InternalLogger.LogFile Xml Documentation

b22b763

Reset InternalLogger.LogFile to Previous Log File

8482062

The CreateDirectoriesIfNeededTests causes unwanted side effect by causing the other tests (or this one) to randomly fail. To get around this, we set the InternalLogger.LogFile back to the previous log file, which resolves the issue.

304NotModified reviewed Aug 7, 2015
View reviewed changes

Added Log to Internal Logger if Not Rethrowing

a5492ca

Added logging to internal logging. Unfortunately, this will cause a lot of logs because the Write method calls LogFile, which is the source call.

kevindaub force-pushed the create_directories_when_needed branch from 8bd8c47 to a5492ca Compare August 9, 2015 23:09

Added Compiler Flag for CreateDirectoriesIfNeeded

4e3ab08

This is needed because this functionality does not exist for Silverlight.

304NotModified reviewed Aug 10, 2015
View reviewed changes

Moved Log Message For Create Directories If Needed Error

e6a3d2b

Added Check for Nested Private Types

731c475

The compiler was generating a type that was a private nested type, which was missing the ThreadAgnosticAttribute. Because it was auto-generated there was not a way to add it.

304NotModified added a commit that referenced this pull request Aug 12, 2015

Merge pull request #818 from kevindaub/create_directories_when_needed

2fb2fb5

Updated InternalLogger to Create Directories If Needed

304NotModified merged commit 2fb2fb5 into NLog:master Aug 12, 2015

kevindaub deleted the create_directories_when_needed branch August 17, 2015 21:51

304NotModified mentioned this pull request Sep 4, 2015

Internal NLog log: path is required since 4.1 #895

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated InternalLogger to Create Directories If Needed #818

Updated InternalLogger to Create Directories If Needed #818

kevindaub commented Jul 26, 2015

304NotModified Jul 26, 2015

kevindaub Jul 26, 2015

304NotModified commented Jul 26, 2015

304NotModified commented Jul 26, 2015

kevindaub commented Jul 26, 2015

304NotModified commented Jul 26, 2015

304NotModified commented Jul 26, 2015

kevindaub commented Jul 27, 2015

304NotModified commented Jul 27, 2015

kevindaub commented Jul 27, 2015

304NotModified commented Jul 27, 2015

kevindaub commented Jul 28, 2015

kevindaub commented Aug 7, 2015

304NotModified Aug 7, 2015

kevindaub Aug 7, 2015

304NotModified commented Aug 7, 2015

kevindaub commented Aug 9, 2015

304NotModified commented Aug 10, 2015

304NotModified Aug 10, 2015

kevindaub commented Aug 10, 2015

kevindaub commented Aug 10, 2015

304NotModified commented Aug 12, 2015

304NotModified commented Aug 12, 2015

304NotModified commented Aug 12, 2015

kevindaub commented Aug 12, 2015

304NotModified commented Aug 12, 2015

304NotModified commented Aug 12, 2015

Updated InternalLogger to Create Directories If Needed #818

Updated InternalLogger to Create Directories If Needed #818

Conversation

kevindaub commented Jul 26, 2015

304NotModified Jul 26, 2015

Choose a reason for hiding this comment

kevindaub Jul 26, 2015

Choose a reason for hiding this comment

304NotModified commented Jul 26, 2015

304NotModified commented Jul 26, 2015

kevindaub commented Jul 26, 2015

304NotModified commented Jul 26, 2015

304NotModified commented Jul 26, 2015

kevindaub commented Jul 27, 2015

304NotModified commented Jul 27, 2015

kevindaub commented Jul 27, 2015

304NotModified commented Jul 27, 2015

kevindaub commented Jul 28, 2015

kevindaub commented Aug 7, 2015

304NotModified Aug 7, 2015

Choose a reason for hiding this comment

kevindaub Aug 7, 2015

Choose a reason for hiding this comment

304NotModified commented Aug 7, 2015

kevindaub commented Aug 9, 2015

304NotModified commented Aug 10, 2015

304NotModified Aug 10, 2015

Choose a reason for hiding this comment

kevindaub commented Aug 10, 2015

kevindaub commented Aug 10, 2015

304NotModified commented Aug 12, 2015

304NotModified commented Aug 12, 2015

304NotModified commented Aug 12, 2015

kevindaub commented Aug 12, 2015

304NotModified commented Aug 12, 2015

304NotModified commented Aug 12, 2015