Skip to content

fix(lettuce5): decorate Redis command spans with resolved node connection for master-replica connection providers#11819

Open
ygree wants to merge 8 commits into
masterfrom
ygree/fix-lettuce5-masterreplica-instrumentation
Open

fix(lettuce5): decorate Redis command spans with resolved node connection for master-replica connection providers#11819
ygree wants to merge 8 commits into
masterfrom
ygree/fix-lettuce5-masterreplica-instrumentation

Conversation

@ygree

@ygree ygree commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

What Does This Do

Decorates Redis command spans with resolved node connection for master-replica connection providers

Before (no hostname) and After (hostname and other resolved connection related tags present):

Screenshot 2026-06-30 at 15-47-55 Screenshot 2026-06-30 at 15-49-19

Motivation

Redis command span missing hostname and other connection related tags when run with a master/replica connection

Additional Notes

Lettuce master/replica APIs expose a routing connection, while the concrete node connection is selected only when the command is dispatched.
The existing command advice only sees the wrapper connection, so static master/replica routing can leave peer.hostname unset.

Instrument the legacy MasterSlaveConnectionProvider.getConnection path and the newer MasterReplicaConnectionProvider.getConnection* paths. When a Redis command span is active, decorate it with the RedisURI from the resolved StatefulRedisConnection.

Contributor Checklist

  • Format the title according to the contribution guidelines
  • Assign the type: and (comp: or inst:) labels in addition to any other useful labels
  • Avoid using close, fix, or any linking keywords when referencing an issue
    Use solves instead, and assign the PR milestone to the issue
  • Update the CODEOWNERS file on source file addition, migration, or deletion
  • Update public documentation with any new configuration flags or behaviors
  • Add your completed PR to the merge queue by commenting /merge. You can also:
    • Customize the commit message associated with the merge with /merge --commit-message "..."
    • Remove your PR from the merge queue with /merge -c
    • Skip all merge queue checks with /merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)
    • Get more information in this doc

Jira ticket: APMS-19871

@ygree ygree self-assigned this Jun 30, 2026
@ygree ygree added type: bug Bug report and fix inst: lettuce Lettuce instrumentation tag: ai generated Largely based on code generated by an AI or LLM labels Jun 30, 2026
connection for master-replica connetion providers

Lettuce master/replica APIs expose a routing connection, while the
concrete node connection is selected only when the command is
dispatched. The existing command advice only sees the wrapper
connection, so static master/replica routing can leave peer.hostname
unset.

Instrument the legacy MasterSlaveConnectionProvider#getConnection path
and the newer MasterReplicaConnectionProvider#getConnection* paths. When
a Redis command span is active, decorate it with the RedisURI from the
resolved StatefulRedisConnection.
@dd-octo-sts

dd-octo-sts Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite Status
Startup 🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results
Scenario Candidate master Δ (95% CI of mean)
startup:insecure-bank:iast:Agent 14.03 s 13.97 s [-0.2%; +1.1%] (no difference)
startup:insecure-bank:tracing:Agent 12.97 s 12.98 s [-0.6%; +0.5%] (no difference)
startup:petclinic:appsec:Agent 16.93 s 16.77 s [-0.1%; +2.0%] (no difference)
startup:petclinic:iast:Agent 16.79 s 16.91 s [-1.5%; +0.1%] (no difference)
startup:petclinic:profiling:Agent 16.43 s 16.89 s [-7.1%; +1.6%] (no difference)
startup:petclinic:sca:Agent 16.95 s 16.85 s [-0.5%; +1.7%] (no difference)
startup:petclinic:tracing:Agent 15.62 s 15.61 s [-5.9%; +6.0%] (unstable)

Commit: a63c83f3 · CI Pipeline · Benchmarking Platform UI


Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

@ygree ygree changed the title Fix lettuce5 command span decoration when used with master-replica connection fix(lettuce5): decorate Redis command spans with resolved node connection for master-replica connection providers Jun 30, 2026
@ygree ygree marked this pull request as ready for review June 30, 2026 23:23
@ygree ygree requested a review from a team as a code owner June 30, 2026 23:23
@ygree ygree requested review from ValentinZakharov and removed request for a team June 30, 2026 23:23

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a2351d0726

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

import io.lettuce.core.RedisURI;
import io.lettuce.core.api.StatefulRedisConnection;
import io.lettuce.core.codec.StringCodec;
import io.lettuce.core.masterslave.MasterSlave;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep legacy-only test out of latest suite

This source is also compiled by latestDepTest because this module uses addTestSuiteForDir('latestDepTest', 'test') and latestDepTestImplementation depends on lettuce-core:+. The latest Lettuce 7.x no longer has the legacy io.lettuce.core.masterslave package (the production code treats it as [5,7)), so compileLatestDepTestJava/forked compile will fail on this import before tests run. Put this test in a 5.x-only source set or use the masterreplica API for the latest-dependency suite.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this against the current latest-dependency resolution. latestDepTestCompileClasspath resolves lettuce-core:+ to 7.6.0.RELEASE, and compileLatestDepTestJava --rerun-tasks succeeds.

Lettuce 7.6 still ships the public compatibility API io.lettuce.core.masterslave.MasterSlave.

Given that the MasterSlave public API exists in both our baseline 5.0 dependency and current latest 7.6 dependency, I’d prefer to keep this test in src/test for now so it runs against both versions. If Lettuce removes that compatibility API in a future release, we can then split it into a version-specific source set or add a latest-dependency variant using masterreplica.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the test so it prefers the newer master/replica API and falls back to the old one via reflection. This way, it uses the old API only before 5.2. This should keep working even if the deprecated API is removed completely.

Comment on lines +84 to +85
.and(takesArgument(0, named("io.lettuce.core.protocol.ConnectionIntent")))
.and(returns(named("java.util.concurrent.CompletableFuture"))),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Add legacy async master/slave matching

For Lettuce 5.1–6, io.lettuce.core.masterslave.MasterSlaveChannelWriter.write dispatches commands through MasterSlaveConnectionProvider.getConnectionAsync(Intent), not the synchronous getConnection(Intent). This async matcher only accepts the newer io.lettuce.core.protocol.ConnectionIntent, so those legacy versions never run either advice during command dispatch and still emit master/slave command spans without the resolved connection tags.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding few explicit tests in e0e65be for lettuce 5.1, 6.0, 6.1, and 6.2 uncovered some missing instrumentation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8add908 and c4b629d

ygree added 6 commits June 30, 2026 18:47
Prefer the newer MasterReplica facade in the Lettuce master/replica test
and fall back to the legacy MasterSlave facade when needed.
master/replica command span connection tagging across the version range.
Collapse version-specific `getConnection*` matchers to public method
shape matching and keep coverage for Lettuce 5.0 through latest
master/replica command span connection tagging.
@ygree ygree added this to the 1.64.0 milestone Jul 1, 2026
@datadog-datadog-prod-us1

Copy link
Copy Markdown
Contributor

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 69.73% (+14.74%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: a63c83f | Docs | Datadog PR Page | Give us feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inst: lettuce Lettuce instrumentation tag: ai generated Largely based on code generated by an AI or LLM type: bug Bug report and fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant