feat(SocketClientOptions): add EnableLog parameter #6371
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideIntroduces an EnableLog flag in SocketClientOptions to control detailed TCP send/receive logging, incorporates conditional logging in the client implementation, and updates tests to enable logging. Class diagram for updated SocketClientOptions and TcpSocketClientBaseclassDiagram
class SocketClientOptions {
IPEndPoint LocalEndPoint
bool EnableLog
}
class TcpSocketClientBase {
+ValueTask<bool> SendAsync(ReadOnlyMemory<byte> data, CancellationToken token)
+ValueTask<int> ReceiveCoreAsync(ISocketClientProvider client, Memory<byte> buffer, CancellationToken token)
-Log(LogLevel, Exception, string)
-_localEndPoint
-_remoteEndPoint
-options: SocketClientOptions
}
TcpSocketClientBase --> SocketClientOptions : uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Services/TcpSocket/TcpSocketClientBase.cs:169` </location>
<code_context>
Log(LogLevel.Error, ex, $"TCP Socket send failed from {_localEndPoint} to {_remoteEndPoint}");
}
+
+ if (options.EnableLog)
+ {
+ Log(LogLevel.Information, null, $"Sending data from {_localEndPoint} to {_remoteEndPoint}, Data Length: {data.Length} Data Content: {BitConverter.ToString(data.ToArray())} Result: {ret}");
+ }
return ret;
</code_context>
<issue_to_address>
Logging raw data content may expose sensitive information.
Redact or limit logged data content, or make logging configurable to avoid exposing sensitive information.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Services/TcpSocket/TcpSocketClientBase.cs:171` </location>
<code_context>
+
+ if (options.EnableLog)
+ {
+ Log(LogLevel.Information, null, $"Sending data from {_localEndPoint} to {_remoteEndPoint}, Data Length: {data.Length} Data Content: {BitConverter.ToString(data.ToArray())} Result: {ret}");
+ }
return ret;
</code_context>
<issue_to_address>
Converting ReadOnlyMemory<byte> to array may impact performance.
ToArray() allocates and copies data, which can be costly for large or frequent logs. Consider limiting logged data size or using a more efficient method.
Suggested implementation:
```csharp
if (options.EnableLog)
{
const int MaxLogBytes = 64;
int logLength = Math.Min(data.Length, MaxLogBytes);
string dataContent;
if (logLength > 0)
{
var span = data.Span.Slice(0, logLength);
dataContent = BitConverter.ToString(span.ToArray());
if (data.Length > MaxLogBytes)
{
dataContent += "...(truncated)";
}
}
else
{
dataContent = string.Empty;
}
Log(LogLevel.Information, null, $"Sending data from {_localEndPoint} to {_remoteEndPoint}, Data Length: {data.Length} Data Content: {dataContent} Result: {ret}");
}
return ret;
```
```csharp
if (options.EnableLog)
{
const int MaxLogBytes = 64;
int logLength = Math.Min(len, MaxLogBytes);
string dataContent;
if (logLength > 0)
{
var span = buffer.Span.Slice(0, logLength);
dataContent = BitConverter.ToString(span.ToArray());
if (len > MaxLogBytes)
{
dataContent += "...(truncated)";
}
}
else
{
dataContent = string.Empty;
}
Log(LogLevel.Information, null, $"Receiving data from {_localEndPoint} to {_remoteEndPoint}, Data Length: {len} Data Content: {dataContent}");
}
```
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Services/TcpSocket/TcpSocketClientBase.cs:269` </location>
<code_context>
+
+ if (options.EnableLog)
+ {
+ Log(LogLevel.Information, null, $"Receiving data from {_localEndPoint} to {_remoteEndPoint}, Data Length: {len} Data Content: {BitConverter.ToString(buffer.ToArray())}");
+ }
return len;
</code_context>
<issue_to_address>
Repeated ToArray() calls on buffer may be inefficient.
For large or frequently used buffers, this conversion can impact performance. Consider logging a subset of the buffer or a more efficient format.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (options.EnableLog)
{
Log(LogLevel.Information, null, $"Receiving data from {_localEndPoint} to {_remoteEndPoint}, Data Length: {len} Data Content: {BitConverter.ToString(buffer.ToArray())}");
}
return len;
=======
if (options.EnableLog)
{
const int maxLogBytes = 32;
var logBytes = buffer.Count > maxLogBytes ? buffer.Slice(0, maxLogBytes).ToArray() : buffer.ToArray();
var dataContent = BitConverter.ToString(logBytes);
if (buffer.Count > maxLogBytes)
{
dataContent += $"... (truncated, total {buffer.Count} bytes)";
}
Log(LogLevel.Information, null, $"Receiving data from {_localEndPoint} to {_remoteEndPoint}, Data Length: {len} Data Content: {dataContent}");
}
return len;
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:646` </location>
<code_context>
var provider = sc.BuildServiceProvider();
var factory = provider.GetRequiredService<ITcpSocketFactory>();
- var client = factory.GetOrCreate("test", op => op.LocalEndPoint = Utility.ConvertToIpEndPoint("localhost", 0));
+ var client = factory.GetOrCreate("test", op =>
+ {
+ op.LocalEndPoint = Utility.ConvertToIpEndPoint("localhost", 0);
+ op.EnableLog = true;
+ });
return client;
</code_context>
<issue_to_address>
No test coverage for EnableLog=false scenario.
Please add a test to cover the default EnableLog=false case and confirm that logging is not triggered.
Suggested implementation:
```csharp
var provider = sc.BuildServiceProvider();
var factory = provider.GetRequiredService<ITcpSocketFactory>();
var client = factory.GetOrCreate("test", op =>
{
op.LocalEndPoint = Utility.ConvertToIpEndPoint("localhost", 0);
op.EnableLog = true;
});
return client;
}
[Fact]
public void GetOrCreate_DefaultEnableLogFalse_DoesNotTriggerLogging()
{
// Arrange
var sc = new ServiceCollection();
// Register ITcpSocketFactory and any required dependencies, including a mock logger if needed
var mockLogger = new Mock<ILogger>();
sc.AddSingleton<ILogger>(mockLogger.Object);
sc.AddSingleton<ITcpSocketFactory, TcpSocketFactory>();
var provider = sc.BuildServiceProvider();
var factory = provider.GetRequiredService<ITcpSocketFactory>();
// Act
var client = factory.GetOrCreate("test-default", op =>
{
op.LocalEndPoint = Utility.ConvertToIpEndPoint("localhost", 0);
// Do not set EnableLog, should default to false
});
// Assert
// Check that logging was not triggered
mockLogger.Verify(
logger => logger.Log(
It.IsAny<LogLevel>(),
It.IsAny<EventId>(),
It.IsAny<It.IsAnyType>(),
It.IsAny<Exception>(),
(Func<It.IsAnyType, Exception, string>)It.IsAny<object>()),
Times.Never
);
}
```
- If your project uses a different logging abstraction or does not use dependency injection for the logger, you may need to adjust the mocking and registration code accordingly.
- If `TcpSocketFactory` does not use `ILogger`, or logging is handled differently, adapt the assertion to match your actual logging mechanism.
- Ensure you have the necessary using directives at the top of the file:
```csharp
using Xunit;
using Moq;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
```
- If you do not have Moq installed, add it to your test project.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+169
to
+171
| if (options.EnableLog) | ||
| { | ||
| Log(LogLevel.Information, null, $"Sending data from {_localEndPoint} to {_remoteEndPoint}, Data Length: {data.Length} Data Content: {BitConverter.ToString(data.ToArray())} Result: {ret}"); |
There was a problem hiding this comment.
🚨 issue (security): Logging raw data content may expose sensitive information.
Redact or limit logged data content, or make logging configurable to avoid exposing sensitive information.
|
|
||
| if (options.EnableLog) | ||
| { | ||
| Log(LogLevel.Information, null, $"Sending data from {_localEndPoint} to {_remoteEndPoint}, Data Length: {data.Length} Data Content: {BitConverter.ToString(data.ToArray())} Result: {ret}"); |
There was a problem hiding this comment.
suggestion (performance): Converting ReadOnlyMemory to array may impact performance.
ToArray() allocates and copies data, which can be costly for large or frequent logs. Consider limiting logged data size or using a more efficient method.
Suggested implementation:
if (options.EnableLog)
{
const int MaxLogBytes = 64;
int logLength = Math.Min(data.Length, MaxLogBytes);
string dataContent;
if (logLength > 0)
{
var span = data.Span.Slice(0, logLength);
dataContent = BitConverter.ToString(span.ToArray());
if (data.Length > MaxLogBytes)
{
dataContent += "...(truncated)";
}
}
else
{
dataContent = string.Empty;
}
Log(LogLevel.Information, null, $"Sending data from {_localEndPoint} to {_remoteEndPoint}, Data Length: {data.Length} Data Content: {dataContent} Result: {ret}");
}
return ret; if (options.EnableLog)
{
const int MaxLogBytes = 64;
int logLength = Math.Min(len, MaxLogBytes);
string dataContent;
if (logLength > 0)
{
var span = buffer.Span.Slice(0, logLength);
dataContent = BitConverter.ToString(span.ToArray());
if (len > MaxLogBytes)
{
dataContent += "...(truncated)";
}
}
else
{
dataContent = string.Empty;
}
Log(LogLevel.Information, null, $"Receiving data from {_localEndPoint} to {_remoteEndPoint}, Data Length: {len} Data Content: {dataContent}");
}
Comment on lines
+267
to
271
| if (options.EnableLog) | ||
| { | ||
| Log(LogLevel.Information, null, $"Receiving data from {_localEndPoint} to {_remoteEndPoint}, Data Length: {len} Data Content: {BitConverter.ToString(buffer.ToArray())}"); | ||
| } | ||
| return len; |
There was a problem hiding this comment.
suggestion (performance): Repeated ToArray() calls on buffer may be inefficient.
For large or frequently used buffers, this conversion can impact performance. Consider logging a subset of the buffer or a more efficient format.
Suggested change
| if (options.EnableLog) | |
| { | |
| Log(LogLevel.Information, null, $"Receiving data from {_localEndPoint} to {_remoteEndPoint}, Data Length: {len} Data Content: {BitConverter.ToString(buffer.ToArray())}"); | |
| } | |
| return len; | |
| if (options.EnableLog) | |
| { | |
| const int maxLogBytes = 32; | |
| var logBytes = buffer.Count > maxLogBytes ? buffer.Slice(0, maxLogBytes).ToArray() : buffer.ToArray(); | |
| var dataContent = BitConverter.ToString(logBytes); | |
| if (buffer.Count > maxLogBytes) | |
| { | |
| dataContent += $"... (truncated, total {buffer.Count} bytes)"; | |
| } | |
| Log(LogLevel.Information, null, $"Receiving data from {_localEndPoint} to {_remoteEndPoint}, Data Length: {len} Data Content: {dataContent}"); | |
| } | |
| return len; |
Comment on lines
+646
to
+649
| var client = factory.GetOrCreate("test", op => | ||
| { | ||
| op.LocalEndPoint = Utility.ConvertToIpEndPoint("localhost", 0); | ||
| op.EnableLog = true; |
There was a problem hiding this comment.
suggestion (testing): No test coverage for EnableLog=false scenario.
Please add a test to cover the default EnableLog=false case and confirm that logging is not triggered.
Suggested implementation:
var provider = sc.BuildServiceProvider();
var factory = provider.GetRequiredService<ITcpSocketFactory>();
var client = factory.GetOrCreate("test", op =>
{
op.LocalEndPoint = Utility.ConvertToIpEndPoint("localhost", 0);
op.EnableLog = true;
});
return client;
}
[Fact]
public void GetOrCreate_DefaultEnableLogFalse_DoesNotTriggerLogging()
{
// Arrange
var sc = new ServiceCollection();
// Register ITcpSocketFactory and any required dependencies, including a mock logger if needed
var mockLogger = new Mock<ILogger>();
sc.AddSingleton<ILogger>(mockLogger.Object);
sc.AddSingleton<ITcpSocketFactory, TcpSocketFactory>();
var provider = sc.BuildServiceProvider();
var factory = provider.GetRequiredService<ITcpSocketFactory>();
// Act
var client = factory.GetOrCreate("test-default", op =>
{
op.LocalEndPoint = Utility.ConvertToIpEndPoint("localhost", 0);
// Do not set EnableLog, should default to false
});
// Assert
// Check that logging was not triggered
mockLogger.Verify(
logger => logger.Log(
It.IsAny<LogLevel>(),
It.IsAny<EventId>(),
It.IsAny<It.IsAnyType>(),
It.IsAny<Exception>(),
(Func<It.IsAnyType, Exception, string>)It.IsAny<object>()),
Times.Never
);
}
- If your project uses a different logging abstraction or does not use dependency injection for the logger, you may need to adjust the mocking and registration code accordingly.
- If
TcpSocketFactorydoes not useILogger, or logging is handled differently, adapt the assertion to match your actual logging mechanism. - Ensure you have the necessary using directives at the top of the file:
using Xunit; using Moq; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging;
- If you do not have Moq installed, add it to your test project.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6371 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 718 718
Lines 31615 31624 +9
Branches 4461 4463 +2
=========================================
+ Hits 31615 31624 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #6367
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add a new EnableLog option to SocketClientOptions to allow conditional detailed logging of TCP socket send and receive operations and update the client and tests to use this flag.
New Features:
Tests: