Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ILogger for ConnectionMultiplexer #2051

Merged
merged 44 commits into from Aug 19, 2023
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
6d1b16f
Options cleanup and defaulting
NickCraver Mar 19, 2022
6608c95
Move to uncloned ConfigurationOptions
NickCraver Mar 20, 2022
0cf1e75
Simplify Validate
NickCraver Mar 20, 2022
c70d83a
Update comments per-memoized config options
NickCraver Mar 20, 2022
ab4ed36
Reflect HighPrioritySocketThreads as retired
NickCraver Mar 20, 2022
581d88f
Obsolete message updates
NickCraver Mar 20, 2022
c7070be
Add mutability tests
NickCraver Mar 20, 2022
073bd08
Comment tweaks
NickCraver Mar 20, 2022
a8c934c
Add release notes
NickCraver Mar 20, 2022
84f2b96
Promote obsoletes for 3.0 deprecation
NickCraver Mar 20, 2022
f34ac25
WIP: ILogger bits!
NickCraver Mar 22, 2022
7629f71
Merge branch 'main' into craver/ilogger
NickCraver Mar 29, 2022
d1f905f
Merge remote-tracking branch 'origin/main' into craver/ilogger
NickCraver Oct 24, 2022
0b5c814
Merge in NRTs
NickCraver Oct 24, 2022
75f298b
Add initial test bits
NickCraver Oct 24, 2022
56cd4c4
Add co Clone/DefaultOptions and some test checks on that
NickCraver Oct 24, 2022
e32f4a2
When *we* close the socket, it's an info message not an error
NickCraver Oct 24, 2022
1c24b77
Merge branch 'main' into craver/ilogger
NickCraver Oct 24, 2022
befb0fe
Make some test more resilient
NickCraver Oct 26, 2022
5f7e0f3
Merge remote-tracking branch 'origin/main' into craver/ilogger
NickCraver Oct 26, 2022
c91422e
Merge branch 'main' into craver/ilogger
NickCraver May 9, 2023
4e2a6ad
Fix new log line
NickCraver May 9, 2023
7142d6c
Merge remote-tracking branch 'origin/main' into craver/ilogger
NickCraver Jun 20, 2023
a76ceca
Move to ILoggerFactory to support more use cases.
NickCraver Jun 20, 2023
ee7450f
Merge remote-tracking branch 'origin/main' into craver/ilogger
NickCraver Jul 1, 2023
c5fb56a
Revert "Move to ILoggerFactory to support more use cases."
NickCraver Jul 4, 2023
30fee1b
Memoize on ConnectionMultiplexer
NickCraver Jul 4, 2023
9614fbe
Reduce LogProxy dependence
NickCraver Jul 4, 2023
87051de
Refactor LogProxy => ILogger wrapper
NickCraver Jul 4, 2023
cab1f78
Fix GetStatus
NickCraver Jul 4, 2023
6d4a6c6
Add wrapping example
NickCraver Jul 4, 2023
d23b0cb
Tests: up tolerance on AbortOnConnectFailTests
NickCraver Jul 4, 2023
03820fb
Add to docs
NickCraver Jul 9, 2023
4a314c2
Merge remote-tracking branch 'origin/main' into craver/ilogger
NickCraver Jul 10, 2023
aef0d49
Eliminate double timestamps + simplify
NickCraver Jul 10, 2023
b53f2aa
Fix disposal on TextWriter handles
NickCraver Jul 25, 2023
168c4f6
Merge remote-tracking branch 'origin/main' into craver/ilogger
NickCraver Jul 28, 2023
ceeb3b0
Merge remote-tracking branch 'origin/main' into craver/ilogger
NickCraver Jul 28, 2023
a4ded47
Fix logging, add more for Role test that's failing
NickCraver Aug 4, 2023
d0c9295
ILoggerFactory on the config side
NickCraver Aug 8, 2023
25c658f
AppVeyor: add .trx to artifacts
NickCraver Aug 12, 2023
99bfc5e
Upgrade actions/setup-dotnet
NickCraver Aug 12, 2023
94b7030
Doc updates!
NickCraver Aug 19, 2023
c1b54cf
Merge remote-tracking branch 'origin/main' into craver/ilogger
NickCraver Aug 19, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Directory.Packages.props
Expand Up @@ -12,6 +12,7 @@
<PackageVersion Include="BenchmarkDotNet" Version="0.13.1" />
<PackageVersion Include="GitHubActionsTestLogger" Version="1.3.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.PublicApiAnalyzers" Version="3.3.3" />
<PackageVersion Include="Microsoft.Extensions.Logging.Abstractions" Version="6.0.0" />
<PackageVersion Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.2" />
<PackageVersion Include="Microsoft.NET.Test.Sdk" Version="17.1.0" />
<PackageVersion Include="Microsoft.SourceLink.GitHub" Version="1.1.1" />
Expand Down
12 changes: 12 additions & 0 deletions src/StackExchange.Redis/ConfigurationOptions.cs
Expand Up @@ -10,6 +10,7 @@
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
using StackExchange.Redis.Configuration;

