NCBC-4302: Collapse the two redactor implementations into one by davidkelly · Pull Request #183 · couchbase/couchbase-net-client

davidkelly · 2026-09-09T23:20:06Z

Motivation

The SDK has two redactors. They do the same job and honour the same RedactionLevel, but they are separate classes with separate lifetimes, and every cluster builds one of each.

NCBC-3079 (2021) moved the redaction logic into TypedRedactor — returning Redacted<T> rather than object?, so log arguments are not boxed and RedactMessage can be aggressively inlined — then gutted Redactor from 92 lines to 10 and kept it as a pass-through. The stated reason: "Keep IRedactor the same as a forwarder since it is a public API used by Transactions."

That reason protects the interface, which this PR keeps byte-identical. Transactions only ever called it, and has been in-assembly since NCBC-3920. So the forwarder class buys nothing, and which of the two a class ends up holding is an accident: measured on master, IRedactor is held by 39 files carrying 300 redaction call sites (88%) and TypedRedactor by 9 files carrying 40 (12%). Only the latter get the unboxed, inlinable path — and the split does not follow any design intent. Key-value, management, views, diagnostics and transactions all go through the boxing interface; the concrete-typed minority is the bootstrap/config/retry path, where ClusterNode alone is 23 of those call sites.

Modification

Four commits, each building and testing green on its own.

Merge the forwarder. TypedRedactor implements IRedactor, and Redactor is deleted. The interface members are implemented explicitly — implemented implicitly, the object-typed overloads would beat the generic ones at any call site whose argument is statically object, silently boxing and defeating the point of the type. Their null short-circuit is preserved verbatim. Behaviour-neutral: every existing IRedactor field still compiled untouched.
Move the holders onto the concrete type, so every redaction call site is on the same unboxed path.
Rename TypedRedactor to Redactor. "Typed" only ever meant "not the interface one".
Warn when a custom IRedactor is registered, since it no longer does anything (below).

One call site needed more than a type change: CertificateFactory null-coalesced a redacted certificate against a string, which stops unifying once the result is a struct, so it boxes explicitly. Trace-only, behind IsEnabled.

Tests drop Mock<IRedactor> for a real redactor at RedactionLevel.None. Those mocks were not stand-ins for behaviour — their methods returned null, silently blanking every field they were meant to be checking. The two that did behave were both pass-throughs, which is what RedactionLevel.None already does, so LoadTests' MockRedactor is deleted.

Why `IRedactor` survives

Most of this diff is one line per file swapping an IRedactor parameter or field for Redactor, which invites the obvious question: why keep an interface the SDK no longer uses anywhere?

Because it is not an internal wiring type, it is the boundary. Redacted<T> and the concrete redactor are both internal, so code outside the assembly cannot name them — IRedactor is the only way for a caller to reach the cluster's redactor and redact their own log arguments the way the SDK does. That is the single use it was ever made public for (NCBC-2598, Jul 2020), it still works — verified from an assembly referencing the built SDK with no InternalsVisibleTo, using only public API — and it is now in the release note.

The SDK stopped using it internally because it is expensive. Measured over the real Redacted<T>, 200M iterations, net8.0 Release: the concrete sealed class is 1.5 ns / 0 B per redaction; the same class reached through this object-typed interface is 9.9 ns / 56 B (a boxed payload plus a boxed Redacted<object>). That is what 88% of call sites were paying before this PR.

So internal callers take the concrete type for speed and external callers get the interface. IRedactor.cs now says exactly that in its own doc comment, which also fills in the empty <param>/<returns> tags it has carried since 2020. The file was previously untouched by this PR and therefore absent from the diff, which is why the question had nowhere to be answered.

Behaviour change: replacing the redactor

Registering your own via AddClusterService<IRedactor>(...) really did work — I verified it rather than assumed it, by registering one and reflecting on a DI-constructed holder. This ends it. It is less than it sounds:

Never an intended feature. When IRedactor was added in Feb 2020 the DI container was internal, so substitution was impossible. It became possible in Jul 2020 as a side effect of NCBC-2591, "Allow registration of custom services with the cluster", whose motivation was Linq2Couchbase registering serializers and document filters. Redaction is not mentioned in it.
Undocumented and untested. IRedactor appears zero times in docs-sdk-dotnet, no SDK RFC covers redaction, no test exercised substitution, and the interface appears in no public method signature — the DI key was the only way to reach it. Neither Java nor Go offers an equivalent.
Unguided. IRedactor has no redaction-level member and nothing hands one to an implementer, so honouring RedactionLevel meant knowing to read ClusterOptions.RedactionLevel yourself. Nothing in the SDK reads that level for control flow, so a custom redactor could ignore it silently.
Inconsistent since 2021. It still shaped 88% of call sites but was ignored on the bootstrap and config path, so logs carried two different redaction behaviours.

