Skip to content

Adjust overloads and parameter order to resolve potential ambiguous method call errors #59

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 15, 2025
Merged

Adjust overloads and parameter order to resolve potential ambiguous method call errors #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a62ee8
Remove overloads that seem unnecessary and move new parameters to aft…
tbdrake Apr 15, 2025
be5b72c
Fix nullability warnings
tbdrake Apr 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions PingPonger/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,17 @@ public static int Main(string[] args)
try
{
var options = new Options();
if (Parser.Default.ParseArguments<Options>(args).Tag == ParserResultType.NotParsed)
var parseResult = Parser.Default.ParseArguments<Options>(args)
.WithNotParsed(errs =>
{
Console.WriteLine("Failed parsing command line arguments:");
errs.ToList().ForEach(e => Console.WriteLine(@"error: {0}", e));
args.ToList().ForEach(a => Console.WriteLine(@"arg: {0}", a));
})
.WithParsed(opts => options = opts);

if (parseResult.Tag == ParserResultType.NotParsed)
{
Console.WriteLine(@"Failed parsing command line arguments");
args.ToList().ForEach(a => Console.WriteLine(@"arg: {0}", a));
return 1;
}

Expand All @@ -64,11 +71,19 @@ public static int Main(string[] args)
{
if (options.UDP)
{
logConfig.WriteTo.UDPSink(options.Url, options.Port, new LogstashJsonFormatter());
logConfig.WriteTo.UDPSink(options.Url, options.Port);
}
if (options.TCP)
{
logConfig.WriteTo.TCPSink(options.Url, options.Port, null, null, new LogstashJsonFormatter());
if (options.Port != 0)
{
logConfig.WriteTo.TCPSink(options.Url, options.Port);
}
else
{
// URL option value may already have a port in it, e.g. "tcp://localhost:8888"
logConfig.WriteTo.TCPSink(options.Url);
}
}
}
else if (options.IP?.Length > 0)
Expand All @@ -81,11 +96,11 @@ public static int Main(string[] args)

if (options.UDP)
{
logConfig.WriteTo.UDPSink(ipAddress, options.Port, new LogstashJsonFormatter());
logConfig.WriteTo.UDPSink(ipAddress, options.Port);
}
if (options.TCP)
{
logConfig.WriteTo.TCPSink(ipAddress, options.Port,null,null, new LogstashJsonFormatter());
logConfig.WriteTo.TCPSink(ipAddress, options.Port);
}
}
else
Expand Down
10 changes: 5 additions & 5 deletions Serilog.Sinks.Network.Test/JsonFormatter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,15 @@ public class JsonFormatter
{
private static LoggerAndSocket ConfigureTestLogger(
ITextFormatter? formatter = null,
ILogEventEnricher[] enrichers = null
ILogEventEnricher[]? enrichers = null
)
{
var socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
socket.Bind(new IPEndPoint(IPAddress.Loopback, 0));
socket.Listen();

var loggerConfiguration = new LoggerConfiguration()
.WriteTo.TCPSink(IPAddress.Loopback, ((IPEndPoint)socket.LocalEndPoint!).Port, null, null, formatter);
.WriteTo.TCPSink(IPAddress.Loopback, ((IPEndPoint)socket.LocalEndPoint!).Port, formatter);
if (enrichers != null)
{
loggerConfiguration.Enrich.With(enrichers);
Expand Down Expand Up @@ -79,7 +79,7 @@ public async Task IncludesCurrentActivityTraceAndSpanIds()

var receivedData = await ServerPoller.PollForReceivedData(fixture.Socket);

receivedData.Should().Contain($"\"traceId\":\"{activity.TraceId}\"");
receivedData.Should().Contain($"\"traceId\":\"{activity!.TraceId}\"");
receivedData.Should().Contain($"\"spanId\":\"{activity.SpanId}\"");
}

Expand Down Expand Up @@ -119,7 +119,7 @@ public async Task WritesTraceAndSpanIdsBeforeDuplicatePropertiesFromEnrichers()

var receivedData = await ServerPoller.PollForReceivedData(fixture.Socket);

var indexOfTraceId = receivedData.IndexOf($"\"traceId\":\"{activity.TraceId}\"");
var indexOfTraceId = receivedData.IndexOf($"\"traceId\":\"{activity!.TraceId}\"");
var indexOfTraceIdFromEnricher = receivedData.IndexOf("\"traceId\":\"traceId-from-enricher\"");
indexOfTraceId.Should().BePositive().And.BeLessThan(indexOfTraceIdFromEnricher);

Expand All @@ -139,4 +139,4 @@ private static ActivityListener CreateAndAddActivityListener(string sourceName)
return activityListener;
}
}
}
}
2 changes: 1 addition & 1 deletion Serilog.Sinks.Network.Test/WhenLoggingViaTcp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ private static LoggerAndSocket ConfigureTestLogger(ITextFormatter? formatter = n
socket.Listen();

var logger = new LoggerConfiguration()
.WriteTo.TCPSink(IPAddress.Loopback, ((IPEndPoint)socket.LocalEndPoint!).Port, null, null, formatter)
.WriteTo.TCPSink(IPAddress.Loopback, ((IPEndPoint)socket.LocalEndPoint!).Port, formatter)
.CreateLogger();

return new LoggerAndSocket { Logger = logger, Socket = socket };
Expand Down
39 changes: 11 additions & 28 deletions Serilog.Sinks.Network/NetworkLoggerConfigurationExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,42 +41,33 @@ public static LoggerConfiguration TCPSink(
this LoggerSinkConfiguration loggerConfiguration,
IPAddress ipAddress,
int port,
int? writeTimeoutMs = null,
int? disposeTimeoutMs = null,
ITextFormatter? textFormatter = null,
LogEventLevel restrictedToMinimumLevel = LevelAlias.Minimum)
{
return TCPSink(loggerConfiguration, $"tcp://{ipAddress}:{port}", writeTimeoutMs, disposeTimeoutMs, textFormatter, restrictedToMinimumLevel);
}

public static LoggerConfiguration TCPSink(
this LoggerSinkConfiguration loggerConfiguration,
IPAddress ipAddress,
int port,
LogEventLevel restrictedToMinimumLevel = LevelAlias.Minimum)
Comment on lines -52 to -56
Copy link
Contributor Author

@tbdrake tbdrake Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This overload

        public static LoggerConfiguration TCPSink(
            this LoggerSinkConfiguration loggerConfiguration,
            IPAddress ipAddress,
            int port,
            LogEventLevel restrictedToMinimumLevel = LevelAlias.Minimum)

was added in #51 included in 3.0.0-alpha.1 but I think can be safely removed. It can cause ambiguous call errors with code like TCPSink(IPAddress.Parse("127.0.0.1"), 1234) because the overload above could also match.

I think this is OK to remove because the overload above can cover the same functionality/parameters with calls like this:
TCPSink(IPAddress.Parse("127.0.0.1"), 1234, restrictedToMinimumLevel: logLevel)

LogEventLevel restrictedToMinimumLevel = LevelAlias.Minimum,
int? writeTimeoutMs = null,
int? disposeTimeoutMs = null)
{
return TCPSink(loggerConfiguration, ipAddress, port,null, null, null, restrictedToMinimumLevel);
return TCPSink(loggerConfiguration, $"tcp://{ipAddress}:{port}", textFormatter, restrictedToMinimumLevel, writeTimeoutMs, disposeTimeoutMs);
}

