-
-
Notifications
You must be signed in to change notification settings - Fork 27
feat: add server address tracing for database connections #1024
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add DbConnectionAspect to capture database server address and port - Add TRACE_DB_SERVER_ADDRESS and TRACE_DB_SERVER_PORT constants - Register DbConnectionAspect in ConfigProvider - Update EventHandleListener to include server.address and server.port in tracing data This implements similar functionality to Redis server tracing for database connections.
Walkthrough为 Sentry tracing 添加了对数据库连接信息的捕获:新增 DbConnectionAspect 拦截 Hyperf 数据库连接以记录主机信息,新增常量用于存储地址/端口,并在 EventHandleListener 中将这两项作为 db span 标签输出。 Changes
Sequence Diagram(s)sequenceDiagram
participant Request
participant DbConnectionAspect
participant PDO
participant Context
participant EventHandleListener
participant TraceSpan
Request->>DbConnectionAspect: 调用 getPdo()/getPdoForSelect (被拦截)
DbConnectionAspect->>PDO: proceedingJoinPoint.process()
PDO-->>DbConnectionAspect: 返回 PDO 实例
alt 首次在当前 Context
DbConnectionAspect->>PDO: 读取连接信息 (host/port)
DbConnectionAspect->>Context: Context::getOrSet(TRACE_DB_SERVER_ADDRESS/PORT) 写入
end
DbConnectionAspect-->>Request: 返回 PDO
Request->>EventHandleListener: 生成/结束 SQL span
EventHandleListener->>Context: 读取 TRACE_DB_SERVER_ADDRESS / TRACE_DB_SERVER_PORT
EventHandleListener->>TraceSpan: 添加 db.server.address / db.server.port 标签
EventHandleListener-->>Request: 完成 span 标签注入
Estimated code review effort🎯 3 (中等复杂度) | ⏱️ ~20 分钟 需要额外关注:
Possibly related PRs
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
176-198:server.port始终为默认值 3306,未被实际填充这里从
Context::get(Constants::TRACE_DB_SERVER_PORT, 3306)读取端口。验证表明:
DbConnectionAspect中没有任何地方设置过TRACE_DB_SERVER_PORT到 Context;- 该常量仅定义和使用于此处,不存在任何其他 Context::set 调用;
- 结果是所有数据库查询的
server.port总是默认值 3306。这对非 MySQL 驱动(如 PostgreSQL)或自定义端口配置会产生明显的追踪错误,与 PR 目标"同时追踪地址和端口"不符。
建议至少选一种方案修复:
- 在 DbConnectionAspect 中补充设置端口(如果能从连接信息可靠解析);或
- 在此处回退到连接配置,确保端口从配置读取:
$defaultPort = (int) ($event->connection->getConfig('port') ?? 3306); $data += [ 'server.address' => (string) Context::get(Constants::TRACE_DB_SERVER_ADDRESS, 'localhost'), 'server.port' => (int) Context::get(Constants::TRACE_DB_SERVER_PORT, $defaultPort), ];
🧹 Nitpick comments (1)
src/sentry/src/Tracing/Aspect/DbConnectionAspect.php (1)
24-38:DbConnectionAspect建议补充 PDO 类型校验并顺带填充端口信息整体思路(通过
getPdo*第一次返回时解析连接信息并写入 Context)是合理的,也和 RedisConnectionAspect 的模式基本一致。不过这里有几个可以改进的点:
- 为
$pdo做类型防御,避免意外 fatal当前闭包直接调用:
$connectionStatus = $pdo->getAttribute(PDO::ATTR_CONNECTION_STATUS);如果未来某些驱动或自定义 Connection 返回的不是
PDO实例,会直接触发致命错误。建议加一个轻量的类型检查,例如:return tap($proceedingJoinPoint->process(), function ($pdo) { if (! $pdo instanceof PDO) { return; } if (! Context::get(self::class . '.executed')) { $connectionStatus = $pdo->getAttribute(PDO::ATTR_CONNECTION_STATUS); [$host] = explode(' ', (string) $connectionStatus); Context::set(Constants::TRACE_DB_SERVER_ADDRESS, $host); Context::set(self::class . '.executed', true); } });这样在非 PDO 场景下只是丢失 server 信息,不会影响主逻辑执行。
- 当前只设置了 address,没有设置 TRACE_DB_SERVER_PORT
本 PR 已经在
Constants中定义了TRACE_DB_SERVER_PORT,并在EventHandleListener中读取它来填充server.port,但这里没有写入,导致端口始终使用默认值。可以在解析ATTR_CONNECTION_STATUS时顺带尝试拆出端口,例如(示意):$connectionStatus = $pdo->getAttribute(PDO::ATTR_CONNECTION_STATUS); // 例如 "127.0.0.1:3307 via TCP/IP" [$hostPort] = explode(' ', (string) $connectionStatus, 2); [$host, $port] = array_pad(explode(':', $hostPort, 2), 2, null); Context::set(Constants::TRACE_DB_SERVER_ADDRESS, $host); if ($port !== null) { Context::set(Constants::TRACE_DB_SERVER_PORT, (int) $port); } Context::set(self::class . '.executed', true);这样一来 DB 的
server.address/server.port就都能根据真实连接信息填充。
- (可选)多连接场景下细化 executed 标记
目前的
self::class . '.executed'是类级别标记,同一协程内如果先后使用多个不同 DB 连接,后面的连接会一直沿用第一次解析到的 host。若你们有多库、多实例的使用场景,可以考虑通过ProceedingJoinPoint拿到Connection实例,再按连接名或 DSN 维度打标,例如:$connection = $proceedingJoinPoint->getInstance(); // Hyperf\Database\Connection $name = method_exists($connection, 'getName') ? $connection->getName() : 'default'; $key = self::class . '.executed.' . $name;然后用
$key存储 executed 与对应的 address/port,避免不同连接间互相覆盖。
[recommendation: 建议在后续迭代中根据实际多连接使用情况再考虑这一点。]
📜 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/Constants.php(1 hunks)src/sentry/src/Tracing/Aspect/DbConnectionAspect.php(1 hunks)src/sentry/src/Tracing/Listener/EventHandleListener.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/sentry/src/Tracing/Listener/EventHandleListener.php (1)
src/sentry/src/Constants.php (1)
Constants(14-41)
src/sentry/src/Tracing/Aspect/DbConnectionAspect.php (1)
src/sentry/src/Constants.php (1)
Constants(14-41)
src/sentry/src/ConfigProvider.php (1)
src/sentry/src/Tracing/Aspect/DbConnectionAspect.php (1)
DbConnectionAspect(22-41)
⏰ 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.1.2
- GitHub Check: Test on PHP 8.1 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.3 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.1 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.2 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.3 with Swoole 5.1.8
🔇 Additional comments (2)
src/sentry/src/ConfigProvider.php (1)
30-37: 将 DbConnectionAspect 注册到 aspects 列表是合理的新增的
Tracing\Aspect\DbConnectionAspect::class与现有DbAspect/RedisConnectionAspect的组织方式一致,能确保新的 DB 连接切面被 AOP 框架正确加载执行,看起来没有问题。src/sentry/src/Constants.php (1)
18-21: 新增 DB 追踪常量命名合理,但端口常量目前仅被读取未被写入
TRACE_DB_SERVER_ADDRESS/TRACE_DB_SERVER_PORT的命名和前缀与 Redis、RPC 等现有常量保持一致,便于统一管理 trace key,本身没有问题。需要注意的是:在当前改动中,只有
TRACE_DB_SERVER_ADDRESS在DbConnectionAspect中被写入 Context;TRACE_DB_SERVER_PORT虽已定义并在EventHandleListener中被读取,但尚未看到有任何写入逻辑,这会导致server.port一直走默认值(上一条评论已有展开)。建议后续补齐写入逻辑即可。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/sentry/src/Tracing/Aspect/DbConnectionAspect.php (1)
31-31: 建议为回调参数添加类型提示。为
$pdo参数添加PDO类型提示可以提高代码可读性和类型安全性。- return tap($proceedingJoinPoint->process(), function ($pdo) { + return tap($proceedingJoinPoint->process(), function (PDO $pdo) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sentry/src/Tracing/Aspect/DbConnectionAspect.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Aspect/DbConnectionAspect.php (1)
src/sentry/src/Constants.php (1)
Constants(14-41)
⏰ 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 5.1.8
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.8
- 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.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.2 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 5.1.8
- 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 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.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.1.2
🔇 Additional comments (1)
src/sentry/src/Tracing/Aspect/DbConnectionAspect.php (1)
22-27: 类结构和拦截目标设置正确。拦截
getPdoForSelect和getPdo方法能够覆盖数据库连接的主要场景。
* feat: add server address tracing for database connections - Add DbConnectionAspect to capture database server address and port - Add TRACE_DB_SERVER_ADDRESS and TRACE_DB_SERVER_PORT constants - Register DbConnectionAspect in ConfigProvider - Update EventHandleListener to include server.address and server.port in tracing data This implements similar functionality to Redis server tracing for database connections. * feat: 优化 DbConnectionAspect 以简化数据库连接状态追踪逻辑 --------- Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
This PR adds server address and port tracing for database connections, similar to the recent Redis server tracing implementation.
Changes:
The DbConnectionAspect intercepts calls to Connection::getPdoForSelect() and Connection::getPdo() methods, extracts the server address from the PDO connection status, and stores it in the Hyperf context for later use in tracing.
Test Plan:
Summary by CodeRabbit
发布说明
✏️ Tip: You can customize this high-level summary in your review settings.