Use configured Redis targets for Jedis 3.x telemetry - #20071
Draft
trask wants to merge 25 commits into
Draft
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Pull request dashboard statusWaiting on the author · refreshed 2026-09-12 03:43 UTC Move out of draft to request review. Status above doesn't look right?
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The broad Set VirtualField carrier should be narrowed, and normal cluster-command propagation lacks coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds configured logical Redis targets to Jedis 3.x telemetry while preserving legacy selected-endpoint behavior.
Changes:
- Propagates configured targets through connections, pools, shards, clusters, and Sentinel clients.
- Uses shared target normalization and ordering utilities.
- Adds integration and unit coverage plus a dedicated unit-test module.
File summaries
| File | Description |
|---|---|
settings.gradle.kts |
Registers the unit-test module. |
ShardedJedis30ClientTest.java |
Tests configured shard targets. |
JedisAggregateTargetTest.java |
Tests Sentinel and cluster refresh targets. |
Jedis30ClientTest.java |
Adds pooled-client coverage. |
ShardedRoutingInstrumentation.java |
Propagates targets during shard selection. |
ShardedJedisInstrumentation.java |
Captures shard configuration. |
PoolResourceInstrumentation.java |
Transfers pool targets to resources. |
JedisSingletons.java |
Manages target state and scopes. |
JedisServerTargets.java |
Builds normalized Redis targets. |
JedisSentinelPoolInstrumentation.java |
Captures Sentinel configuration and lifecycle. |
JedisInstrumentationModule.java |
Registers new instrumentations. |
JedisDbAttributesGetter.java |
Emits configured stable server attributes. |
JedisConnectionInstrumentation.java |
Captures and refreshes connection targets. |
JedisClusterInstrumentation.java |
Propagates cluster seed targets. |
JedisSingletonsTest.java |
Tests target replacement and scoping. |
JedisServerTargetsTest.java |
Tests rendering, ordering, and validation. |
javaagent-unit-tests/build.gradle.kts |
Configures focused unit tests. |
Review details
- Files reviewed: 17/17 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.
Copilot comment: [Javaagent] Using the ubiquitous JDK `Set` interface as the carrier registers this virtual field against every concrete `Set` implementation, not just Jedis' temporary parsed-sentinel sets. That unnecessarily broadens class matching/field injection to unrelated application and JDK collection classes. Please derive the target from the parsed sentinels or carry it on a narrow Jedis carrier such as the pool/listener instead. Analysis: JedisServerTargets.ofSentinels already accepts the parsed HostAndPort values. The initSentinels advice can derive the same normalized and sorted target from those values without attaching state to Set instances. Upsides: This removes virtual-field registration from unrelated Set implementations and deletes the parse-result bookkeeping. Downsides: The constructor and initSentinels paths normalize equivalent configured and parsed inputs independently. No material behavior change is expected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment: [Testing] The new cluster test invokes only `renewSlotCache`, which is covered by `RenewSlotCacheAdvice`; it never sends a normal command through `getConnection`/`getConnectionFromSlot`. This advice and its target attachment could therefore stop working while the suite remains green. Add a `SET` or `GET` through `JedisCluster` and assert the configured stable target/span name (and the unchanged legacy endpoint). Analysis: The test now sends SET through JedisCluster before renewing the slot cache. It checks the aggregate configured target in the stable span name and server attributes, and checks the announced Redis node in legacy mode. Upsides: The test now covers getConnection target propagation and protects both stable and legacy behavior. Downsides: The test sends one extra Redis command and makes three additional span assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The configured-target propagation is consistent with the stated semantics and is covered across the supported client paths.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Review finding:
This new test class carries `@SuppressWarnings("deprecation") // using deprecated semconv`, but it uses no deprecated API. Its only semconv imports are the stable `io.opentelemetry.semconv.ServerAttributes.SERVER_ADDRESS` and `SERVER_PORT`, plus `SemconvStability.emitStableDatabaseSemconv()`, which is not deprecated. `InstrumentationExtension.spans()`, `AutoCleanupExtension`, `SpanData`, and the Testcontainers APIs it uses are not deprecated either. The sibling class this PR adds, `ShardedJedis30ClientTest`, does import deprecated `DbIncubatingAttributes.DB_OPERATION`, `DB_STATEMENT`, and `DB_SYSTEM`, so its identical suppression is warranted and this one was copied along with it. `.github/instructions/java-style.instructions.md` says to add a deprecation suppression "only for intentional use of an API verified to be deprecated", and the repository review instructions say to place `@SuppressWarnings` on the single member that needs it rather than the class. Fix: delete the annotation and its comment from `JedisAggregateTargetTest`.
Analysis: JedisAggregateTargetTest asserts only against the stable server.address and server.port keys, so it never touches the deprecated incubating DB attributes that force a suppression on the other Jedis tests. Every remaining API it uses was checked against the managed dependency versions and none is deprecated: semconv 1.43.0 ServerAttributes, SDK 1.65.0 SpanData and Attributes, Testcontainers 2.0.5 GenericContainer and Wait, Awaitility 4.3.0, AssertJ 3.27.7, and the JUnit 5 annotations. The build disables javac deprecation diagnostics with -Xlint:-deprecation and relies on an Error Prone check that only reports symbols actually marked deprecated, so removing the annotation produces no diagnostic and no behavior change.
Upsides: The class no longer claims a deprecated-API dependency it does not have, so a reader can trust the annotation on ShardedJedis30ClientTest to mean what it says. A later change that does introduce a deprecated call now has to opt in deliberately instead of inheriting a class-wide suppression.
Downsides: No material downside identified.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Failing check: build / common / test2: JedisClusterException: CLUSTERDOWN The cluster is down Cause: The test assigned all slots and created the client before Redis reported the single-node cluster as healthy. Fix: Poll CLUSTER INFO until cluster_state is ok before constructing JedisCluster. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Failing check: build / test-latest-deps / testLatestDeps2: expected: "localhost:34279/mymaster" but was: "10.1.0.46:34279/mymaster" Cause: Jedis 3.9 resolves configured Sentinel hostnames before initSentinels, so the advice replaced the original endpoint with the resolved IP address. Fix: Associate the parsed Sentinel set with the configured set and use that configured set when assigning the pool target. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
trask
changed the title
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Broad Set instrumentation remains and one cluster pool-propagation path lacks coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Balanced
Copilot comment: The cluster integration test exercises `getConnection` and `renewSlotCache`, but never calls `getNodes` or borrows a resource from one of its returned pools. Consequently, this target-propagation path can regress while all new tests still pass. Add a test that obtains a pool through `JedisCluster#getClusterNodes`, executes a command on a borrowed Jedis instance, and verifies the aggregate stable target plus the selected legacy endpoint. Analysis: The new test gets the selected node pool from `JedisCluster#getClusterNodes`, borrows a Jedis resource, and runs `SET`. It checks the aggregate configured target in stable mode and the selected node endpoint in legacy mode. Upsides: The test now covers `GetNodesAdvice`, pool target propagation, and resource borrowing as one end-to-end path. Downsides: The test runs one more Redis command against the existing cluster fixture. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment: [Javaagent] The current diff still keys a `VirtualField` by the ubiquitous `Set` interface, so the agent must consider unrelated application/JDK `Set` implementations for context-store transformation. Keep this state on a Jedis-specific carrier, or derive the normalized target directly from the parsed sentinel values, rather than attaching it to the temporary set. Analysis: Each parsed `HostAndPort` now carries the original configured Sentinel collection. The target lookup reads that state from the Jedis-specific parsed values instead of registering a virtual field for every `Set` implementation. Upsides: Context-store transformation is limited to Jedis `HostAndPort` instances while configured hostnames still survive Jedis 3.9 address resolution. Downsides: The small configured-endpoint snapshot is attached to each parsed Sentinel address instead of once to the containing set. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Target propagation spans several version-sensitive Byte Buddy paths and Redis client topologies, warranting final human validation.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
trask
marked this pull request as ready for review
September 11, 2026 02:04
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Connections created by pool prefill or min-idle maintenance can still report selected endpoints in stable telemetry.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
…nections Copilot comment: [General] This scope only covers the borrow itself, so it does not repair a pooled connection created outside `getResource()` (for example via `Pool.addObjects()` or Commons Pool's min-idle maintenance). Such a connection captures the selected node in its constructor; after this scope closes, normal commands on the returned Jedis still report that node instead of the configured cluster/Sentinel target in stable mode. Persist the pool target onto every returned Jedis connection before closing the scope, and ensure background factory-created setup commands receive the same target; add an eager/min-idle pool case to the integration tests. Analysis: The borrow scope supplied the configured target only while Commons Pool returned a resource. A connection that the min-idle thread created earlier retained its node target, and setup commands created outside the borrow had no configured target. The pool now binds its factory to the configured target, scopes factory lifecycle calls, and stores the pool target on each borrowed Jedis connection before closing the scope. Upsides: Stable database semantic conventions now report the configured cluster or Sentinel target for eager connections, background setup commands, and later commands on borrowed clients. Legacy attributes keep reporting the connected node. Downsides: The instrumentation adds two per-object target associations and wraps configured Jedis factory lifecycle calls in a context scope. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Broad bytecode instrumentation across several asynchronous and pooled Jedis lifecycle paths warrants final human review despite strong targeted coverage.
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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
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.
Updates stable Jedis 3.x telemetry to report the Redis target configured by the application instead of the endpoint selected for each command. This applies to direct, pooled, sharded, cluster, and Sentinel clients, including span names and database client duration metrics.
Legacy telemetry continues to report the selected endpoint. Multi-endpoint targets keep shard order, sort cluster and Sentinel endpoints, and include at most five endpoints.