P3: Add LoggingHandler with structured logging and sensitive data redaction by jhosm · Pull Request #1 · jhosm/CSharp-ACDC

jhosm · 2026-02-09T00:21:57Z

Summary

Adds AcdcLoggingOptions configuration (now a record with FrozenSet defaults) with configurable thresholds and 16 default sensitive field names
Adds SensitiveDataRedactor for redacting headers, query parameters, and JSON body fields (case-insensitive matching via System.Text.Json)
Adds LoggingHandler (DelegatingHandler) with structured request/response logging, slow request and large payload warnings, AsyncLocal reentrancy prevention, and per-request SkipLogging opt-out
Adds comprehensive unit tests for both SensitiveDataRedactor (18 tests) and LoggingHandler (18 tests)

Review feedback addressed

Converted AcdcLoggingOptions to record with FrozenSet for immutable defaults
Protected HTTP pipeline from logging/redaction failures (try/catch with graceful degradation)
Defensive copy of SensitiveFields in SensitiveDataRedactor with forced OrdinalIgnoreCase
Added ArgumentNullException.ThrowIfNull() null guards in constructors
OperationCanceledException now logged at Information level (not Error)
Non-JSON bodies return placeholder [non-JSON body, redaction skipped] instead of leaking raw content
Fixed repeated query parameter handling via GetValues()
Added null-check for response.Content?.Headers.ContentLength
Renamed IsLogging → _isLogging (naming convention)
Fixed .GetAwaiter().GetResult() → proper await in reentrancy test
Removed unused usings, added using var for disposables
Added tests: AsyncLocal error recovery, concurrent request isolation, cancellation logging
Improved reentrancy test with InnerLogger assertion
Enhanced response header redaction test with sensitive header assertion

Test plan

🤖 Generated with Claude Code

…ta redaction - Add AcdcLoggingOptions with configurable slow request threshold (3s default), large payload threshold (1 MiB), and 16 default sensitive field names - Add SensitiveDataRedactor for header, query parameter, and JSON body redaction with case-insensitive field matching via System.Text.Json - Add LoggingHandler DelegatingHandler with request/response logging at Info level, slow request warnings, large payload warnings, error logging with redacted details, AsyncLocal<bool> reentrancy prevention, and per-request SkipLogging opt-out - Add comprehensive unit tests for both SensitiveDataRedactor and LoggingHandler covering all redaction scenarios, threshold boundaries, reentrancy, and error paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Adds structured HTTP request/response logging to the CSharpAcdc HTTP pipeline, including configurable thresholds and a reusable sensitive-data redactor to prevent leaking secrets in logs.

Changes:

Introduces AcdcLoggingOptions for slow-request / large-payload thresholds and a default sensitive field set.
Adds SensitiveDataRedactor to redact sensitive headers, query parameters, and JSON body fields.
Adds LoggingHandler (DelegatingHandler) plus unit tests for both the handler and the redactor.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
src/CSharpAcdc/Configuration/AcdcLoggingOptions.cs	Adds configuration for logging thresholds and default sensitive fields.
src/CSharpAcdc/Logging/SensitiveDataRedactor.cs	Implements header/query/JSON redaction logic.
src/CSharpAcdc/Handlers/LoggingHandler.cs	Adds request/response structured logging and warning logic.
tests/CSharpAcdc.Tests/Logging/SensitiveDataRedactorTests.cs	Adds unit tests covering redaction behaviors.
tests/CSharpAcdc.Tests/Handlers/LoggingHandlerTests.cs	Adds unit tests for logging, redaction, warnings, and reentrancy behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-09T00:27:23Z

tests/CSharpAcdc.Tests/Handlers/LoggingHandlerTests.cs

+using FluentAssertions;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Options;
+using NSubstitute;


Unused using NSubstitute; will produce CS8019 (unnecessary using directive) and can fail builds when warnings are treated as errors. Remove this using if it’s not needed.

Suggested change

using NSubstitute;

Copilot · 2026-02-09T00:27:24Z

tests/CSharpAcdc.Tests/Handlers/LoggingHandlerTests.cs

+
+        // Should see request + response logs for outer, but not for inner
+        infoLogs.Should().AllSatisfy(e => e.Message.Should().Contain("/outer"));
+        infoLogs.Should().NotContain(e => e.Message.Contains("/inner"));


This reentrancy test doesn’t currently validate whether the nested request was actually skipped: the inner request logs would go to innerLogger, but the assertions only inspect the outer _logger, so it would pass even if the reentrancy guard failed. Expose innerLogger to the test (e.g., via a field on NestedRequestHandler) and assert it has no entries (or explicitly assert expected counts).

Suggested change

infoLogs.Should().NotContain(e => e.Message.Contains("/inner"));

infoLogs.Should().NotContain(e => e.Message.Contains("/inner"));

// Additionally ensure the nested handler's own logger did not record any entries

nestedHandler.InnerLogger.Entries.Should().BeEmpty();

Copilot · 2026-02-09T00:27:24Z

tests/CSharpAcdc.Tests/Handlers/LoggingHandlerTests.cs

+            // This should be skipped by reentrancy guard
+            _ = innerClient.GetAsync("/inner", cancellationToken).GetAwaiter().GetResult();
+
+            return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK));


Calling GetAsync(...).GetAwaiter().GetResult() from within a handler can deadlock under certain synchronization contexts and can also block threadpool threads unnecessarily. Make this override async and await the nested call instead.

Copilot · 2026-02-09T00:27:24Z

src/CSharpAcdc/Logging/SensitiveDataRedactor.cs

+            redactedParts.Add(IsSensitive(name)
+                ? $"{Uri.EscapeDataString(name)}={Redacted}"
+                : $"{Uri.EscapeDataString(name)}={Uri.EscapeDataString(query[name] ?? string.Empty)}");


HttpUtility.ParseQueryString collapses repeated query keys (e.g., a=1&a=2) into a single comma-separated string via query[name], and this implementation will log it as a single value (and after escaping, it becomes 1%2C2). If preserving query shape matters for debugging, iterate query.GetValues(name) and emit one name=value pair per value while still redacting sensitive keys.

Suggested change

redactedParts.Add(IsSensitive(name)

? $"{Uri.EscapeDataString(name)}={Redacted}"

: $"{Uri.EscapeDataString(name)}={Uri.EscapeDataString(query[name] ?? string.Empty)}");

var values = query.GetValues(name);

var escapedName = Uri.EscapeDataString(name);

if (IsSensitive(name))

{

if (values is null || values.Length == 0)

{

redactedParts.Add($"{escapedName}={Redacted}");

}

else

{

foreach (var _ in values)

{

redactedParts.Add($"{escapedName}={Redacted}");

}

}

}

else

{

if (values is null || values.Length == 0)

{

redactedParts.Add($"{escapedName}=");

}

else

{

foreach (var value in values)

{

redactedParts.Add($"{escapedName}={Uri.EscapeDataString(value ?? string.Empty)}");

}

}

}

Copilot · 2026-02-09T00:27:24Z

src/CSharpAcdc/Handlers/LoggingHandler.cs

+{
+    private static readonly AsyncLocal<bool> IsLogging = new();
+


Private field naming deviates from the repository’s naming rules (private fields should be prefixed with '_' and camelCase). Rename this static field (e.g., to '_isLogging') to satisfy the analyzer rules in .editorconfig and avoid style warnings becoming build breaks under TreatWarningsAsErrors.

Copilot · 2026-02-09T00:27:25Z

tests/CSharpAcdc.Tests/Handlers/LoggingHandlerTests.cs

+        mockHandler.When("*").Respond(HttpStatusCode.OK);
+
+        using var client = CreateClient(mockHandler);
+        var request = new HttpRequestMessage(HttpMethod.Get, "/test");


Disposable 'HttpRequestMessage' is created but not disposed.

Suggested change

var request = new HttpRequestMessage(HttpMethod.Get, "/test");

using var request = new HttpRequestMessage(HttpMethod.Get, "/test");

Copilot · 2026-02-09T00:27:25Z

tests/CSharpAcdc.Tests/Handlers/LoggingHandlerTests.cs

+        mockHandler.When("*").Respond(HttpStatusCode.OK);
+
+        using var client = CreateClient(mockHandler, options);
+        var request = new HttpRequestMessage(HttpMethod.Get, "/test");


Disposable 'HttpRequestMessage' is created but not disposed.

Suggested change

var request = new HttpRequestMessage(HttpMethod.Get, "/test");

using var request = new HttpRequestMessage(HttpMethod.Get, "/test");

Copilot · 2026-02-09T00:27:26Z

tests/CSharpAcdc.Tests/Handlers/LoggingHandlerTests.cs

+        mockHandler.When("*").Respond(HttpStatusCode.OK);
+
+        using var client = CreateClient(mockHandler, options);
+        var content = new StringContent(new string('x', 200));


Disposable 'StringContent' is created but not disposed.

Suggested change

var content = new StringContent(new string('x', 200));

using var content = new StringContent(new string('x', 200));

Copilot · 2026-02-09T00:27:26Z

tests/CSharpAcdc.Tests/Handlers/LoggingHandlerTests.cs

+        mockHandler.When("*").Respond(HttpStatusCode.OK);
+
+        using var client = CreateClient(mockHandler);
+        var request = new HttpRequestMessage(HttpMethod.Get, "/silent");


Disposable 'HttpRequestMessage' is created but not disposed.

Suggested change

var request = new HttpRequestMessage(HttpMethod.Get, "/silent");

using var request = new HttpRequestMessage(HttpMethod.Get, "/silent");

Copilot · 2026-02-09T00:27:26Z

tests/CSharpAcdc.Tests/Handlers/LoggingHandlerTests.cs

+            // This should be skipped by reentrancy guard
+            _ = innerClient.GetAsync("/inner", cancellationToken).GetAwaiter().GetResult();
+
+            return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK));