The interface stays public, so existing code compiles. A replaced registration is discarded at BuildServiceProvider() and a warning is logged, so ClusterServices always hands back a redactor that honours RedactionLevel — the same treatment AddClusterService<IRequestTracer> already gets today, silently, being likewise configured through ClusterOptions. A warning rather than a throw, so upgrading cannot break an application over a registration that no longer does anything.

This needs a release-note line.

The 2021 performance work is fully preserved

The obvious risk is that implementing an interface makes the generic methods virtual and costs devirtualization. It does not, because the interface members are explicit and land as three separate private members. Verified three ways, master vs this branch:

Compiled type shape (via MetadataLoadContext) — identical on both: sealed, Redacted<T> is a value type, the three generic methods non-virtual returning Redacted<T>, RedactMessage still carrying AggressiveInlining.

The boxing path is unreachable — counting members typed IRedactor in the built assembly: master 36 fields / 34 parameters, this branch 0 / 0. That covers source-generated logging code, which grep does not.

The repo's own benchmarks (Redactor_Rendered, Redactor_Unrendered, --job Short) — identical within error bars and identical allocations:

Benchmark	master (None / Partial / Full)	branch (None / Partial / Full)
Rendered	4.94 / 24.39 / 24.21 ns — 0 / 48 / 48 B	4.89 / 24.01 / 24.11 ns — 0 / 48 / 48 B
Unrendered	4.81 / 4.87 / 4.86 ns — 32 B flat	4.74 / 4.75 / 4.75 ns — 32 B flat

The 32 B is the benchmark boxing its own object return, present on master too.

As a side effect each cluster now builds one redactor instead of two: a single factory is registered under both keys.

Known deviation

Transactions still service-locates its redactor, now asking for the concrete type rather than the interface. Giving it a real injected dependency is not reachable without changing the public Transactions.Create(ICluster, TransactionsConfig) entry point, since it only ever receives an ICluster and has no container of its own. Left as a separate decision.

Results

Net 96 files, +442 / −353. Core/Logging is now IRedactor.cs (unchanged) plus one internal sealed class Redactor : IRedactor. No public API change.

All solutions build — couchbase-net-client, -onpremise, -di, -otel, and couchbase-fit-performer. Unit tests 3003 passed, 0 failed on net8.0 and net10.0, plus the 103 DI and OpenTelemetry tests the build-and-test gate does not run. Warning set compared leg by leg against master and unchanged.

Targets 3.10, since the next release is 3.10.0 and this carries a behavioural change rather than a fix. 3.10 was synced with master first (it was 4 behind); the four commits then rebase onto it cleanly and git range-diff reports all four patches byte-identical, so retargeting changed no code. The 3.10-only work (score fusion, staged user flags) introduces no redactor references, and both branches referenced IRedactor in exactly the same 45 files.

Draft pending a FIT run and the release note.

Release note

Keep this section in the squash-merge commit message. The 3.10.0 release notes are generated by walking the commit range, so if this text does not reach the commit, /release-notes will not see it. Also recorded on the ticket.

For ==== New Features and Behavioral Changes in docs-sdk-dotnet/modules/project-docs/pages/sdk-release-notes.adoc:

* https://couchbasecloud.atlassian.net/browse/NCBC-4302[NCBC-4302]:
Log redaction is now performed by a single internal redactor rather than two.
Registering your own `IRedactor` via `AddClusterService<IRedactor>` is now ignored: the registration is discarded
when the cluster is built and the SDK logs a warning, so the change is not silent.
This was never a documented extension point, and since 3.3.0 it had been honoured on some code paths and ignored on others.
The `IRedactor` interface is unchanged and remains public, and `ClusterOptions.RedactionLevel` is unaffected.
Retrieving the cluster's redactor in order to redact your own log arguments the same way the SDK does
remains supported: `cluster.ClusterServices.GetRequiredService<IRedactor>()` always returns a
redactor that honours the configured `ClusterOptions.RedactionLevel`.