namespace StackExchange.Redis
Expand Down Expand Up @@ -159,6 +160,8 @@ public static string TryNormalize(string value)

private BacklogPolicy backlogPolicy;

private ILogger logger;

/// <summary>
/// A LocalCertificateSelectionCallback delegate responsible for selecting the certificate used for authentication; note
/// that this cannot be specified in the configuration-string.
Expand Down Expand Up @@ -410,6 +413,15 @@ public int KeepAlive
set => keepAlive = value;
}

/// <summary>
/// The <see cref="ILogger"/> to use for connection events.
/// </summary>
public ILogger Logger
{
get => logger;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add to the default options as well

set => logger = value;
}

/// <summary>
/// The user to use to authenticate with the server.
/// </summary>
Expand Down
2 changes: 1 addition & 1 deletion src/StackExchange.Redis/ConnectionMultiplexer.Sentinel.cs
Expand Up @@ -349,7 +349,7 @@ internal void SwitchPrimary(EndPoint switchBlame, ConnectionMultiplexer connecti
{
if (log == null) log = TextWriter.Null;

using (var logProxy = LogProxy.TryCreate(log))
using (var logProxy = LogProxy.TryCreate(log, RawConfig))
{
string serviceName = connection.RawConfig.ServiceName;

Expand Down
140 changes: 72 additions & 68 deletions src/StackExchange.Redis/ConnectionMultiplexer.cs

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions src/StackExchange.Redis/EndPointCollection.cs
Expand Up @@ -209,12 +209,12 @@ internal async Task ResolveEndPointsAsync(ConnectionMultiplexer multiplexer, Log
}
else
{
log?.WriteLine($"Using DNS to resolve '{dns.Host}'...");
log?.LogInfo($"Using DNS to resolve '{dns.Host}'...");
var ips = await Dns.GetHostAddressesAsync(dns.Host).ObserveErrors().ForAwait();
if (ips.Length == 1)
{
ip = ips[0];
log?.WriteLine($"'{dns.Host}' => {ip}");
log?.LogInfo($"'{dns.Host}' => {ip}");
cache[dns.Host] = ip;
this[i] = new IPEndPoint(ip, dns.Port);
}
Expand All @@ -223,7 +223,7 @@ internal async Task ResolveEndPointsAsync(ConnectionMultiplexer multiplexer, Log
catch (Exception ex)
{
multiplexer.OnInternalError(ex);
log?.WriteLine(ex.Message);
log?.LogError(ex, ex.Message);
}
}
}
Expand Down
15 changes: 0 additions & 15 deletions src/StackExchange.Redis/ExceptionFactory.cs
@@ -1,6 +1,5 @@
using System;
using System.Collections.Generic;
using System.Reflection;
using System.Text;
using System.Threading;

Expand Down Expand Up @@ -389,19 +388,5 @@ internal static Exception UnableToConnect(ConnectionMultiplexer muxer, string fa

return new RedisConnectionException(ConnectionFailureType.UnableToConnect, sb.ToString());
}

internal static Exception BeganProfilingWithDuplicateContext(object forContext)
{
var exc = new InvalidOperationException("Attempted to begin profiling for the same context twice");
exc.Data["forContext"] = forContext;
return exc;
}

internal static Exception FinishedProfilingWithInvalidContext(object forContext)
{
var exc = new InvalidOperationException("Attempted to finish profiling for a context which is no longer valid, or was never begun");
exc.Data["forContext"] = forContext;
return exc;
}
}
}
77 changes: 61 additions & 16 deletions src/StackExchange.Redis/LogProxy.cs
@@ -1,50 +1,77 @@
using System;
using System.IO;
using Microsoft.Extensions.Logging;

namespace StackExchange.Redis;

internal sealed class LogProxy : IDisposable
{
public static LogProxy TryCreate(TextWriter writer)
=> writer == null ? null : new LogProxy(writer);
public static LogProxy TryCreate(TextWriter writer, ConfigurationOptions options)
=> writer == null && options?.Logger == null
? null
: new LogProxy(writer, options?.Logger);

public override string ToString()
private TextWriter _log;
private ILogger _logger;
internal static Action<string> NullWriter = _ => { };
public object SyncLock => this;

private LogProxy(TextWriter log, ILogger logger)
{
string s = null;
if (_log != null)
_log = log;
_logger = logger;
}

public void WriteLine()
{
if (_log != null) // note: double-checked
{
lock (SyncLock)
{
s = _log?.ToString();
_log?.WriteLine();
}
}
return s ?? base.ToString();
}
private TextWriter _log;
internal static Action<string> NullWriter = _ => { };

public object SyncLock => this;
private LogProxy(TextWriter log) => _log = log;
public void WriteLine()
public void LogInfo(string message = null)
{
var msg = $"{DateTime.UtcNow:HH:mm:ss.ffff}: {message}";
if (_log != null) // note: double-checked
NickCraver marked this conversation as resolved.
Show resolved Hide resolved
{
lock (SyncLock)
{
_log?.WriteLine();
_log?.WriteLine(msg);
}
}
_logger?.LogInformation(msg);
}

public void LogTrace(string message)
{
var msg = $"{DateTime.UtcNow:HH:mm:ss.ffff}: {message}";
if (_log != null) // note: double-checked
{
lock (SyncLock)
{
_log?.WriteLine(msg);
}
}
_logger?.LogTrace(msg);
}
public void WriteLine(string message = null)

public void LogError(Exception ex, string message)
{
var msg = $"{DateTime.UtcNow:HH:mm:ss.ffff}: {message}";
if (_log != null) // note: double-checked
{
lock (SyncLock)
{
_log?.WriteLine($"{DateTime.UtcNow:HH:mm:ss.ffff}: {message}");
_log?.WriteLine(msg);
}
}
_logger?.LogError(ex, msg);
}

public void WriteLine(string prefix, string message)
{
if (_log != null) // note: double-checked
Expand All @@ -55,11 +82,29 @@ public void WriteLine(string prefix, string message)
}
}
}

public override string ToString()
{
string s = null;
if (_log != null)
{
lock (SyncLock)
{
s = _log?.ToString();
}
}
return s ?? base.ToString();
}

public void Dispose()
{
if (_log != null) // note: double-checked
{
lock (SyncLock) { _log = null; }
lock (SyncLock)
{
_log = null;
}
}
_logger = null;
}
}
2 changes: 1 addition & 1 deletion src/StackExchange.Redis/Message.cs
Expand Up @@ -37,7 +37,7 @@ protected override void WriteImpl(PhysicalConnection physical)
try
{
var bridge = physical.BridgeCouldBeNull;
log?.WriteLine($"{bridge.Name}: Writing: {tail.CommandAndKey}");
log?.LogTrace($"{bridge.Name}: Writing: {tail.CommandAndKey}");
}
catch { }
tail.WriteTo(physical);
Expand Down
12 changes: 8 additions & 4 deletions src/StackExchange.Redis/PhysicalBridge.cs
Expand Up @@ -6,6 +6,7 @@
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
#if !NETCOREAPP
using Pipelines.Sockets.Unofficial.Threading;
using static Pipelines.Sockets.Unofficial.Threading.MutexSlim;
Expand Down Expand Up @@ -384,7 +385,7 @@ internal async Task OnConnectedAsync(PhysicalConnection connection, LogProxy log
if (physical == connection && !isDisposed && ChangeState(State.Connecting, State.ConnectedEstablishing))
{
await ServerEndPoint.OnEstablishingAsync(connection, log).ForAwait();
log?.WriteLine($"{Format.ToString(ServerEndPoint)}: OnEstablishingAsync complete");
log?.LogInfo($"{Format.ToString(ServerEndPoint)}: OnEstablishingAsync complete");
}
else
{
Expand All @@ -408,6 +409,7 @@ internal void ResetNonConnected()

internal void OnConnectionFailed(PhysicalConnection connection, ConnectionFailureType failureType, Exception innerException)
{
Multiplexer.RawConfig.Logger?.LogError(innerException, innerException.Message);
Trace($"OnConnectionFailed: {connection}");
// If we're configured to, fail all pending backlogged messages
if (Multiplexer.RawConfig.BacklogPolicy?.AbortPendingOnConnectionFailure == true)
Expand Down Expand Up @@ -527,7 +529,9 @@ internal void OnHeartbeat(bool ifConnectedOnly)
if (shouldRetry)
{
Interlocked.Increment(ref connectTimeoutRetryCount);
LastException = ExceptionFactory.UnableToConnect(Multiplexer, "ConnectTimeout");
var ex = ExceptionFactory.UnableToConnect(Multiplexer, "ConnectTimeout");
LastException = ex;
Multiplexer.RawConfig.Logger?.LogError(ex, ex.Message);
Trace("Aborting connect");
// abort and reconnect
var snapshot = physical;
Expand Down Expand Up @@ -1291,7 +1295,7 @@ public PhysicalConnection TryConnect(LogProxy log)
{
if (!Multiplexer.IsDisposed)
{
log?.WriteLine($"{Name}: Connecting...");
log?.LogInfo($"{Name}: Connecting...");
Multiplexer.Trace("Connecting...", Name);
if (ChangeState(State.Disconnected, State.Connecting))
{
Expand All @@ -1308,7 +1312,7 @@ public PhysicalConnection TryConnect(LogProxy log)
}
catch (Exception ex)
{
log?.WriteLine($"{Name}: Connect failed: {ex.Message}");
log?.LogError(ex, $"{Name}: Connect failed: {ex.Message}");
Multiplexer.Trace("Connect failed: " + ex.Message, Name);
ChangeState(State.Disconnected);
OnInternalError(ex);
Expand Down
18 changes: 10 additions & 8 deletions src/StackExchange.Redis/PhysicalConnection.cs
Expand Up @@ -15,6 +15,7 @@
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
using Pipelines.Sockets.Unofficial;
using Pipelines.Sockets.Unofficial.Arenas;

Expand Down Expand Up @@ -91,15 +92,15 @@ internal async Task BeginConnectAsync(LogProxy log)
var endpoint = bridge?.ServerEndPoint?.EndPoint;
if (endpoint == null)
{
log?.WriteLine("No endpoint");
log?.LogError(new ArgumentNullException(nameof(endpoint)), "No endpoint");
return;
}

Trace("Connecting...");
_socket = SocketManager.CreateSocket(endpoint);
bridge.Multiplexer.RawConfig.BeforeSocketConnect?.Invoke(endpoint, bridge.ConnectionType, _socket);
bridge.Multiplexer.OnConnecting(endpoint, bridge.ConnectionType);
log?.WriteLine($"{Format.ToString(endpoint)}: BeginConnectAsync");
log?.LogInfo($"{Format.ToString(endpoint)}: BeginConnectAsync");

CancellationTokenSource timeoutSource = null;
try
Expand Down Expand Up @@ -143,7 +144,7 @@ internal async Task BeginConnectAsync(LogProxy log)
}
else if (await ConnectedAsync(x, log, bridge.Multiplexer.SocketManager).ForAwait())
{
log?.WriteLine($"{Format.ToString(endpoint)}: Starting read");
log?.LogInfo($"{Format.ToString(endpoint)}: Starting read");
try
{
StartReading();
Expand All @@ -161,9 +162,9 @@ internal async Task BeginConnectAsync(LogProxy log)
Shutdown();
}
}
catch (ObjectDisposedException)
catch (ObjectDisposedException ex)
{
log?.WriteLine($"{Format.ToString(endpoint)}: (socket shutdown)");
log?.LogError(ex, $"{Format.ToString(endpoint)}: (socket shutdown)");
try { RecordConnectionFailed(ConnectionFailureType.UnableToConnect, isInitialConnect: true); }
catch (Exception inner)
{
Expand Down Expand Up @@ -1400,7 +1401,7 @@ internal async ValueTask<bool> ConnectedAsync(Socket socket, LogProxy log, Socke

if (config.Ssl)
{
log?.WriteLine("Configuring TLS");
log?.LogInfo("Configuring TLS");
var host = config.SslHost;
if (string.IsNullOrWhiteSpace(host)) host = Format.ToStringHostOnly(bridge.ServerEndPoint.EndPoint);

Expand All @@ -1418,9 +1419,10 @@ internal async ValueTask<bool> ConnectedAsync(Socket socket, LogProxy log, Socke
{
Debug.WriteLine(ex.Message);
bridge.Multiplexer?.SetAuthSuspect();
bridge.Multiplexer.RawConfig.Logger?.LogError(ex, ex.Message);
throw;
}
log?.WriteLine($"TLS connection established successfully using protocol: {ssl.SslProtocol}");
log?.LogInfo($"TLS connection established successfully using protocol: {ssl.SslProtocol}");
}
catch (AuthenticationException authexception)
{
Expand All @@ -1438,7 +1440,7 @@ internal async ValueTask<bool> ConnectedAsync(Socket socket, LogProxy log, Socke

_ioPipe = pipe;

log?.WriteLine($"{bridge?.Name}: Connected ");
log?.LogInfo($"{bridge?.Name}: Connected ");

await bridge.OnConnectedAsync(this, log).ForAwait();
return true;
Expand Down
2 changes: 2 additions & 0 deletions src/StackExchange.Redis/PublicAPI.Shipped.txt
Expand Up @@ -227,6 +227,8 @@ StackExchange.Redis.ConfigurationOptions.IncludePerformanceCountersInExceptions.
StackExchange.Redis.ConfigurationOptions.IncludePerformanceCountersInExceptions.set -> void
StackExchange.Redis.ConfigurationOptions.KeepAlive.get -> int
StackExchange.Redis.ConfigurationOptions.KeepAlive.set -> void
StackExchange.Redis.ConfigurationOptions.Logger.get -> Microsoft.Extensions.Logging.ILogger
StackExchange.Redis.ConfigurationOptions.Logger.set -> void
StackExchange.Redis.ConfigurationOptions.Password.get -> string
StackExchange.Redis.ConfigurationOptions.Password.set -> void
StackExchange.Redis.ConfigurationOptions.PreserveAsyncOrder.get -> bool
Expand Down
4 changes: 2 additions & 2 deletions src/StackExchange.Redis/RedisServer.cs
Expand Up @@ -330,7 +330,7 @@ public Task<DateTime> LastSaveAsync(CommandFlags flags = CommandFlags.None)

public void MakeMaster(ReplicationChangeOptions options, TextWriter log = null)
{
using (var proxy = LogProxy.TryCreate(log))
using (var proxy = LogProxy.TryCreate(log, multiplexer.RawConfig))
{
// Do you believe in magic?
multiplexer.MakePrimaryAsync(server, options, proxy).Wait(60000);
Expand All @@ -339,7 +339,7 @@ public void MakeMaster(ReplicationChangeOptions options, TextWriter log = null)

public async Task MakePrimaryAsync(ReplicationChangeOptions options, TextWriter log = null)
{
using (var proxy = LogProxy.TryCreate(log))
using (var proxy = LogProxy.TryCreate(log, multiplexer.RawConfig))
{
await multiplexer.MakePrimaryAsync(server, options, proxy);
}
Expand Down