refactor(sentry): improve span null safety and consistency in tracing aspects #918
Conversation
… aspects - Replace null checks with null-safe operators in ElasticsearchAspect - Refactor FilesystemAspect to use SpanStarter trait for consistency - Add proper error handling and span status management in FilesystemAspect - Ensure all span operations use null-safe operators to prevent null pointer issues
Walkthrough本次变更在两个切面中调整了 Sentry span 的处理：Elasticsearch 切面改为对可能为 null 的 span 使用空安全调用；Filesystem 切面改为显式 startSpan + try/catch/finally 生命周期并引入 SpanStarter，用以记录状态与异常元数据并始终 finish span。（≤50 字） Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as 调用方
participant FS as FilesystemAspect
participant T as Sentry Tracer
participant S as Span
participant F as 原始文件系统操作
C->>FS: 调用被代理的方法(args)
FS->>T: startSpan("filesystem.operation", context)
T-->>FS: S (可能为 null)
alt span 存在
FS->>S: setData(args/tags)
end
FS->>F: 执行原始操作
F-->>FS: 返回结果 / 抛出异常
alt 成功
FS->>S: setStatus(ok)
FS->>S: finish()
FS-->>C: 返回结果
else 异常
FS->>S: setStatus(internalError)
FS->>S: setTag/ setData(异常类/消息/代码/可选堆栈)
FS->>S: finish()
FS-->>C: 重新抛出异常
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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.28)At least one path must be specified to analyse. Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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
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/Aspect/ElasticsearchAspect.php (1)
65-78: 避免把 $span 赋值为 setData 的返回值（可能导致丢失 Span 实例）当前写法把
$span设为setData(...)的返回值；若该方法返回void|null，$span将变为null，后续finish()不会执行，造成跨度未关闭。建议先保存startSpan(...)的返回，再单独调用setData(...)。应用如下修改：
- $span = $this->startSpan( + $span = $this->startSpan( op: 'db.elasticsearch', description: sprintf('%s::%s()', $proceedingJoinPoint->className, $proceedingJoinPoint->methodName), origin: 'auto.elasticsearch', - )?->setData([ + ); + $span?->setData([ 'coroutine.id' => Coroutine::id(), 'db.system' => 'elasticsearch', 'db.operation.name' => $proceedingJoinPoint->methodName, 'http.request.method' => '', // TODO 'url.full' => '', // TODO 'server.host' => '', // TODO 'server.port' => '', // TODO - 'arguments' => json_encode($proceedingJoinPoint->arguments['keys'], JSON_UNESCAPED_UNICODE), + 'arguments' => json_encode($proceedingJoinPoint->arguments['keys'] ?? [], JSON_UNESCAPED_UNICODE), ]);
🧹 Nitpick comments (6)
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (4)
77-78: 防止未定义索引与 JSON 失败静默
$proceedingJoinPoint->arguments['keys']可能不存在，建议加上空合并；同时可考虑JSON_THROW_ON_ERROR并在失败时回退为字符串“[unserializable]”。- 'arguments' => json_encode($proceedingJoinPoint->arguments['keys'], JSON_UNESCAPED_UNICODE), + 'arguments' => json_encode($proceedingJoinPoint->arguments['keys'] ?? [], JSON_UNESCAPED_UNICODE),（可选：再加
| JSON_INVALID_UTF8_SUBSTITUTE或JSON_THROW_ON_ERROR并捕获异常）
83-86: 结果体可能巨大/不可序列化，建议截断或限制将
$result整体json_encode可能非常大或包含资源对象。建议在开启该开关时对序列化后的字符串进行长度限制（如 16KB），或仅提取关键信息。
69-77: 避免写入空字段占用事件体积
http.request.method/url.full/server.host/server.port目前为''。建议在能取到真实值前，不写入这些键，减少噪声。
96-99: 堆栈字符串可能过长，建议截断
(string) $exception包含完整堆栈，建议限制长度（如 16KB）以控制体积。src/sentry/src/Tracing/Aspect/FilesystemAspect.php (2)
40-44: 重复设置 OK 状态为可选
startSpan()已默认设为ok（见 SpanStarter）。此处再次设置可省略，减少冗余。- $span?->setStatus(SpanStatus::ok());
52-56: 异常堆栈体积控制建议对
exception.stack_trace截断（例如 16KB）或仅保留前若干帧，避免事件过大。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/FilesystemAspect.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (1)
src/sentry/src/Switcher.php (1)
isTracingExtraTagEnabled(85-88)
src/sentry/src/Tracing/Aspect/FilesystemAspect.php (4)
src/sentry/src/Aspect/FilesystemAspect.php (2)
FilesystemAspect(20-133)process(48-65)src/sentry/src/Tracing/SpanStarter.php (1)
startSpan(32-52)src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (1)
process(59-107)src/sentry/src/Switcher.php (1)
isTracingExtraTagEnabled(85-88)
⏰ 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). (10)
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
- 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.1 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
🔇 Additional comments (3)
src/sentry/src/Tracing/Aspect/FilesystemAspect.php (3)
28-30: 已验证：开关方法与键名一致Switcher 类实现了 isTracingEnabled(string $key, bool $default = true)，调用 isTracingEnabled('filesystem') 有对应实现（定义：src/sentry/src/Switcher.php 行51；调用：src/sentry/src/Tracing/Aspect/FilesystemAspect.php 行28）。
23-24: LGTM：统一采用 SpanStarter，生命周期清晰引入 SpanStarter 并用 try/catch/finally 保证 finish()，方向正确。已尝试在仓库中扫描是否存在 "?->...->" 混用，但 ripgrep 返回 "No files were searched"，扫描未完成，无法确认是否存在该问题；合并前需人工核实或我可重新运行带 --no-ignore 的扫描复核。
25-26: 确认：已包含 symfony/polyfill-php83，可保留 #[Override]
composer.json 要求 php >=8.1，vendor 中已安装 symfony/polyfill-php83（含 Resources/stubs/Override.php 并已注册到 autoload），因此当前依赖已为 #[Override] 提供 polyfill，无需移除。
… aspects (#918) * refactor(sentry): improve span null safety and consistency in tracing aspects - Replace null checks with null-safe operators in ElasticsearchAspect - Refactor FilesystemAspect to use SpanStarter trait for consistency - Add proper error handling and span status management in FilesystemAspect - Ensure all span operations use null-safe operators to prevent null pointer issues * fix(filesystem): remove redundant span status setting in process method --------- Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sentry/src/Tracing/Aspect/FilesystemAspect.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Aspect/FilesystemAspect.php (4)
src/sentry/src/Aspect/FilesystemAspect.php (2)
FilesystemAspect(20-133)process(48-65)src/sentry/src/Tracing/SpanStarter.php (1)
startSpan(32-52)src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (1)
process(59-107)src/sentry/src/Switcher.php (1)
isTracingExtraTagEnabled(85-88)
⏰ 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). (12)
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
🔇 Additional comments (5)
src/sentry/src/Tracing/Aspect/FilesystemAspect.php (5)
23-23: 采用 SpanStarter trait：一致性提升，赞与其他 tracing aspect 统一为显式 startSpan 生命周期，方向正确。
34-38: 避免用 setData() 的返回值覆盖 $span保持 $span 指向真实的 Span 实例更稳妥（兼容潜在签名变化），先赋值再 setData。
- $span = $this->startSpan( - op: $op, - description: $description, - origin: 'auto.filesystem', - )?->setData($data); + $span = $this->startSpan( + op: $op, + description: $description, + origin: 'auto.filesystem', + ); + $span?->setData($data);
50-54: 按开关写入堆栈：实现符合预期条件化的
exception.stack_trace采集与开关对齐，OK。请确认 sentry.tracing.extra_tags.exception.stack_trace 的默认值与文档一致，避免意外大字段写入。
57-57: 在 finally 中 finish：好习惯无论成功或异常都能正确结束 span。
43-49: 修复?->与->链式混用导致的致命错误；同时将 error 设为布尔值当
$span为 null 时，$span?->setStatus(...)->setTags(...)会在第二段->setTags处崩溃；应拆分为两句并均使用空安全调用。同时建议error使用布尔值。- $span?->setStatus(SpanStatus::internalError()) - ->setTags([ - 'error' => 'true', - 'exception.class' => $exception::class, - 'exception.message' => $exception->getMessage(), - 'exception.code' => (string) $exception->getCode(), - ]); + $span?->setStatus(SpanStatus::internalError()); + $span?->setTags([ + 'error' => true, + 'exception.class' => $exception::class, + 'exception.message' => $exception->getMessage(), + 'exception.code' => (string) $exception->getCode(), + ]);
| use FriendsOfHyperf\Sentry\Tracing\SpanStarter; | ||
| use Hyperf\Di\Aop\ProceedingJoinPoint; | ||
| use Override; | ||
| use Sentry\Tracing\SpanContext; | ||
|
|
||
| use function Sentry\trace; | ||
| use Sentry\Tracing\SpanStatus; | ||
| use Throwable; |
There was a problem hiding this comment.
🧩 Analysis chain
引入 SpanStarter/SpanStatus/Throwable 合理；请确认 #[Override] 的 PHP 版本兼容性
改造方向对齐其它切面，OK。但请确认项目 PHP 约束≥8.3，否则使用 #[Override] 会导致运行时错误。
建议在仓库根目录执行以下脚本核对 PHP 版本要求与 Override 使用点：
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PHP requirement in composer.json:"
if command -v jq >/dev/null 2>&1; then
jq -r '.require.php // .require."php"' composer.json
else
rg -nP '"php"\s*:\s*"[^"]+"' composer.json
fi
echo -e "\nAll occurrences of #[Override]:"
rg -nP '#\[\s*Override\s*\]' -C2Length of output: 11740
根据脚本结果，项目的 PHP 版本要求是 >=8.1，但 #[Override] 属性需要 PHP 8.3 或更高版本才能使用。尽管发现代码库中已经广泛使用了 #[Override] 属性，但这与项目的 PHP 版本约束存在冲突。
让我进一步验证这个版本兼容性问题：
Based on the verification results, I can see there's a version compatibility issue that needs to be addressed.
PHP 版本约束与 #[Override] 属性不兼容
项目的 composer.json 要求 PHP >=8.1，但 #[Override] 属性需要 PHP 8.3+。虽然代码库中已广泛使用此属性，但当前配置会导致在 PHP 8.1-8.2 环境中运行时出错。
建议：
- 将 composer.json 中的 PHP 版本约束更新为
>=8.3 - 或者移除所有
#[Override]属性以保持向下兼容
🌐 Web query:
PHP Override attribute minimum version requirement
💡 Result:
The built-in #[\Override] attribute was added in PHP 8.3 — so PHP >= 8.3.0 is required for the engine to enforce it (in older PHP versions the attribute syntax is ignored). [1][2][3]
Sources:
- PHP manual: Override attribute (PHP 8 >= 8.3.0). [1]
- RFC: Marking overridden methods (#[\Override]). [2]
- PHP 8.3 migration / new features (lists Override attribute). [3]
必须修复：#[Override] 与 composer.json 中的 PHP 版本约束冲突
引入 SpanStarter/SpanStatus/Throwable 合理；但仓库根目录 composer.json 要求 PHP >=8.1，而内置属性 #[\Override]（等同 #[Override]）仅在 PHP >=8.3 可用，仓库中已多处使用该属性（例如 src/sentry/src/Tracing/Aspect/FilesystemAspect.php:25）。
- 方案 A（推荐）：将 composer.json 的 php 约束提升到 >=8.3，并同步 CI/镜像/文档。
- 方案 B：保留 php >=8.1，移除或替换仓库中所有 #[Override] 使用以恢复对 8.1/8.2 的兼容性。
🤖 Prompt for AI Agents
In src/sentry/src/Tracing/Aspect/FilesystemAspect.php around lines 15 to 19 and
usage at ~line 25, the code uses the built‑in attribute #[Override] which
requires PHP >=8.3 while the repository composer.json currently constrains PHP
to >=8.1, causing a compatibility conflict; fix by either (A, recommended)
updating composer.json php constraint to ">=8.3" and then update CI images/docs
to match, or (B) revert to PHP 8.1/8.2 compatibility by removing/replacing all
uses of the #[Override] attribute across the codebase (search & replace
occurrences and run tests) so the project no longer relies on a PHP 8.3‑only
attribute.
Summary
Changes Made
ElasticsearchAspect
?->) instead of explicit null checksFilesystemAspect
trace()function with direct span managementTest plan
Summary by CodeRabbit
Bug Fixes
Refactor