feat: add server address tracing for RedisCluster connections #1026
Conversation
This commit enhances the RedisConnectionAspect to support server address tracing for RedisCluster connections by implementing slot-based node resolution. The implementation includes: - Added WeakMap-based caching for cluster slot information to minimize redundant CLUSTER SLOTS commands - Implemented getSlotByKey() to determine the hash slot for a given key - Implemented findNodeBySlot() to locate the master node responsible for a specific slot range - Extracts and sets server address (host/port) in tracing context for RedisCluster operations This enables distributed tracing systems to accurately track which Redis cluster node handles each operation, improving observability for clustered Redis deployments.
Walkthrough为 Redis 集群追踪添加了基于 WeakMap 的插槽到节点映射缓存；新增构造函数初始化缓存；在 process 中对 RedisCluster 键计算 slot、解析对应主节点（host/port/nodeId）并写入追踪上下文；新增私有方法 Changes
Sequence Diagram(s)sequenceDiagram
participant Tracer
participant Aspect as RedisConnectionAspect
participant RC as RedisCluster
participant Context as Tracing Context
Tracer->>Aspect: process(connection, key)
activate Aspect
alt connection is RedisCluster and key is string
Aspect->>Aspect: getClusterNodeBySlot(rc, key)
activate Aspect
Note over Aspect: 计算 KEYSLOT (CRC16)\n并从缓存或 RC 获取 slots 映射
Aspect->>RC: 获取 slots（仅当未缓存）
RC-->>Aspect: slots 数组
Note over Aspect: 将 slots 缓存到 WeakMap（关联 RedisCluster 实例）
Aspect-->>Aspect: 找到对应 master 节点 {host, port, nodeId?}
deactivate Aspect
Aspect->>Context: 设置节点 host/port/(nodeId)
else
Aspect->>Context: 使用现有连接信息写入
end
deactivate Aspect
Aspect-->>Tracer: 处理完成
Estimated code review effort🎯 3 (中等) | ⏱️ ~20 分钟 需要额外关注的文件/点：
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.32)At least one path must be specified to analyse. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (1)
76-91: 代码结构符合 Redis CLUSTER SLOTS 文档规范。验证结果确认：Redis CLUSTER SLOTS 命令返回的格式结构与代码期望相符。每个槽位范围条目至少包含
[start-slot, end-slot, master-node]，主节点信息数组至少包含[IP, port, node-id]。代码在第 79 行的数组解构和第 84-85 行对主机、端口的访问在 Redis 规范下是安全的，对nodeId的可选处理（使用?? null）也是正确的。添加数组结构验证将是可选的防御性编程改进，但对于直接来自 Redis 连接的数据而言不是必需的。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (1)
src/sentry/src/Constants.php (1)
Constants(14-43)
🪛 GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php
[failure] 71-71:
Method RedisCluster::cluster() invoked with 2 parameters, 3 required.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
🔇 Additional comments (2)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (2)
20-20: WeakMap 缓存实现合理！使用 WeakMap 来缓存集群槽位信息是很好的选择，它可以在 RedisCluster 实例被垃圾回收时自动清理缓存，避免内存泄漏。
Also applies to: 30-30, 32-35
48-60: Now I need to examine the actual code file to understand the full context better.验证 RedisCluster 多键命令处理
当前实现在行 50 处仅提取第一个参数作为 key (
$arguments[0])。在 Redis 集群中，任何操作多个哈希槽的命令都是被禁止的。需要确认：
- 项目中是否使用多键命令（MGET、MSET、DEL 多键等）与 RedisCluster
- Redis Enterprise 允许 DEL、MSET、MGET、EXISTS、UNLINK、TOUCH 跨槽执行，但开源 Redis 集群会抛出 CROSSSLOT 错误
- 此实现仅为追踪方面，提取第一个键用于上下文设置，这通常是可以接受的
建议代码审查者确认所使用的 Redis 集群版本及实际应用中的命令模式。
| private function getSlotByKey(RedisCluster $rc, string $key): int | ||
| { | ||
| return (int) $rc->cluster('CLUSTER', 'KEYSLOT', $key); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
验证 CLUSTER KEYSLOT 命令的调用方式并添加错误处理。
需要确认以下几点：
$rc->cluster('CLUSTER', 'KEYSLOT', $key)是否是调用 CLUSTER KEYSLOT 的正确方式- 如果 cluster() 调用失败（例如网络错误、集群不可用），当前没有任何错误处理，可能导致未捕获的异常
请搜索 RedisCluster 的官方文档以确认 API 使用方式：
同时，考虑添加 try-catch 来处理潜在的异常情况。
🌐 Web query:
PHP RedisCluster cluster() method CLUSTER KEYSLOT usage
💡 Result:
Short answer — call Redis' CLUSTER KEYSLOT through RedisCluster->cluster (or rawCommand) and it returns the integer slot for a key. Example:
-
Using RedisCluster::cluster:
$rc = new RedisCluster(NULL, ['127.0.0.1:7000']);
$slot = $rc->cluster(['127.0.0.1',7000], 'KEYSLOT', 'mykey'); // integer slot -
Or with rawCommand (if you prefer):
$slot = $rc->rawCommand('CLUSTER', 'KEYSLOT', 'mykey');
Notes:
- CLUSTER KEYSLOT implements Redis’ cluster hashing including hash-tags (the {...} behavior) and returns an integer slot 0–16383. [1]
- Some Redis Enterprise deployments require enabling the OSS cluster API for CLUSTER KEYSLOT to be allowed. [3]
- phpredis exposes RedisCluster->cluster(...) and rawCommand(...) for sending cluster/any commands to a specific node. [2]
Sources:
[1] Redis CLUSTER KEYSLOT doc.
[2] phpredis API / RedisCluster method signatures (cluster/rawCommand).
[3] Redis Enterprise KB on CLUSTER KEYSLOT restrictions.
修复 RedisCluster API 调用方式 - 当前传参错误。
根据 phpredis 文档，cluster() 方法的第一个参数应为节点标识符（数组形式），而非 'CLUSTER' 字符串。
当前错误的调用方式（第 66 行）：
return (int) $rc->cluster('CLUSTER', 'KEYSLOT', $key);正确的调用方式之一：
return (int) $rc->rawCommand('CLUSTER', 'KEYSLOT', $key);或使用带节点参数的 cluster() 方法。
同时建议添加 try-catch 块处理潜在的异常（如网络错误或集群不可用）。
🤖 Prompt for AI Agents
In src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php around lines 64 to
67, the RedisCluster API call is incorrect: cluster('CLUSTER', 'KEYSLOT', $key)
passes a string as the first argument; change this to call rawCommand('CLUSTER',
'KEYSLOT', $key) or call cluster() with the correct node identifier signature
per phpredis docs, cast the result to int as before, and wrap the call in a
try-catch to handle and log exceptions (network/cluster errors) and return a
sensible fallback or rethrow after logging.
| private function getSlots(RedisCluster $rc): array | ||
| { | ||
| $this->slotNodeCache[$rc] ??= $rc->cluster('CLUSTER', 'SLOTS'); | ||
|
|
||
| return $this->slotNodeCache[$rc]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
修复 RedisCluster::cluster() 方法参数数量不匹配问题。
静态分析工具报告 Line 71 存在严重问题：RedisCluster::cluster() 方法需要 3 个参数，但实际只传递了 2 个参数。这可能导致运行时错误。
请运行以下脚本验证 RedisCluster API 的正确用法：
同时搜索官方文档确认正确的调用方式：
根据 Redis 协议，CLUSTER SLOTS 不需要额外参数。如果 cluster() 方法要求 3 个参数，可能需要传递一个空字符串或 null 作为第三个参数，或者使用不同的方法调用方式（如 $rc->rawCommand('CLUSTER', 'SLOTS')）。
🏁 Script executed:
#!/bin/bash
# 检查 RedisCluster 扩展文档和存根文件
fd -e php stub --exec rg -n "function cluster" {} \;
fd -e php Redis --exec rg -nA5 "class RedisCluster" {} \;Length of output: 0
🌐 Web query:
PHP RedisCluster cluster method signature CLUSTER SLOTS
💡 Result:
Signature (from the phpredis API):
RedisCluster::cluster(string|array $key_or_address, string $command, mixed ...$extra_args) — mixed. [1]
Example to run CLUSTER SLOTS (returns the slots array as Redis does):
$slots = $redisCluster->cluster(null, 'SLOTS');
Or specify a node/address as first arg:
$slots = $redisCluster->cluster('127.0.0.1:7000', 'SLOTS');
CLUSTER SLOTS reply format described by Redis. [2]
Sources:
[1] phpredis RedisCluster API docs — RedisCluster::cluster.
[2] Redis CLUSTER SLOTS command specification (reply format).
修复 RedisCluster::cluster() 方法的参数错误
根据 phpredis 官方文档，RedisCluster::cluster() 方法签名为 cluster(string|array $key_or_address, string $command, mixed ...$extra_args)，第一个参数是节点地址或 null，第二个参数才是集群子命令。
第 71 行的代码调用方式错误：
$rc->cluster('CLUSTER', 'SLOTS')应改为：
$rc->cluster(null, 'SLOTS')正确的调用方式是 $redisCluster->cluster(null, 'SLOTS')。当前代码错误地将 'CLUSTER' 作为节点地址传入，会导致 CLUSTER SLOTS 命令执行失败。
🧰 Tools
🪛 GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
[failure] 71-71:
Method RedisCluster::cluster() invoked with 2 parameters, 3 required.
🤖 Prompt for AI Agents
In src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php around lines 69 to
74, the call to RedisCluster::cluster uses the wrong argument order — it
currently calls cluster('CLUSTER', 'SLOTS') which passes 'CLUSTER' as the
node/address; change the call to pass null as the first argument and 'SLOTS' as
the second (i.e. call cluster(null, 'SLOTS')) so the CLUSTER SLOTS command is
invoked correctly and the slot node cache population works as intended.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (3)
64-64: 修复 CLUSTER KEYSLOT 命令的错误调用方式。根据之前的评审反馈，这个 API 调用方式仍然不正确。phpredis 的
RedisCluster::cluster()方法签名为cluster(string|array $key_or_address, string $command, mixed ...$extra_args)，第一个参数应该是节点地址或null，而不是'CLUSTER'。应用以下修复：
- $slot = $rc->cluster('CLUSTER', 'KEYSLOT', $key); + $slot = (int) $rc->rawCommand('CLUSTER', 'KEYSLOT', $key);或者使用：
- $slot = $rc->cluster('CLUSTER', 'KEYSLOT', $key); + $slot = (int) $rc->cluster(null, 'KEYSLOT', $key);注意：需要将结果转换为 int 类型。
65-65: 修复 CLUSTER SLOTS 命令的错误调用方式。根据之前的评审反馈，这个 API 调用方式仍然不正确。同样的问题：第一个参数应该是
null而不是'CLUSTER'。应用以下修复：
- $slots = ($this->slotNodeCache[$rc] ??= $rc->cluster('CLUSTER', 'SLOTS')); // @phpstan-ignore-line + $slots = ($this->slotNodeCache[$rc] ??= $rc->cluster(null, 'SLOTS')); // @phpstan-ignore-line
62-80: 缺少错误处理机制。
getClusterNodeBySlot方法中的 Redis 集群命令调用可能会抛出异常（例如网络错误、集群不可用、节点故障等），但当前没有任何错误处理。这可能导致追踪功能中断或应用程序崩溃。建议添加 try-catch 块来处理潜在的异常：
private function getClusterNodeBySlot(RedisCluster $rc, string $key) { + try { $slot = (int) $rc->rawCommand('CLUSTER', 'KEYSLOT', $key); $slots = ($this->slotNodeCache[$rc] ??= $rc->cluster(null, 'SLOTS')); // @phpstan-ignore-line foreach ($slots as $range) { [$start, $end, $master] = $range; if ($slot >= $start && $slot <= $end) { // $master = [host, port, nodeId] return [ 'host' => $master[0], 'port' => $master[1], 'nodeId' => $master[2] ?? null, ]; } } + } catch (\Throwable $e) { + // 记录错误但不中断追踪流程 + // 可以考虑使用日志记录：logger()->warning('Failed to get cluster node by slot', ['error' => $e->getMessage()]); + return null; + } return null; }这样即使集群命令失败，也不会影响主要的 Redis 操作流程。
🧹 Nitpick comments (1)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (1)
48-58: RedisCluster 处理逻辑基本正确，但建议增强健壮性。当前实现从参数中提取键、计算插槽并设置追踪上下文的流程是合理的。不过可以考虑以下改进：
- 增加对空字符串键的验证
- 考虑记录日志当无法解析节点时（便于调试）
可选的改进建议：
if ($connection instanceof RedisCluster) { // RedisCluster $arguments = $proceedingJoinPoint->arguments['keys']['arguments'] ?? []; $key = $arguments[0] ?? null; - if (is_string($key)) { + if (is_string($key) && $key !== '') { $node = $this->getClusterNodeBySlot($connection, $key); if ($node !== null) { Context::set(Constants::TRACE_REDIS_SERVER_ADDRESS, $node['host']); Context::set(Constants::TRACE_REDIS_SERVER_PORT, $node['port']); } } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (1)
src/sentry/src/Constants.php (1)
Constants(14-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
🔇 Additional comments (1)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (1)
20-20: WeakMap 的使用和初始化正确。使用 WeakMap 缓存集群插槽信息是个好选择，可以避免内存泄漏，当 RedisCluster 实例被垃圾回收时，缓存也会自动清理。
Also applies to: 30-30, 32-35
* feat: add server address tracing for RedisCluster connections This commit enhances the RedisConnectionAspect to support server address tracing for RedisCluster connections by implementing slot-based node resolution. The implementation includes: - Added WeakMap-based caching for cluster slot information to minimize redundant CLUSTER SLOTS commands - Implemented getSlotByKey() to determine the hash slot for a given key - Implemented findNodeBySlot() to locate the master node responsible for a specific slot range - Extracts and sets server address (host/port) in tracing context for RedisCluster operations This enables distributed tracing systems to accurately track which Redis cluster node handles each operation, improving observability for clustered Redis deployments. * feat: optimize RedisCluster node tracing by consolidating slot retrieval methods --------- Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Summary
Changes
WeakMap $slotNodeCacheproperty to cache cluster slot information and reduce redundantCLUSTER SLOTScommandsgetSlotByKey()method to calculate the hash slot for a given Redis key usingCLUSTER KEYSLOTgetSlots()method with caching to retrieve and store cluster slot rangesfindNodeBySlot()method to locate the master node (host, port, nodeId) responsible for a specific slot rangeprocess()method to extract the key from command arguments and set server address contextTechnical Details
The implementation follows this flow:
CLUSTER KEYSLOTThis enables distributed tracing systems (like Sentry) to accurately track which Redis cluster node handles each operation, providing better visibility into RedisCluster deployments.
Test plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.