The last two lines are the use case IRedactor was made public for in the first place (NCBC-2598, so the
then-separate Transactions package could redact its logs like the SDK). It is undocumented but it does
work — verified from an assembly referencing the built SDK with no InternalsVisibleTo, using only public
API, at all three redaction levels. Worth stating explicitly so the note does not read as "IRedactor is
finished", and so it has somewhere constructive to point anyone who was substituting.

https://couchbasecloud.atlassian.net/browse/NCBC-4302

🤖 Generated with Claude Code

github-actions · 2026-09-09T23:21:13Z

FIT performer image

Published:

ghcr.io/couchbase/dotnet-fit-performer:NCBC-4302

Run FIT locally against this PR:

fit run preset <preset-name> --performer dotnet-fit-performer:NCBC-4302

Or run it from the workflow here.

Note

Each push to this PR replaces the image and edits this comment. The image is deleted 7 days after the last push.

Motivation ========== Redaction is split across two classes that do the same job. NCBC-3079 moved the logic into TypedRedactor for performance but left Redactor in place as a pass-through, to avoid disturbing the public IRedactor interface used by Transactions. Merging the classes does not disturb that interface, and Transactions has been in-assembly since NCBC-3920, so the forwarder no longer earns its keep. Meanwhile every cluster builds two redactor singletons that share one redaction level. Modification ============ TypedRedactor now implements IRedactor directly and Redactor is gone. The interface members are implemented explicitly: implemented implicitly, the object-typed overloads would beat the generic ones at any call site whose argument is statically object, silently boxing and defeating the reason TypedRedactor exists. Their null short-circuit is preserved verbatim, so null still returns null rather than a Redacted<object> wrapping null. DefaultServices registers one singleton and resolves IRedactor from it, so both keys now yield the same object. StellarCluster stops hand-building new Redactor(new TypedRedactor(options)). This is a behaviour-neutral first step. Every existing IRedactor field compiles untouched, and substituting a redactor with AddClusterService<IRedactor> keeps working exactly as before. Later changes migrate those fields to the concrete type and rename it. Results ======= Unit tests pass on net8.0 and net10.0, 3000 passed and 0 failed. Three tests were added covering redaction through an IRedactor reference, the null short-circuit, and both service keys resolving to one instance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Motivation ========== With the forwarder gone, IRedactor is a public interface that nothing inside the SDK needs. Holding it still costs: a call through it boxes its argument and returns object, losing the Redacted<T> the typed redactor exists to produce, and the virtual call blocks the inlining it is annotated for. Which classes paid that cost was arbitrary. The KV and bootstrap paths did not; the management, views, diagnostics and data-structure paths did. Modification ============ The 41 files that held IRedactor now hold TypedRedactor, so every redaction call site is on the same unboxed path. Transactions asks ClusterServices for the concrete type, and StellarCluster's service provider answers for both keys. One call site needed more than a type change. CertificateFactory null-coalesced a redacted certificate against a string, which no longer unifies now that the result is a struct, so it boxes explicitly. That path is trace-only, behind IsEnabled. Tests drop Mock<IRedactor> for a real redactor at RedactionLevel.None. The mocks were not stand-ins for behaviour: their methods returned null, silently blanking every redacted field. The two that did behave were both pass-throughs, which is what RedactionLevel.None does, so LoadTests' MockRedactor is deleted. IRedactor stays public and stays registered, so ClusterServices still answers for it. But nothing in the SDK resolves it now, so replacing it with AddClusterService<IRedactor> no longer has any effect at all, where before it took effect for some classes and not others. Needs a release note. Results ======= The full solution builds, as do the DI, OpenTelemetry and FIT performer solutions. Unit tests pass on net8.0 and net10.0, 3000 passed and 0 failed, as do the 103 DI and OpenTelemetry tests that the build-and-test gate does not run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Motivation ========== "Typed" only ever distinguished this class from the forwarder that implemented the interface. That class is gone and this is the only redactor left, so the qualifier now distinguishes it from nothing. Modification ============ TypedRedactor becomes Redactor, and its file is renamed to match. The change is mechanical everywhere else. The class documentation is rewritten, since it described itself in terms of the class that no longer exists, and now says what the type does and why IRedactor is implemented explicitly. Three classes end up with a Redactor property of type Redactor, which the C# "Color Color" rule allows. Results ======= The full solution builds with an unchanged set of warnings, compared leg by leg against the previous commit. Unit tests pass on net8.0 and net10.0, 3000 passed and 0 failed, as do the DI and OpenTelemetry tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Motivation ========== Registering a custom IRedactor used to change how log arguments were redacted. After this refactor it does nothing, and it does nothing quietly: the registration still resolves, the application still compiles and runs, and the redactor is simply never consulted. A behaviour change that silent is worth announcing at runtime, not only in a release note. Modification ============ DefaultServices registers a single factory object under both the Redactor and IRedactor keys instead of resolving one through the other. That is simpler, and it makes replacement detectable: if the two keys no longer hold the same factory, something has overwritten IRedactor. BuildServiceProvider checks that and logs a warning pointing at RedactionLevel. A warning rather than an exception, because throwing would break an application on upgrade over a registration that no longer does anything. Results ======= Three tests: the warning fires on replacement, stays quiet otherwise, and a replaced redactor really is not consulted by a DI-constructed holder. Unit tests pass on net8.0 and net10.0, 3003 passed and 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Motivation ========== Review of the four preceding commits turned up one behaviour regression and a handful of small things. Transactions service-locates its redactor, and moving that lookup to the concrete type made it ask for an internal class. A caller-supplied ICluster cannot register one, so Transactions.Create threw for anyone passing their own ICluster, with no registration they could add to fix it. Both Create overloads and ICluster are public. Modification ============ The redactor lookup falls back to a default-level redactor instead of throwing, matching the IRequestTracer lookup on the very next line. RedactionLevel.None is the ClusterOptions default, so this is the same redactor a default-configured cluster hands out, and no real cluster can reach it: both Cluster and StellarCluster answer for the concrete type. The rest are cosmetic. The replaced-redactor warning used three placeholders bound to compile-time constants, one of them twice, so they are now literals and name ClusterOptions.RedactionLevel. Its summary says what the method does and names the DefaultServices coupling it detects through. The Redactor class remarks no longer repeat what the comment on the explicit members already says. BucketFactory passes the parameter name to ArgumentNullException like its neighbours. Two using directives, one unused and one unsorted. Single rather than First, so a missing field is diagnosable. Results ======= One test, which fails against the throw and passes against the fallback. Unit tests pass on net8.0 and net10.0, 3047 passed and 0 failed, as do the DI tests. Every TFM builds with no warnings, as do the integration, load test and FIT performer projects. Verified unchanged against master: Redactor is sealed, its generic methods are non-virtual returning Redacted<T>, RedactMessage still carries AggressiveInlining, and no member in the built assembly is typed IRedactor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Motivation ========== The previous commit left a custom IRedactor registration in place and merely stopped consulting it. Storing an object nothing uses is worth questioning on its own, and it has two concrete costs. It makes ClusterServices hand back a redactor the SDK does not use, which reads as confirmation that the substitution took. And fetching the cluster's redactor to redact your own log arguments is the one intended use of the public interface, dating to NCBC-2598; it is being written into the release note for 3.10.0. Storing the replacement makes that documented call return something that need not honour ClusterOptions.RedactionLevel, and nothing hands an implementer the level to honour. There is already a house pattern for a service configured through ClusterOptions rather than by registration: BuildServiceProvider overwrites it. AddClusterService<IRequestTracer> is discarded exactly this way today, silently. The redactor is the same shape. Modification ============ BuildServiceProvider now restores the built-in factory under the IRedactor key when it detects a replacement, and says so in the same warning as before, reworded from "will have no effect" to "has been ignored". Still a warning rather than a throw. This also settles the divergence with StellarCluster, which has always answered for IRedactor from its own field and so has never returned a registered redactor. Both cluster types now hand back the built-in one. Results ======= Unit tests pass on net8.0 and net10.0, 3048 passed and 0 failed, as do the DI tests; every TFM builds with no warnings. One test added: a replaced redactor is not handed back by ClusterServices, and both keys still resolve to the same instance. Verified, not assumed, that IRequestTracer is already discarded this way: registering one and building the provider hands back a RequestTracerWrapper, not the registered instance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Motivation ========== Most of this change is one line per file swapping an IRedactor parameter or field for the concrete Redactor, which invites the question: why keep an interface the SDK no longer uses anywhere? The file that answers it, IRedactor.cs, was untouched and therefore absent from the diff, so the question had nowhere to be answered. Its documentation did not answer it either. The summary was "An interface used for redacting specific log information" and every param and returns tag was empty, on a public interface, since 2020. CS1591 is suppressed, so nothing flagged it. Modification ============ Say what the type is for: it is the boundary, not internal wiring. Redacted<T> and the concrete redactor are both internal, so a caller outside the assembly cannot name them, and this interface is the only way to reach the cluster's redactor and redact your own log arguments the way the SDK does. That is the use it was made public for in NCBC-2598, and it is in the release note for 3.10.0. Also state what the SDK does instead and why, and that registering an implementation is discarded rather than honoured, so the answer is in the API reference rather than only in a release note. The per-method tags are filled in, including that the returned value defers formatting until it is converted to a string, and that metadata and system data are not redacted at Partial. Results ======= Builds with no warnings on every TFM, so every cref resolves; verified they reach Couchbase.NetClient.xml as real links rather than being dropped. Unit tests pass, 3048 passed and 0 failed. No API change: the interface, its members and their attributes are untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟡 Changes recommended

