Add logging API #396
Add logging API #396
Conversation
| public static void Error(this IMySqlConnectorLogger log, string message, params object[] args) => log.Log(MySqlConnectorLogLevel.Error, message, args, null); | ||
| public static void Fatal(this IMySqlConnectorLogger log, string message, params object[] args) => log.Log(MySqlConnectorLogLevel.Fatal, message, args, null); | ||
|
|
||
| public static void Trace(this IMySqlConnectorLogger log, string message, Exception exception) => log.Log(MySqlConnectorLogLevel.Trace, message, null, exception); |
There was a problem hiding this comment.
I'd put the exception before the message so that it can't be confused with the arguments. Or just require that callers use IMySqlConnectorLogger.Log directly.
There was a problem hiding this comment.
Yes, I should have done this.
| sb.Append(m_name); | ||
| sb.Append('\t'); | ||
|
|
||
| if (args == null) |
There was a problem hiding this comment.
I'd use Append instead of AppendFormat if args.Length == 0.
| if (args == null) | ||
| sb.Append(message); | ||
| else | ||
| sb.AppendFormat(message, args); |
There was a problem hiding this comment.
I'd use CultureInfo.InvariantCulture here.
There was a problem hiding this comment.
If only to demonstrate how to correctly write a logging implementation, sure.
|
|
||
| var sb = new StringBuilder(); | ||
| sb.Append(s_levels[(int) level]); | ||
| sb.Append('\t'); |
There was a problem hiding this comment.
Why not just a space? Tabs inconsistently waste horizontal space on a console.
There was a problem hiding this comment.
This is not a "production quality" logger; it's a sample implementation that can be used for testing. To get console output formatted the way you would like, hook IMySqlConnectorLogger up to log4net and use a ConsoleAppender (sample code/NuGet package for that hopefully coming soon).
Why not just a space?
I prefer tabs in my console output; keeps the columns lined up.
| @@ -326,6 +359,14 @@ private ConnectionPool(ConnectionSettings cs) | |||
| cs.LoadBalance == MySqlLoadBalance.Random ? RandomLoadBalancer.Instance : | |||
| cs.LoadBalance == MySqlLoadBalance.FewestConnections ? new FewestConnectionsLoadBalancer(this) : | |||
| (ILoadBalancer) new RoundRobinLoadBalancer(); | |||
|
|
|||
| Id = Interlocked.Increment(ref s_poolId); | |||
| m_logArguments = new object[] { "Pool{0} ".FormatInvariant(Id) }; | |||
There was a problem hiding this comment.
I'd find the log statements more readable if you omitted the trailing space here and included them after {0} elsewhere.
| @@ -350,6 +391,7 @@ public IEnumerable<string> LoadBalance(IReadOnlyList<string> hosts) | |||
| readonly ConnectionPool m_pool; | |||
| } | |||
|
|
|||
| static readonly IMySqlConnectorLogger Log = MySqlConnectorLogManager.CreateLogger("ConnectionPool"); | |||
| @@ -44,10 +52,12 @@ internal sealed class ConnectionPool | |||
| } | |||
| if (session != null) | |||
| { | |||
| Log.Debug("{0}Found an existing session; checking it for validity", m_logArguments); | |||
There was a problem hiding this comment.
Inconsistent capitalization of first word (unless there's meaning I'm missing).
| @@ -37,10 +38,10 @@ public void Log(MySqlConnectorLogLevel level, string message, object[] args = nu | |||
| sb.Append(m_name); | |||
| sb.Append('\t'); | |||
|
|
|||
| if (args == null) | |||
| if (args == null || args.Length == 0) | |||
There was a problem hiding this comment.
Should probably fix the docs as well.
bgrainger
force-pushed
the
logging
branch
2 times, most recently
from
4ed3433
to
91806a3
Compare
| #if NET45 || NET46 | ||
| public IMySqlConnectorLogger CreateLogger(string name) => new Log4netLogger(LogManager.GetLogger(typeof(Log4netLogger).Assembly, "MySqlConnector." + name)); | ||
| #else | ||
| public IMySqlConnectorLogger CreateLogger(string name) => new Log4netLogger(LogManager.GetLogger(typeof(Log4netLogger).GetTypeInfo().Assembly, "MySqlConnector." + name)); |
There was a problem hiding this comment.
Does this not work on NET45 || NET46? Or is this an optimization?
Would it be worth caching the assembly in a static?
There was a problem hiding this comment.
Good call; that code path works for both. I'll add a static.
|
I ran the Benchmark suite before and after; didn't notice any significant increase in time or allocations from these changes. |
Fixes #390.