public static LoggerConfiguration TCPSink(
this LoggerSinkConfiguration loggerConfiguration,
string host,
int port,
int? writeTimeoutMs = null,
int? disposeTimeoutMs = null,
ITextFormatter? textFormatter = null,
LogEventLevel restrictedToMinimumLevel = LevelAlias.Minimum)
LogEventLevel restrictedToMinimumLevel = LevelAlias.Minimum,
int? writeTimeoutMs = null,
int? disposeTimeoutMs = null)
{
return TCPSink(loggerConfiguration, $"{host}:{port}", writeTimeoutMs, disposeTimeoutMs, textFormatter, restrictedToMinimumLevel);
return TCPSink(loggerConfiguration, $"{host}:{port}", textFormatter, restrictedToMinimumLevel, writeTimeoutMs, disposeTimeoutMs);
}

public static LoggerConfiguration TCPSink(
this LoggerSinkConfiguration loggerConfiguration,
string uri,
int? writeTimeoutMs = null,
int? disposeTimeoutMs = null,
ITextFormatter? textFormatter = null,
LogEventLevel restrictedToMinimumLevel = LevelAlias.Minimum)
LogEventLevel restrictedToMinimumLevel = LevelAlias.Minimum,
int? writeTimeoutMs = null,
int? disposeTimeoutMs = null)
{
var socketWriter = new TcpSocketWriter(
BuildUri(uri),
Expand All @@ -86,14 +77,6 @@ public static LoggerConfiguration TCPSink(
return loggerConfiguration.Sink(sink, restrictedToMinimumLevel);
}

public static LoggerConfiguration TCPSink(
this LoggerSinkConfiguration loggerConfiguration,
string uri,
LogEventLevel restrictedToMinimumLevel = LevelAlias.Minimum)
{
return TCPSink(loggerConfiguration, uri, null, null, null, restrictedToMinimumLevel);
}

Comment on lines -89 to -96
Copy link
Contributor Author

@tbdrake tbdrake Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This overload was added in #51 and included in 3.0.0-alpha.1. Code like TCPSink("tcp://localhost:1234") would get an ambiguous call error because both this overload and the one above could match.

I think this one can be safely removed since it is only in an alpha release and the same functionality can be achieved using the overload above with args like TCPSink("tcp://localhost:1234") or TCPSink("tcp://localhost:1234", restrictedToMinimumLevel: logLevel)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I added this because of #51 (comment).
But as we are going for a major, removing this is fine I guess?

Copy link
Contributor Author

@tbdrake tbdrake Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I see, but I was thinking that rather than add that overload, we could address the concern in that comment by putting the new optional parameters writeTimeoutMs and disposeTimeoutMs at the end of the existing methods, e.g.

        public static LoggerConfiguration TCPSink(
            this LoggerSinkConfiguration loggerConfiguration,
            string uri,
            ITextFormatter? textFormatter = null,
            LogEventLevel restrictedToMinimumLevel = LevelAlias.Minimum,
            int? writeTimeoutMs = null,
            int? disposeTimeoutMs = null)

That way, existing code that was calling that existing method like cfg.TCPSink("tcp://localhost:1234", new LogstashJsonFormatter(), LogEventLevel.Debug) would still work and be compatible with the new signature (it would use the default args writeTimeoutMs = null and disposeTimeoutMs = null).

Does that make sense and seem OK to you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup, works for me.

private static IPAddress ResolveAddress(string uri)
{
// Check if it is IP address
Expand Down