The public documentation makes an incorrect allocation claim, and invalid redaction levels still produce malformed diagnostics.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Consolidates log redaction into one concrete implementation while retaining IRedactor for external consumers.

Changes:

Merges and renames the typed redactor implementation.
Migrates internal consumers to the allocation-reducing concrete type.
Adds DI substitution warnings and associated tests.

File summaries

File	Description
`tests/Couchbase.UnitTests/Views/ViewClientTests.cs`	Uses the concrete redactor.
`tests/Couchbase.UnitTests/Utils/MockedHttpClients.cs`	Updates view-client test construction.
`tests/Couchbase.UnitTests/Utils/ArrayExtensionTests.cs`	Renames the redactor dependency.
`tests/Couchbase.UnitTests/Transactions/TransactionsForeignClusterTests.cs`	Tests foreign-cluster fallback behavior.
`tests/Couchbase.UnitTests/Transactions/PreserveTtlCapabilityTests.cs`	Renames the redactor dependency.
`tests/Couchbase.UnitTests/Transactions/CustomSerializerStagingRegressionTests.cs`	Replaces an interface mock with pass-through redaction.
`tests/Couchbase.UnitTests/MemcachedBucketTests.cs`	Renames the redactor dependency.
`tests/Couchbase.UnitTests/Management/Query/QueryIndexManagerTests.cs`	Removes nested redactor construction.
`tests/Couchbase.UnitTests/Management/Query/CollectionQueryIndexManagerTests.cs`	Updates manager construction.
`tests/Couchbase.UnitTests/Management/CollectionManagerTests.cs`	Replaces the interface mock.
`tests/Couchbase.UnitTests/Management/AnalyticsIndexManagerTests.cs`	Shares a concrete test redactor.
`tests/Couchbase.UnitTests/KeyValue/ScopeTests.cs`	Renames the redactor dependency.
`tests/Couchbase.UnitTests/KeyValue/CouchbaseCollectionZoneAwareTests.cs`	Uses the unified redactor.
`tests/Couchbase.UnitTests/KeyValue/CouchbaseCollectionTests.cs`	Uses the unified redactor.
`tests/Couchbase.UnitTests/KeyValue/CouchbaseCollectionCollectionIdTests.cs`	Uses the unified redactor.
`tests/Couchbase.UnitTests/CouchbaseBucketTests.cs`	Renames the redactor dependency.
`tests/Couchbase.UnitTests/Core/Retry/RetryOrchestratorTests.cs`	Updates retry test dependencies.
`tests/Couchbase.UnitTests/Core/Logging/RedactorSubstitutionTests.cs`	Tests replacement rejection and warnings.
`tests/Couchbase.UnitTests/Core/Logging/LogRedactionTests.cs`	Tests unified and interface redaction paths.
`tests/Couchbase.UnitTests/Core/IO/HTTP/CouchbaseHttpClientFactoryTests.cs`	Uses a concrete test redactor.
`tests/Couchbase.UnitTests/Core/IO/Errors/ErrorMapTests.cs`	Renames the redactor dependency.
`tests/Couchbase.UnitTests/Core/IO/Connections/DefaultConnectionPoolScaleControllerTests.cs`	Updates controller construction.
`tests/Couchbase.UnitTests/Core/IO/Connections/DataFlow/DataFlowConnectionPoolTests.cs`	Updates pool redactor types.
`tests/Couchbase.UnitTests/Core/IO/Connections/ConnectionPoolScaleControllerFactoryTests.cs`	Updates factory construction.
`tests/Couchbase.UnitTests/Core/IO/Connections/Channels/ChannelConnectionProcessorTests.cs`	Updates channel pool construction.
`tests/Couchbase.UnitTests/Core/IO/Connections/Channels/ChannelConnectionPoolTests.cs`	Updates channel pool construction.
`tests/Couchbase.UnitTests/Core/DI/ScopeFactoryTests.cs`	Renames the redactor dependency.
`tests/Couchbase.UnitTests/Core/DI/BucketFactoryTests.cs`	Renames the redactor dependency.
`tests/Couchbase.UnitTests/Core/Configuration/Server/ConfigPushHandlerTests.cs`	Updates configuration redactors.
`tests/Couchbase.UnitTests/Core/Configuration/Server/BucketConfigTests.cs`	Updates cluster-node construction.
`tests/Couchbase.UnitTests/Core/Configuration/Server/BucketConfigExtensionTests.cs`	Updates bucket construction.
`tests/Couchbase.UnitTests/Core/Configuration/ConfigHandlerTests.cs`	Uses the unified redactor.
`tests/Couchbase.UnitTests/Core/ClusterNodeReauthenticationTests.cs`	Updates cluster-node construction.
`tests/Couchbase.UnitTests/Core/ClusterContextTests.cs`	Updates cluster-node construction.
`tests/Couchbase.LoadTests/Helpers/MockRedactor.cs`	Removes the obsolete pass-through mock.
`tests/Couchbase.LoadTests/Core/Logging/Redactor_Unrendered.cs`	Renames benchmark target.
`tests/Couchbase.LoadTests/Core/Logging/Redactor_Rendered.cs`	Renames benchmark target.
`tests/Couchbase.LoadTests/Core/IO/Connections/ConnectionPoolFlowRate.cs`	Uses the unified benchmark redactor.
`tests/Couchbase.IntegrationTests/DataStructures/PersistentSetTests.cs`	Updates persistent-set construction.
`tests/Couchbase.IntegrationTests/DataStructures/PersistentQueueTests.cs`	Updates persistent-queue construction.
`tests/Couchbase.IntegrationTests/DataStructures/PersistentListTests.cs`	Updates persistent-list construction.
`tests/Couchbase.IntegrationTests/DataStructures/PersistentDictionaryTests.cs`	Updates persistent-dictionary construction.
`tests/Couchbase.IntegrationTests/Core/IO/Authentication/SaslTests.cs`	Updates connection factory construction.
`src/Couchbase/Views/ViewClient.cs`	Uses concrete redaction internally.
`src/Couchbase/Utils/ThrowHelper.cs`	Renames timeout-helper redactor types.
`src/Couchbase/Stellar/StellarCluster.cs`	Unifies Stellar redaction and service exposure.
`src/Couchbase/MemcachedBucket.cs`	Renames bucket redactor dependencies.
`src/Couchbase/Management/Views/ViewIndexManagerFactory.cs`	Resolves the concrete redactor.
`src/Couchbase/Management/Views/ViewIndexManager.cs`	Uses concrete redaction internally.
`src/Couchbase/Management/Users/UserManager.cs`	Uses concrete redaction internally.
`src/Couchbase/Management/Search/SearchIndexManager.cs`	Uses concrete redaction internally.
`src/Couchbase/Management/Query/QueryIndexManager.cs`	Uses concrete redaction internally.
`src/Couchbase/Management/Eventing/EventingFunctionService.cs`	Uses concrete redaction internally.
`src/Couchbase/Management/Collections/CollectionManagerFactory.cs`	Resolves the concrete redactor.
`src/Couchbase/Management/Collections/CollectionManager.cs`	Uses concrete redaction internally.
`src/Couchbase/Management/Buckets/BucketManager.cs`	Uses concrete redaction internally.
`src/Couchbase/Management/Analytics/AnalyticsIndexManager.cs`	Uses concrete redaction internally.
`src/Couchbase/KeyValue/CouchbaseCollection.cs`	Exposes concrete redaction internally.
`src/Couchbase/Diagnostics/UriTesterBase.cs`	Uses concrete redaction internally.
`src/Couchbase/Diagnostics/SearchUriTester.cs`	Updates redactor parameter type.
`src/Couchbase/Diagnostics/QueryUriTester.cs`	Updates redactor parameter type.
`src/Couchbase/DataStructures/PersistentStoreBase.cs`	Uses an optional concrete redactor.
`src/Couchbase/DataStructures/PersistentSet.cs`	Updates constructor dependency.
`src/Couchbase/DataStructures/PersistentQueue.cs`	Updates constructor dependency.
`src/Couchbase/DataStructures/PersistentList.cs`	Updates constructor dependency.
`src/Couchbase/DataStructures/PersistentDictionary.cs`	Uses an optional concrete redactor.
`src/Couchbase/CouchbaseBucket.cs`	Renames bucket redactor dependency.
`src/Couchbase/Core/Retry/RetryOrchestrator.cs`	Renames retry redactor dependency.
`src/Couchbase/Core/Logging/TypedRedactor.cs`	Removes the superseded implementation.
`src/Couchbase/Core/Logging/Redactor.cs`	Implements unified generic and interface redaction.
`src/Couchbase/Core/Logging/IRedactor.cs`	Documents external usage and substitution behavior.
`src/Couchbase/Core/IO/HTTP/CouchbaseHttpClientFactory.cs`	Uses concrete redaction internally.
`src/Couchbase/Core/IO/Connections/DefaultConnectionPoolScaleController.cs`	Uses concrete redaction internally.
`src/Couchbase/Core/IO/Connections/DataFlow/DataFlowConnectionPool.cs`	Uses concrete redaction internally.
`src/Couchbase/Core/IO/Connections/ConnectionPoolScaleControllerFactory.cs`	Uses concrete redaction internally.
`src/Couchbase/Core/IO/Connections/ConnectionPoolFactory.cs`	Uses concrete redaction internally.
`src/Couchbase/Core/IO/Connections/ConnectionFactory.cs`	Uses concrete redaction internally.
`src/Couchbase/Core/IO/Connections/Channels/ChannelConnectionPoolFactory.cs`	Uses concrete redaction internally.
`src/Couchbase/Core/IO/Connections/Channels/ChannelConnectionPool.cs`	Uses concrete redaction internally.
`src/Couchbase/Core/IO/Connections/CallbackCreator.cs`	Uses concrete certificate-log redaction.
`src/Couchbase/Core/IO/Authentication/X509/CertificateFactory.cs`	Adapts nullable certificate redaction.
`src/Couchbase/Core/IO/Authentication/CertificateValidationCallbackFactory.cs`	Uses concrete redaction internally.
`src/Couchbase/Core/Diagnostics/Metrics/AppTelemetry/WebSocketClientHandler.cs`	Resolves the concrete redactor.
`src/Couchbase/Core/Diagnostics/Metrics/AppTelemetry/AppTelemetryCollector.cs`	Uses concrete redaction internally.
`src/Couchbase/Core/DI/DefaultServices.cs`	Registers one singleton under both redactor keys.
`src/Couchbase/Core/DI/CollectionFactory.cs`	Injects the concrete redactor.
`src/Couchbase/Core/DI/ClusterNodeFactory.cs`	Injects the renamed redactor.
`src/Couchbase/Core/DI/BucketFactory.cs`	Injects the renamed redactor.
`src/Couchbase/Core/Configuration/Server/ConfigPushHandlerFactory.cs`	Injects the renamed redactor.
`src/Couchbase/Core/Configuration/Server/ConfigPushHandler.cs`	Uses the renamed redactor.
`src/Couchbase/Core/ClusterNode.cs`	Uses the unified redactor.
`src/Couchbase/Core/ClusterContext.cs`	Resolves the concrete redactor.
`src/Couchbase/Core/BucketBase.cs`	Updates the base redactor contract.
`src/Couchbase/ClusterOptions.cs`	Discards custom redactors with a warning.
`src/Couchbase/Cluster.cs`	Resolves the concrete redactor.
`src/Couchbase/Client/Transactions/Transactions.cs`	Uses concrete redaction with foreign-cluster fallback.
`src/Couchbase/Client/Transactions/Components/GetMultiManager.cs`	Uses concrete transaction redaction.
`src/Couchbase/Client/Transactions/AttemptContext.cs`	Updates the transaction redactor contract.

