Filelogging with nlog #999

pmiossec · 2016-10-03T23:53:56Z

Use nlog to log git-tfs logs in a file (even debug logs even if not run in debug which will help to debug AFTER a problem occured)
Happy side effect: add colors in the console

pmiossec · 2016-10-09T17:09:59Z

Is there someone interested by reviewing or testing it? Otherwise I will merge it...

spraints

This looks mostly fine. I like that TextWriter stdout isn't a constructor arg all over the place. In some places (like where there was code to make a progress meter (stdout.Write(".")) or where command-line usage is displayed, it seems like writing to stdout might still be appropriate. That said, I like having just one way to write output, so I'm 👍 on using Trace.Write* for everything. (Maybe the top-level error handler is an exception to this?)

spraints · 2016-10-10T18:42:03Z

GitTfs.VsCommon/TfsHelper.Common.cs

@@ -312,7 +310,8 @@ private void GetRootChangesetForBranch(IList<RootBranch> rootBranches, string tf
                        GetRelevantChangesetBasedOnChangeType(mergedItemsToFirstChangesetInBranchToCreate, tfsPathParentBranch, tfsPathBranchToCreate, out renameFromBranch);

                    AddNewRootBranch(rootBranches, new RootBranch(rootChangesetInParentBranch, tfsPathBranchToCreate));
-                    Trace.WriteLineIf(renameFromBranch != null, "Found original branch '" + renameFromBranch + "' (renamed in branch '" + tfsPathBranchToCreate + "')");
+                    if(renameFromBranch != null)
+                        Trace.WriteLine("Found original branch '" + renameFromBranch + "' (renamed in branch '" + tfsPathBranchToCreate + "')");
                    if (renameFromBranch != null)
                        GetRootChangesetForBranch(rootBranches, renameFromBranch);


Can these two if statements be combined?

spraints · 2016-10-10T18:44:08Z

GitTfs.VsCommon/TfsHelper.Common.cs

            do
            {
-                _stdout.Write(".");
+                //Trace.TraceInformation(".");


Maybe just delete this line?

spraints · 2016-10-10T18:49:34Z

GitTfs/Core/GitHelpers.cs

-            CommandOutputPipe(stdout => realStdout.Write(stdout.ReadToEnd()), command);
+            var textWriter = new StringWriter();
+            CommandOutputPipe(stdout => textWriter.Write(stdout.ReadToEnd()), command);
+            Trace.TraceInformation(textWriter.ToString());


Why buffer it? Would a one-liner still work?

CommandOutputPipe(stdout => Trace.TraceInformation(stdout.ReadToEnd()), command);

spraints · 2016-10-10T18:52:21Z

GitTfs/GitTfs.cs

            }
        }

        public void InitializeGlobals()
        {
+            _globals.Logger = _logger;


What uses _globals.Logger? Could it be injected in constructors rather than assigned here?

spraints · 2016-10-10T18:55:24Z

GitTfs/GitTfs.cs

            Trace.WriteLine("Command run:" + _globals.CommandLineRun);
+            InitializeGlobals();


The call to InitializeGlobals() feels like it should be at the beginning of Run(), so that ParseOptions can update globals if desired. Why move it here?

spraints · 2016-10-10T18:58:21Z

GitTfs/GitTfs.cs

            var unparsedArgs 
10000
= ParseOptions(command, args);
+            UpdateLoggerOnDebugging();


This is totally a style-nit, but why not continue to set up the logger for debugging in the setter for Globals.DebugOutput? It could be factored much better, but it's nice to have that contained in Globals so that this class can stay focussed on delegating things to the right command object.

I don't remember what is the reason. This development stayed a long time in my repository :(

I have fixed everything except that. Not sure if really needed....

It's not needed.

spraints · 2016-10-10T19:02:06Z

GitTfs/Program.cs

                    foreach (var solution in gitTfsException.RecommendedSolutions)
                    {
-                        Console.WriteLine("- " + solution);
+                        Trace.TraceError("- " + solution);


Is there any chance that nlog won't be configured to show errors by this point? For example, if GetLogger() throws an exception, maybe because the log file can't be written to, what will happen here? I'd like to err on the side of these messages always being shown.

spraints · 2016-10-10T19:18:21Z

GitTfsTest/BaseTest.cs

+            if (!System.Diagnostics.Debugger.IsAttached)
+            {
+                System.Diagnostics.Trace.Listeners.Clear();
+            }


Is there a way to do this that doesn't require every test to subclass BaseTest, or do anything at all? It'd be nice to be able to add new test classes without worrying about including this bit of setup.
8000
Looking around a bit, it looks like the answer to my question is no, or not really. A static initializer might work, but a static initializer might not run often enough(?) and might run at the wrong time, too. It looks like there's an IUseFixture thing that would make it so that the test classes derive from an interface instead of a class, which is marginally better, but also harder to look at and understand.

So consider this comment a rant that you don't need to do anything about. Unless you have a good idea of how to make it better.

Not a big problem for me. If we forget to ingherit from BaseTest, that's just that the build will outpout some logs. Not a big deal and if it is annoying it will be quickly fixed...

spraints · 2016-10-10T19:23:11Z

Happy side effect: add colors in the console

:) :)

Is there someone interested by reviewing or testing it? Otherwise I will merge it...

Sorry for the delay. There was a lot to look through, so I skipped over this when you first opened it. I left some comments now.

pmiossec · 2016-10-10T22:53:51Z

Sorry for the delay.

No problem

I left some comments now.

👍 I will try to fix all that as soon as possible...

spraints

🤘

and enable it only when a debugger is attached...

+ cleaning

pmiossec force-pushed the filelogging_with_nlog branch from 80d854b to 6ea86ff Compare October 4, 2016 00:15

spraints reviewed Oct 10, 2016

View reviewed changes

spraints approved these changes Oct 13, 2016

View reviewed changes

pmiossec added 14 commits October 18, 2016 21:20

Add Nlog dependency

5b0fddd

Using NLog to save the logs in a filelog AND display them in the console

ae43f94

Redirect System.Diagnostic.Trace to Nlog

6faa02a

Using Diagnostics.Trace instead of TextWriter stdout

c66f4fb

Reorganize startup to log earlier

73a7ecd

Disable Trace in Tests (due to xunit2)

0789239

and enable it only when a debugger is attached...

Clear Trace listeners....

51dae40

fix nlog paket ref

182422c

Display log file path to the user

737a65c

Improve log format (date & level)

c1d07f5

Some fixes

9128e9f

Display log file path when an error occured

22a7867

Log in the console when unable to use Nlog

9cbe55c

+ cleaning

Update release note

7fefecb

pmiossec force-pushed the filelogging_with_nlog branch from e607649 to 7fefecb Compare October 18, 2016 19:23

pmiossec merged commit 101fa7e into git-tfs:master Oct 18, 2016

pmiossec deleted the filelogging_with_nlog branch October 18, 2016 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Filelogging with nlog #999

Filelogging with nlog #999

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		Trace.WriteLine("Command run:" + _globals.CommandLineRun);
		InitializeGlobals();

		var unparsedArgs 10000 = ParseOptions(command, args);
		UpdateLoggerOnDebugging();

Filelogging with nlog #999

Filelogging with nlog #999

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!