Disposable 'HttpResponseMessage' is created but not disposed.

jhosm · 2026-02-09T07:26:40Z

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

jhosm · 2026-02-09T07:39:41Z

Comprehensive PR Review -- P3: LoggingHandler

Five specialized review agents independently analyzed this PR. Below is the deduplicated, aggregated summary.

Critical Issues

1. AcdcLoggingOptions is a mutable class -- violates CLAUDE.md convention (3 agents converged)

CLAUDE.md states: "Use record types for DTOs and immutable config." AcdcLoggingOptions is configuration with public { get; set; } properties. All three properties can be mutated after construction, including replacing SensitiveFields with a case-sensitive set (breaking redaction). In a concurrent server context with pooled DelegatingHandler instances, this is both a convention violation and a thread-safety hazard.

CSharp-ACDC/src/CSharpAcdc/Configuration/AcdcLoggingOptions.cs

Lines 3 to 10 in f837f5f

    
           public class AcdcLoggingOptions 
        
           { 
        
               public TimeSpan SlowRequestThreshold { get; set; } = TimeSpan.FromSeconds(3); 
        
               public long LargePayloadThreshold { get; set; } = 1_048_576; // 1 MiB 
        
               public IReadOnlySet<string> SensitiveFields { get; set; } = new HashSet<string>(StringComparer.OrdinalIgnoreCase) 
        
               {

Type design scores: Encapsulation 2/10, Invariant Expression 2/10, Enforcement 1/10.

Fix: Convert to record with init setters, or at minimum add IValidateOptions<T> validation and freeze SensitiveFields internally.

2. Unprotected redaction/logging can crash the HTTP pipeline (2 agents)

_redactor.RedactUrl(), _redactor.RedactHeaders(), and _logger.Log*() are all called without try/catch. If redaction throws (e.g., UriFormatException on malformed URI) or the logger throws (sink serialization failure), the user's HTTP request fails -- not because of a network problem, but because the logging infrastructure crashed. A logging handler should never take down the pipeline.

CSharp-ACDC/src/CSharpAcdc/Handlers/LoggingHandler.cs

Lines 47 to 54 in f837f5f

    
           var redactedUrl = _redactor.RedactUrl(request.RequestUri); 
        
           var redactedHeaders = _redactor.RedactHeaders(request.Headers); 
        
           _logger.LogInformation( 
        
               "HTTP {Method} {Url} Headers: {Headers}", 
        
               request.Method, 
        
               redactedUrl, 
        
               redactedHeaders);

Fix: Wrap pre-request and post-response logging blocks in try/catch that degrades gracefully.

3. Invalid JSON silently returned unredacted -- sensitive data leak risk (2 agents)

When RedactJsonBody fails to parse JSON, it silently returns the original body unredacted. A body with a minor JSON syntax error containing password, token, api_key fields will be logged in full with all secrets visible. No warning is emitted.

CSharp-ACDC/src/CSharpAcdc/Logging/SensitiveDataRedactor.cs

Lines 70 to 74 in f837f5f

    
           } 
        
           catch (JsonException) 
        
           { 
        
               return body; // Not valid JSON — return as-is 
        
           }

Fix: Either return [non-JSON body, redaction skipped] placeholder, or add ILogger to SensitiveDataRedactor and log a warning when redaction is skipped.

Important Issues

4. No defensive copy of SensitiveFields set (3 agents)

SensitiveDataRedactor stores a reference to options.SensitiveFields without copying. External code can cast IReadOnlySet<string> back to HashSet<string> and mutate it, or the AcdcLoggingOptions setter can replace it entirely. Under concurrency, this is a data race.

CSharp-ACDC/src/CSharpAcdc/Logging/SensitiveDataRedactor.cs

Lines 9 to 12 in f837f5f

    
           private const string Redacted = "[REDACTED]"; 
        
           private readonly IReadOnlySet<string> _sensitiveFields; 
        
           public SensitiveDataRedactor(AcdcLoggingOptions options)

Fix: Defensively copy in constructor: _sensitiveFields = new HashSet<string>(options.SensitiveFields, StringComparer.OrdinalIgnoreCase).

5. Missing constructor null guards (2 agents)

Neither LoggingHandler nor SensitiveDataRedactor validate constructor parameters. new LoggingHandler(null!, ...) defers the NullReferenceException to the first request instead of failing fast at construction.

Fix: Add ArgumentNullException.ThrowIfNull() in constructors.

6. OperationCanceledException logged at Error level (2 agents)

The catch block in SendWithLoggingAsync catches all Exception types and logs at LogError. Deliberate cancellation is expected behavior, not an error -- it pollutes error monitoring with false positives.

CSharp-ACDC/src/CSharpAcdc/Handlers/LoggingHandler.cs

Lines 64 to 74 in f837f5f

    
           catch (Exception ex) 
        
           { 
        
               stopwatch.Stop(); 
        
               _logger.LogError( 
        
                   ex, 
        
                   "HTTP {Method} {Url} failed after {ElapsedMs}ms", 
        
                   request.Method, 
        
                   redactedUrl, 
        
                   stopwatch.ElapsedMilliseconds); 
        
               throw; 
        
           }

Fix: Add a separate catch (OperationCanceledException) that logs at Information/Debug level before the general catch (Exception).

7. .GetAwaiter().GetResult() blocking call in test (2 agents)

NestedRequestHandler.SendAsync uses .GetAwaiter().GetResult() which violates the "Async all the way" CLAUDE.md rule and risks deadlocks. While the synchronous call is deliberate for AsyncLocal testing, it needs a comment explaining why await is not used.

CSharp-ACDC/tests/CSharpAcdc.Tests/Handlers/LoggingHandlerTests.cs

Lines 367 to 370 in f837f5f

    
           // This should be skipped by reentrancy guard 
        
           _ = innerClient.GetAsync("/inner", cancellationToken).GetAwaiter().GetResult();

8. Contradictory comment block in NestedRequestHandler (comment-analyzer)

Lines 343-350 contain a paragraph describing an abandoned approach, followed by "But for a proper test, we actually construct an inner pipeline:" -- contradicting the preceding text. This is a draft-then-revise artifact.

CSharp-ACDC/tests/CSharpAcdc.Tests/Handlers/LoggingHandlerTests.cs

Lines 343 to 350 in f837f5f

    
           // The reentrancy check happens in LoggingHandler. Since we're inside the 
        
           // outer LoggingHandler's SendAsync, the AsyncLocal<bool> IsLogging is true. 
        
           // We simulate the inner handler returning without actually nesting a second 
        
           // LoggingHandler (which would require constructing one and calling it). 
        
           // Instead, we validate that the flag is visible by checking it indirectly 
        
           // through the test's log count. 
        
           // But for a proper test, we actually construct an inner pipeline:

Test Coverage Gaps (Severity 7+)

Gap	Severity	Description
AsyncLocal reset after error	9/10	No test verifies `IsLogging` resets to `false` when `base.SendAsync` throws
SuppressLogging vs SkipLogging	8/10	Spec says `SuppressLogging` option, code uses `SkipLogging` -- naming mismatch
Custom fields replace vs merge	8/10	Setting custom `SensitiveFields` replaces all defaults, but no test verifies this behavior is intentional
Concurrent request isolation	7/10	No test verifies two parallel requests log independently without cross-contamination
Response header redaction	7/10	Response headers are redacted in code but no test asserts this

Strengths

AsyncLocal<bool> reentrancy guard with try/finally is a thoughtful design choice
No per-request state in instance fields -- correctly follows IHttpClientFactory handler pooling model
ConfigureAwait(false) used consistently
SensitiveDataRedactor.RedactJsonBody catch block is narrowly typed to JsonException
Comprehensive default sensitive fields list (16 fields covering common secrets)
ShouldSkip method provides clean per-request opt-out mechanism
33 tests covering both SensitiveDataRedactor and LoggingHandler

Type Design Summary

Type	Encapsulation	Invariant Expression	Usefulness	Enforcement
`AcdcLoggingOptions`	2/10	2/10	7/10	1/10
`SensitiveDataRedactor`	6/10	5/10	8/10	3/10
`LoggingHandler`	7/10	6/10	9/10	5/10

Recommended Action Plan

Harden AcdcLoggingOptions: Convert to record or add init setters + validation
Protect the pipeline: Wrap logging operations so they cannot crash HTTP requests
Defensive copy: Copy SensitiveFields in SensitiveDataRedactor constructor with forced OrdinalIgnoreCase
Add null guards: ArgumentNullException.ThrowIfNull() in constructors
Handle cancellation separately: Log OperationCanceledException at Information, not Error
Address JSON fallback: Either placeholder or log warning when body cannot be parsed for redaction
Add missing tests: AsyncLocal error recovery, concurrent isolation, response header redaction
Clean up comments: Remove contradictory NestedRequestHandler comment block

Generated by 5 specialized review agents (code-reviewer, test-analyzer, silent-failure-hunter, type-design-analyzer, comment-analyzer).

🤖 Generated with Claude Code

_{If this review was useful, please react with 👍. Otherwise, react with 👎.}

- Convert AcdcLoggingOptions to record with FrozenSet defaults - Protect HTTP pipeline from logging/redaction failures (try/catch) - Defensive copy of SensitiveFields with forced OrdinalIgnoreCase - Add ArgumentNullException.ThrowIfNull in constructors - Log OperationCanceledException at Information, not Error - Return placeholder for non-JSON bodies instead of leaking raw content - Fix repeated query param handling via GetValues() - Add null-check for response.Content - Rename IsLogging to _isLogging (naming convention) - Fix GetAwaiter().GetResult() to async/await in test - Remove unused usings, add using var for disposables - Add tests: AsyncLocal error recovery, concurrent isolation, cancellation - Improve reentrancy test with InnerLogger assertion - Enhance response header redaction test with sensitive header assertion Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings February 9, 2026 00:21

Copilot started reviewing on behalf of jhosm February 9, 2026 00:22 View session

Copilot AI reviewed Feb 9, 2026

View reviewed changes

jhosm merged commit ca5d6e2 into main Feb 12, 2026
1 check passed

jhosm deleted the feature/p3-logging-handler branch February 12, 2026 07:27

jhosm mentioned this pull request Feb 14, 2026

P8: Add end-to-end integration tests #7

Merged

4 tasks

-            redactedParts.Add(IsSensitive(name)
-                ? $"{Uri.EscapeDataString(name)}={Redacted}"
-                : $"{Uri.EscapeDataString(name)}={Uri.EscapeDataString(query[name] ?? string.Empty)}");
+            var values = query.GetValues(name);
+            var escapedName = Uri.EscapeDataString(name);
+            if (IsSensitive(name))
+            {
+                if (values is null || values.Length == 0)
+                {
+                    redactedParts.Add($"{escapedName}={Redacted}");
+                }
+                else
+                {
+                    foreach (var _ in values)
+                    {
+                        redactedParts.Add($"{escapedName}={Redacted}");
+                    }
+                }
+            }
+            else
+            {
+                if (values is null || values.Length == 0)
+                {
+                    redactedParts.Add($"{escapedName}=");
+                }
+                else
+                {
+                    foreach (var value in values)
+                    {
+                        redactedParts.Add($"{escapedName}={Uri.EscapeDataString(value ?? string.Empty)}");
+                    }
+                }
+            }

		{
		private static readonly AsyncLocal<bool> IsLogging = new();

	var request = new HttpRequestMessage(HttpMethod.Get, "/test");
	using var request = new HttpRequestMessage(HttpMethod.Get, "/test");

	var content = new StringContent(new string('x', 200));
	using var content = new StringContent(new string('x', 200));

Conversation

jhosm commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Review feedback addressed

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

jhosm commented Feb 9, 2026

Code review

Uh oh!

jhosm commented Feb 9, 2026

Comprehensive PR Review -- P3: LoggingHandler

Critical Issues

Important Issues

Test Coverage Gaps (Severity 7+)

Strengths

Type Design Summary

Recommended Action Plan

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jhosm commented Feb 9, 2026 •

edited

Loading