Copilot comment:

This new cluster branch is unreachable for cluster commands: `RequestInstrumentation.typeMatcher()` still matches only `ActorRequest`, `Request`, `BufferedRequest`, and `RoundRobinPoolRequest`, but Rediscala 1.8's `RedisCluster` extends `RedisClientPoolLike` directly and declares its public `send` method. Consequently cluster commands produce no spans and never reach this configured-target extraction; include `redis.RedisClientPoolLike` (or `redis.RedisCluster`) in that matcher.

Analysis: `RedisCluster` declares the one-argument `send` method but did not match `RequestInstrumentation`, so its commands produced no spans. The matcher now includes only `RedisCluster` to avoid adding a second span around existing pool commands. Cluster futures use the pool execution context, which is resolved reflectively because its JVM return type differs across supported Rediscala versions.

Upsides: Cluster commands now produce spans with configured server targets. The focused matcher preserves existing pool span behavior, and reflective lookup keeps muzzle compatibility across Scala and Rediscala versions.

Downsides: Cluster completion adds one cached reflective method invocation per command.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Review finding:

`endpoint(Object)` is declared `public static`, but the only caller is `MutablePoolState.snapshot`, a private method of a nested class in this same file. No advice class, no other instrumentation class, and no test refers to it. The repository's Java style rules require the principle of least access: "Use the most restrictive modifier that still works." Make the method `private static`.

Review finding:

`MutablePoolState.markUnavailable()` and `MutablePoolState.refresh(HashMap)` are declared `public`, but every caller is either an enclosing `RediscalaServerTargets` method in this same file or `RediscalaServerTargetsTest`, which sits in the same package. The two sibling members of this class, `fromMap` and `target()`, are already package-private and carry `// visible for testing` comments, so the public modifier on these two methods is inconsistent with the class's own design and with the repository's least-access style rule. Make both methods package-private.

Analysis: `endpoint` has a single caller, `MutablePoolState.snapshot`, in the same top-level class, so `private static` is the most restrictive modifier that still compiles. `MutablePoolState` itself is only referenced by its enclosing class and by `RediscalaServerTargetsTest`, which declares the same package, so the class and its two mutating methods drop to package-private alongside the `fromMap` and `target()` members that were already package-private. A `VirtualField` value type does not need to be public: `ElasticsearchTransportServerTargets` uses a `private static class UpdateState` as its value type, so the runtime accepts a non-public one.

Upsides: The class now applies one consistent visibility rule instead of two. Nothing outside this package can call into instrumentation-internal state, and a future reader can find every caller of these members without searching the whole repository.

Downsides: No material downside identified. The unit test lives in a different Gradle module but the same package, so package-private access still works, which the module's test run confirms.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot comment:

[General] Stable requests from `SentinelMonitoredRedisBlockingClient` are handled by `RequestInstrumentation`, and `RediscalaServerTargets` already recognizes that client, but this endpoint overload does not. Because the blocking client carries its configured database on its underlying `redisClient`, `ServerEndpoint` stays null and stable `db.namespace` is omitted for every blocking Sentinel command (notably non-default databases). Add a branch that unwraps the blocking client's `redisClient` just like the regular Sentinel branch.

Analysis: `RequestInstrumentation` passes blocking Sentinel requests to `ServerEndpoint.create(Object, boolean)`, but the blocking client does not implement `RedisClientActorLike`. The new branch unwraps its `redisClient()` before endpoint extraction and does not depend on the master-only flag.

Upsides: Stable spans for blocking Sentinel commands now include the configured server endpoint and database namespace. The test covers a non-master command with a non-default database.

Downsides: Stable request handling performs one extra type check for clients that did not match an earlier branch.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Review finding:

This new `else if (action instanceof RedisClientPoolLike && POOL_EXECUTION_CONTEXT != null)` branch can never run, because the earlier `action instanceof Request` branch always claims the same receivers. Every rediscala class that extends `redis.RedisClientPoolLike` also implements `redis.RedisCommands`, and `redis.RedisCommands` extends `redis.commands.Keys`, which extends `redis.Request`. I checked this with javap over the whole supported range: `redis.RedisClientPool` in 1.5.0, 1.6.0, 1.7.0, 1.8.0, 1.9.0, 1.13.0 and 1.17.0; `redis.RedisClientMutablePool` in 1.6.0 onward; and `redis.RedisCluster` in 1.8.0 onward. Each one also exposes a working `executionContext()` returning `scala.concurrent.ExecutionContext`, so the existing `((Request) action).executionContext()` call already resolves correctly, including in 1.8.0 where `RedisClientPoolLike.executionContext()` itself returns `akka.dispatch.MessageDispatcher`. Reordering the branch would not help either, since the `Request` branch wins whatever the order. The remaining shapes `typeMatcher()` admits are covered too: an `ActorRequest` receiver is taken by the first branch, a `BufferedRequest` receiver never reaches `end()` because `start()` returns null for it, a `redis.RedisCluster` receiver implies `redis.Request`, and a `RoundRobinPoolRequest` receiver that somehow is not a `Request` is already served by the later `RoundRobinPoolRequest` branch with a direct call, which this reflective branch would only shadow. For completeness, the `BufferedRequest` branch in the same chain is also unreachable, but it predates this pull request and establishes no convention for adding new dead branches. Delete the branch together with the `POOL_EXECUTION_CONTEXT` field and the `logger` field it needs, and drop the imports that become unused: the static imports of `RediscalaServerTargets.findMethod` and `java.util.logging.Level.FINE`, plus `java.lang.reflect.Method`, `java.util.logging.Logger` and `redis.RedisClientPoolLike`. Keep the new `ctx == null` guard, which is a real fix.

Review finding:

Adding `"redis.RedisCluster"` to `hasSuperType(namedOneOf(...))` matches nothing that the list did not already match. `redis.RedisCluster` implements `redis.RedisCommands`, which extends `redis.commands.Keys`, which extends `redis.Request`, and `"redis.Request"` is already an entry in the same list. I confirmed the hierarchy with javap on 1.8.0, 1.9.0, 1.13.0 and 1.17.0, and `redis.RedisCluster` does not exist at all in 1.5.0, so there is no supported version where the extra entry changes which types are instrumented. The entry is also the only concrete class in a list that otherwise names traits, which makes the matcher read as though cluster clients need a special case when they do not. Remove the line.

Analysis: Both findings share one root cause. The pull request treated rediscala pool and cluster clients as a shape the existing instrumentation did not already cover, but they were covered all along, because `redis.RedisCommands` reaches `redis.Request` through `redis.commands.Keys`. That single fact makes the added type matcher entry match no new type and makes the added reflective execution context branch unreachable, since the `Request` branch takes every receiver that could have reached it. The reflection also solved a problem that does not arise: `RedisClientPoolLike.executionContext()` does return a different type across versions, but the call site goes through `Request`, whose signature is stable. So the fix removes the matcher entry, the dead branch, the `POOL_EXECUTION_CONTEXT` handle and the `logger` it used to report a failed lookup, and the five imports that then have no use. The `ctx == null` guard that the same pull request added stays, because a null execution context is reachable and skipping the callback for it is correct.

Upsides: The advice class no longer performs a reflective method lookup at class initialization, nor an `instanceof` and a null check per request, for a branch that never executes. The type matcher lists only traits again, so it no longer suggests that cluster clients need a special case. Five imports, two fields and a logger disappear, which leaves less code to keep working across the seven rediscala versions this instrumentation supports.

Downsides: If a future rediscala release introduced a `RedisClientPoolLike` implementation that is not a `redis.Request`, its execution context would no longer be found. That receiver would also fail the type matcher, so it would not be instrumented at all, and the branch would not have helped it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Master-and-replicas targets now go through RedisServerTarget.ofEndpointAndUnorderedEndpoints, the helper PR #20070 introduced for Redisson, instead of hand-rolling the master-first sort. Behavior is unchanged: the master stays first, replicas are sorted, and an unsupported replica still fails closed.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Review finding:

`typeMatcher()` in this new instrumentation uses only `named("redis.RedisClientActorLike")`, but the class also adds `classLoaderOptimization()` with the same type name. The repository's TypeInstrumentation guidance says name-only matchers are already fast and that `classLoaderOptimization()` should be added for expensive hierarchy or annotation matchers. Remove this redundant override and its `hasClassesNamed` static import.

Analysis: The exact-name matcher already rejects every unrelated type without inspecting bytecode. The extra class-loader probe repeats the same name check at an earlier stage and does not narrow the match.

Upsides: Removes redundant matcher work and follows the repository's TypeInstrumentation guidance.

Downsides: No material downside identified.

Review finding:

`typeMatcher()` in this new instrumentation uses only `named("redis.RedisClientMutablePool")`, but the class also adds `classLoaderOptimization()` with the same type name. The repository's TypeInstrumentation guidance says name-only matchers are already fast and that `classLoaderOptimization()` should be added for expensive hierarchy or annotation matchers. Remove this redundant override and its `hasClassesNamed` static import.

Analysis: The exact-name matcher already rejects every unrelated type without inspecting bytecode. The extra class-loader probe repeats the same name check at an earlier stage and does not narrow the match.

Upsides: Removes redundant matcher work and follows the repository's TypeInstrumentation guidance.

Downsides: No material downside identified.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 73cbb4d5-6c82-4793-8bbf-4771aa8ceb10

Review finding:

`RediscalaServerTargets.get(Object)` is declared `public`, but its only production callers are `AdviceScope.start` helpers in `RequestInstrumentation` and `TransactionInstrumentation`, which are in the same package. The annotated advice methods call those `start` helpers rather than calling `get` directly, and the repository visibility guidance says only members directly referenced by advice need public access; transitive helpers can stay package-private. Remove the `public` modifier.

Analysis: Both callers compile in the same package. The method is not part of an inlined advice body, so package access preserves advice dispatch while avoiding an unnecessary public helper API.

Upsides: Applies least access and keeps the helper surface limited to its package.

Downsides: No material downside identified.

Review finding:

The new `ServerEndpoint.create(Object)` and `create(Object, boolean)` overloads are declared `public`, but every caller is in this package. The annotated advice directly calls only the pre-existing `create(RedisClientActorLike)` overload; the new overloads are reached from `AdviceScope.start` helpers, so the repository's direct-advice visibility exception does not require them to be public. Remove `public` from both new overloads.

Analysis: The two Object overloads are package-internal dispatch helpers. The directly advice-referenced RedisClientActorLike overload remains public, so narrowing these overloads does not change advice linkage.

Upsides: Applies least access without changing endpoint selection behavior.

Downsides: No material downside identified.

Review finding:

`RediscalaTransactionState.getClient()` and `getEndpoint()` are declared `public`, but their only caller is `TransactionInstrumentation.AdviceScope.start` in the same package. The state class and constructor need public access because the annotated transaction-builder advice directly creates the value, but no advice method directly calls either getter. Make both getters package-private under the repository's least-access rule.

Analysis: TransactionInstrumentation reads both values from a same-package helper. The class and constructor stay public for direct advice construction, while the getters need no cross-package access.

Upsides: Keeps only the advice-linked state construction API public.

Downsides: No material downside identified.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 73cbb4d5-6c82-4793-8bbf-4771aa8ceb10

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>