Review details

Files reviewed: 98/98 changed files
Comments generated: 2
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Motivation ========== Copilot's review of this PR raised two things, both real. The doc comment added to IRedactor promised that passing a redacted value to a logger "costs nothing at a disabled log level". That is true of the concrete redactor and false through the interface, which boxes the argument and the result before any level is consulted - measured at 56 bytes per call. Wrong on the very type whose cost is the reason the SDK stopped using it. An undefined RedactionLevel also produced a useless exception. Not just the placeholder Copilot spotted; all three diagnostics were empty: paramName=<null> actualValue=<null> message=Unexpected redaction level: {redactionLevel} The message carried a log-template placeholder in a plain string, so it was never interpolated, and Enum.GetName sat in the paramName slot, where it returns null for exactly the undefined values that reach it. Latent since NCBC-1870 in Feb 2020, and the only instance of the pattern in src. Worse than the diagnostics is when it fired. The level was only checked on the redaction path, so a bad one did not fail when it was configured; it turned the first log statement that redacted anything into an exception, from wherever that happened to be. Modification ============ The interface doc says formatting is deferred, and says once, in the remarks that already contrast the two types, that a call through it boxes both argument and result. The exception names ClusterOptions.RedactionLevel, the option a caller sets rather than a private parameter, reports the value, and drops the placeholder. The level is validated in the constructor, so an undefined one is rejected when the redactor is built rather than by a later log line. RedactMessage is untouched: its default branch calling a [DoesNotReturn] static is what keeps the method inlinable, which is the point of the type. Results ======= Six tests. Every defined level is accepted; an undefined one is rejected through both constructors with the parameter name, the actual value and no placeholder; and resolving the redactor from the service provider fails, which the test records as arriving wrapped in a TargetInvocationException since the factory constructs by reflection. Three of them fail against the previous code. Unit tests pass on net8.0 and net10.0, 3054 passed and 0 failed, as do the DI tests. Every TFM builds with no warnings, including the trim analyzer, as do the integration, load test and FIT performer projects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟡 Changes recommended

The public IRedactor documentation inaccurately claims that reference-type arguments are boxed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 98/98 changed files
Comments generated: 1
Review effort level: Balanced

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

The foreign-cluster fallback can silently expose transaction log data by ignoring a supplied public redactor.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 98/98 changed files
Comments generated: 1
Review effort level: Balanced

+            // A caller-supplied ICluster need not carry the SDK's internal services; None is the default level.
+            _redactor = _cluster.ClusterServices.GetService(typeof(Redactor)) as Redactor
+                ?? new Redactor(RedactionLevel.None);


davidkelly and others added 4 commits September 9, 2026 17:39

davidkelly force-pushed the NCBC-4302 branch from 2d9466d to 74d6229 Compare September 9, 2026 23:42

davidkelly changed the base branch from master to 3.10 September 9, 2026 23:42

davidkelly and others added 3 commits September 10, 2026 10:59

davidkelly requested a balanced review from Copilot September 10, 2026 18:31

Copilot started reviewing on behalf of davidkelly September 10, 2026 18:31 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Comment thread src/Couchbase/Core/Logging/IRedactor.cs Outdated

Comment thread src/Couchbase/Core/Logging/Redactor.cs Outdated

davidkelly marked this pull request as ready for review September 10, 2026 23:37

davidkelly requested review from emilienbev and jeffrymorris September 10, 2026 23:37

jeffrymorris requested a balanced review from Copilot September 11, 2026 18:59

Copilot started reviewing on behalf of jeffrymorris September 11, 2026 19:00 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

Comment thread src/Couchbase/Core/Logging/IRedactor.cs Outdated

Only value types are boxed not strings

b5ae901

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

jeffrymorris requested a balanced review from Copilot September 11, 2026 19:06

Copilot started reviewing on behalf of jeffrymorris September 11, 2026 19:08 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

Navigation Menu

Uh oh!

NCBC-4302: Collapse the two redactor implementations into one - #183

NCBC-4302: Collapse the two redactor implementations into one#183
davidkelly wants to merge 9 commits into
3.10from
NCBC-4302

davidkelly commented Sep 9, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

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

Uh oh!

Motivation

Modification

Why IRedactor survives

Behaviour change: replacing the redactor

The 2021 performance work is fully preserved

Known deviation

Results

Release note

Uh oh!

github-actions Bot commented Sep 9, 2026

FIT performer image

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davidkelly commented Sep 9, 2026 •

edited

Loading

Why `IRedactor` survives