Add check to see if a logger is enabled before logging

[tomap]

See commit comments

Thomas

[asbjornu]

I like the idea of this, but wouldn't it be better to move the check for Logger.IsInfoEnabled and similar inside the LogMessage method, so it doesn't spread all over the code base? I like being able to write out log statements without having to remember checking a boolean first.

[DanielRose]

@asbjornu The logger itself already does that. The reason for this commit is so that a call to string.Format() is avoided if the logger is not enabled.

Personally, I don't see the need for this. The memory and CPU requirements for string.Format() are vanishingly small. Last time I profiled (2-3 months ago), more than 99% of the time was spent waiting for git, specifically finding the merge-bases.

[tomap]

Hi, the check is only here to avoid unncessary treatments (access property, evaluate lists, ...)
A good example is
22f5093#diff-240091ccd756d009e1677290734085acR122

A less legitimate example is
22f5093#diff-dc5fcc418fdfb7dd363dd7eeec68adfaL65
where there is no formating of any kind, so the test is unnecessary.

But in some cases, you have to be aware that your debug code is taking precious time for nothnig because it will never be displayed.
The aim of this dev is performance.

As for adding the test inside the debug function, I already did that by pointing the Debug/Warn/Error/... to an empty function when the debug level is disabled.
See
af70c48#diff-4abc10f1875195711870f3564a5d5526R59

[DanielRose]

I appreciate what you want to do. But as part of what GitVersion does, it hits the local git repository (i.e. the disk) very heavily. Depending on the circumstances, it even accesses the network quite a bit. In contrast to that, the time spent on logging (which will be enabled most of the time anyway) is almost immeasurably small. So the upside is very small. The downside is that for every future logging call, you have to remember to check if it is enabled first, plus it clutters the code even more than the call to the logger already does.

[JakeGinnivan]

I think there are better ways to resolve removing the string format. The immediate one which comes to mind is change our logging signature from:

public static Action<string> WriteDebug { get; private set; }

to

public static Action<Func<string>> WriteDebug { get; private set; }

Which would change usage to Logger.WriteDebug(() => Use ${stringInterp} here). Then it can be a cross cutting change?

Going to close this as it conflicts, @tomap if you like the idea of the above change another PR is welcome. Another side effect is we can put heavy debug information into the lambda (git operations).