feat: add server address tracing for Redis connections #1021
Conversation
Walkthrough将 Redis 连接信息纳入分布式追踪：新增 Changes
Sequence Diagram(s)sequenceDiagram
participant App as 应用代码
participant Hyperf as Hyperf Redis
participant Aspect as RedisConnectionAspect
participant Context as Trace Context
participant Listener as EventHandleListener
Note over App,Hyperf: Redis 调用发生
App->>Hyperf: 调用 RedisConnection::__call(...)
Hyperf->>Aspect: 切面拦截（Around）
Aspect->>Hyperf: proceed()（执行原始调用）
Hyperf-->>Aspect: 返回 Redis 实例
Aspect->>Context: 写入 TRACE_REDIS_SERVER_ADDRESS / TRACE_REDIS_SERVER_PORT
Note right of Context: 保存主机与端口信息
Listener->>Context: 读取 TRACE_REDIS_SERVER_ADDRESS / TRACE_REDIS_SERVER_PORT
Listener->>Listener: 将 server.address/server.port 附加到 Redis span 数据
Listener-->>App: 追踪跨度包含地址与端口
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分钟
Suggested reviewers
诗歌
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: 3
🧹 Nitpick comments (2)
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
505-505: 考虑过滤掉 null 值以避免污染 span 数据。当配置键缺失时，
$serverData可能包含null值（例如['server.address' => null, 'server.port' => null]）。将这些 null 值传播到 Sentry span 数据中可能导致：
- 数据噪音（无意义的 null 属性）
- 潜在的 Sentry SDK 处理问题
建议在展开前过滤 null 值：
+ ...array_filter($serverData, fn($value) => $value !== null), - ...$serverData,或者在构建 serverData 时使用条件逻辑：
-$serverData = match (true) { - $config['cluster']['enable'] ?? false => ['server.address' => $config['cluster']['nodes'] ?? null], - $config['sentinel']['enable'] ?? false => ['server.address' => $config['sentinel']['nodes'] ?? null], - default => ['server.address' => $config['host'] ?? null, 'server.port' => $config['port'] ?? null], -}; +$serverData = []; +if ($config['cluster']['enable'] ?? false) { + if (isset($config['cluster']['nodes'])) { + $serverData['server.address'] = $config['cluster']['nodes']; + } +} elseif ($config['sentinel']['enable'] ?? false) { + if (isset($config['sentinel']['nodes'])) { + $serverData['server.address'] = $config['sentinel']['nodes']; + } +} else { + if (isset($config['host'])) { + $serverData['server.address'] = $config['host']; + } + if (isset($config['port'])) { + $serverData['server.port'] = $config['port']; + } +}src/sentry/src/Util/ConnectionContainer.php (1)
20-48: LGTM - WeakMap 实现正确且高效。ConnectionContainer 的实现很好地利用了 WeakMap 的特性：
✅ 优点：
- 使用
WeakMap防止内存泄漏：连接对象销毁时自动清理映射- 延迟初始化
$mapping，避免不必要的内存分配- PHPDoc 泛型提供类型安全
- API 简洁明了
可选的增强建议：
可以考虑添加
has()方法以便检查键是否存在（区分"值为 null"和"键不存在"）：/** * @param TKey $object */ public static function has(object $object): bool { if (self::$mapping === null) { return false; } return isset(self::$mapping[$object]); }这在调试时可能有用，但对当前用例不是必需的。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/sentry/src/ConfigProvider.php(1 hunks)src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php(1 hunks)src/sentry/src/Tracing/Listener/EventHandleListener.php(2 hunks)src/sentry/src/Util/ConnectionContainer.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (1)
src/sentry/src/Util/ConnectionContainer.php (2)
ConnectionContainer(20-49)set(28-35)
src/sentry/src/Util/ConnectionContainer.php (1)
src/support/src/Once.php (1)
value(54-73)
⏰ 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.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 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 5.1.8
- 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.3 with Swoole 6.1.2
- 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.2 with Swoole 6.0.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.3 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
🔇 Additional comments (3)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (2)
22-24: LGTM - 切面目标定义正确。使用通配符模式
create*可以有效拦截所有 Redis 连接创建方法（createRedis、createRedisCluster、createRedisSentinel）。
28-30: 私有属性访问模式虽然必要，但需妥善防护第 30 行通过闭包绑定访问
RedisConnection的私有$config属性。验证确认 Hyperf\Redis\RedisConnection 不提供公共方法获取连接配置，因此这种访问方式是必要的。但是，该模式仍存在风险：依赖 Hyperf 的私有实现细节，如果未来版本中 Hyperf 重构或重命名该属性，此代码将失效。
建议采取以下防护措施：
- 添加清晰注释说明这是必要的 hack 及其原因
- 监控 Hyperf 版本更新，及时跟进可能的 API 变化
+ // 访问私有 $config 属性来获取连接配置 + // 必要原因：Hyperf\Redis\RedisConnection 不提供公共 API + // 风险：如果 Hyperf 在未来版本中移除或重命名该属性，此代码需要调整 $config = (fn () => $this->config ?? [])->call($instance);src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
467-473: 原始审查意见基于对代码架构的误解，建议的改动实际上无法实现。原始审查建议使用
ConnectionContainer从事件处理器中获取配置，但这在技术上存在根本问题：
- 事件只提供连接名称字符串：
CommandExecuted事件的$event->connectionName是字符串，不是连接对象实例- ConnectionContainer 使用对象作为 WeakMap 的键：在
RedisConnectionAspect中，ConnectionContainer::set()存储的是连接对象实例，而不是字符串- 无法通过字符串查询：
ConnectionContainer::get()期望接收原始对象实例作为键，而不是连接名称字符串。整个代码库中从未成功调用过ConnectionContainer::get()来检索值- 当前实现是正确的：直接从配置中按连接名称读取（第 468 行）是唯一可行的方法
ConnectionContainer的设计目的是在连接创建期间存储每个连接实例的配置，而不是在事件处理时通过连接名称进行检索。Likely an incorrect or invalid review comment.
- Add server.address and server.port to Redis tracing data - Support cluster and sentinel configurations - Add ConnectionContainer utility for WeakMap-based connection tracking - Add RedisConnectionAspect for connection configuration capture (commented out in config)
huangdijia
force-pushed
the
feat/redis-connection-tracing
branch
from
02d62e2 to
238fa35
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (2)
36-39: 考虑上下文设置的时机优化。如果此切面在每次 Redis 命令执行时触发，则会在每次操作时重复设置相同的服务器地址和端口上下文信息。这可能导致不必要的开销。
建议评估是否可以仅在连接建立时设置一次上下文，或者使用连接对象本身作为键来存储配置信息，避免重复设置。
41-43: RedisCluster 支持待实现。当前 RedisCluster 的处理逻辑为空。根据 PR 目标，需要添加对集群和哨兵配置的支持，包括适当的地址提取逻辑。
对于 RedisCluster，可能需要提取多个节点的地址信息或主节点信息。
如需帮助实现 RedisCluster 的地址提取逻辑，我可以协助生成相关代码。是否需要我创建一个新的 issue 来跟踪此任务？
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/sentry/src/Constants.php(1 hunks)src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php(1 hunks)src/sentry/src/Tracing/Listener/EventHandleListener.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sentry/src/Tracing/Listener/EventHandleListener.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (1)
src/sentry/src/Constants.php (1)
Constants(14-37)
⏰ 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.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 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.2 with Swoole 6.1.2
- 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.0.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.1 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 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
🔇 Additional comments (4)
src/sentry/src/Constants.php (1)
18-21: 常量定义符合预期。新增的 Redis 服务器地址和端口追踪常量与现有的 RPC 追踪常量保持一致，命名清晰，遵循了项目的命名约定。
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (3)
23-46: 确认切面的激活计划。根据 PR 说明，此切面当前在
ConfigProvider中被注释掉。在完成开发和测试后，请记得在配置中启用此切面以使其生效。
33-34: RedisConnection 属性验证已确认——代码正确。验证结果表明，
RedisConnection类确实包含这两个属性，且它们被声明为 受保护的属性 而非私有属性：
- 第 48 行：
protected null|Redis|RedisCluster $connection = null;- 第 51 行：
protected array $config = [...]通过闭包绑定访问受保护属性是 PHP 中的有效模式，代码使用
(fn () => $this->config ?? [])->call($redisConnection)正确地绑定闭包上下文以访问这些属性。实现方式正确，属性确实存在。
25-27: 验证拦截点设计是否最优，并注意切面当前处于禁用状态。切面拦截
RedisConnection::__call是有问题的。该魔术方法会在每次 Redis 命令执行时触发（如 get、set 等），导致process()方法中设置的上下文信息在每个命令上都冗余执行。这会产生性能开销。如果目标是在连接建立时一次性捕获配置信息，应改为拦截
RedisFactory::make()或RedisPool::get()等连接工厂方法。注意： 此切面当前在
ConfigProvider.php第 44 行被注释掉，活跃的是RedisAspect类。若后续计划启用此切面，请先验证上述设计问题。
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (2)
36-37: 建议添加错误处理。调用
$connection->getHost()和$connection->getPort()时没有检查返回值的有效性。虽然这些方法通常不会失败，但添加基本的验证可以提高代码的健壮性，避免将无效值写入追踪上下文。考虑添加简单的非空检查：
if ($connection instanceof Redis) { // Redis or RedisSentinel - Context::set(Constants::TRACE_REDIS_SERVER_ADDRESS, $connection->getHost()); - Context::set(Constants::TRACE_REDIS_SERVER_PORT, $connection->getPort()); + $host = $connection->getHost(); + $port = $connection->getPort(); + if ($host && $port) { + Context::set(Constants::TRACE_REDIS_SERVER_ADDRESS, $host); + Context::set(Constants::TRACE_REDIS_SERVER_PORT, $port); + } }
40-42: RedisCluster 支持标记为 TODO 是合理的。在草稿 PR 中保留 RedisCluster 的 TODO 是可以接受的。不过，在合并到主分支之前，建议要么实现 RedisCluster 支持，要么在文档中明确说明当前仅支持单机和 sentinel 模式。
需要帮助生成 RedisCluster 的实现代码吗？
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sentry/src/ConfigProvider.php(1 hunks)src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (2)
src/sentry/src/Constants.php (1)
Constants(14-37)src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
process(123-168)
src/sentry/src/ConfigProvider.php (1)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (1)
RedisConnectionAspect(23-45)
🪛 PHPMD (2.15.0)
src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php
31-31: Avoid unused parameters such as '$result'. (undefined)
(UnusedFormalParameter)
⏰ 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 6.0.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.3 with Swoole 6.1.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.2 with Swoole 6.1.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- 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.3 with Swoole 6.1.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.1 with Swoole 5.1.8
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.8
- 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.2 with Swoole 6.1.2
🔇 Additional comments (3)
src/sentry/src/ConfigProvider.php (1)
44-44: 已正确启用 Redis 连接追踪切面。该变更将
RedisConnectionAspect添加到切面列表中，使其能够拦截 Redis 连接调用并捕获服务器地址信息用于分布式追踪。这与 PR 目标一致。src/sentry/src/Tracing/Aspect/RedisConnectionAspect.php (2)
31-31: 静态分析标记的未使用参数是可接受的。静态分析工具标记
$result参数未使用，这在技术上是正确的。然而，这是tap()函数的惯用法——当你只需要执行副作用并返回原始值时，闭包参数可以不使用。当前实现是符合习惯的，无需修改。
33-33: 属性访问方式的准确性澄清。原始评论关于属性类型的描述有误：代码访问的
$connection是protected属性而非私有属性，这是 PHP 中访问受保护属性的有效模式。该属性在 Hyperf RedisConnection 类的第38行已明确定义为protected null|Redis|RedisCluster $connection = null，具有完整的类型声明。代码不会因属性名改变而"静默失败"——如果访问
$this->connection时属性不存在或为null，闭包会返回null，这是预期的安全行为，而非隐蔽的失败。由于属性在框架源代码中已明确声明且有类型声明，其稳定性有保障，删除此属性需要框架的重大版本变更。
* feat: add server address tracing for Redis connections - Add server.address and server.port to Redis tracing data - Support cluster and sentinel configurations - Add ConnectionContainer utility for WeakMap-based connection tracking - Add RedisConnectionAspect for connection configuration capture (commented out in config) * feat: improve server address handling for Redis connections * feat: 添加 Redis 服务器地址和端口追踪常量，并更新相关逻辑 * feat: 更新 Redis 连接追踪逻辑以直接获取服务器地址和端口 * feat: 更新 Redis 连接追踪逻辑，添加注释以区分 Redis 和 RedisCluster * fix: 修正 RedisConnectionAspect 中的变量命名以提高代码可读性 * feat: 启用 RedisConnectionAspect 以支持 Redis 连接追踪 --------- Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Summary
server.addressandserver.portattributes to Redis tracing data for better observabilityConnectionContainerutility class using WeakMap for tracking connection configurationsRedisConnectionAspectfor capturing connection configurations (currently commented out in ConfigProvider)Test plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.