-
-
Notifications
You must be signed in to change notification settings - Fork 27
feat(sentry): add filesystem operations tracking with FilesystemAspect #915
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 FilesystemAspect class to track Flysystem adapter operations - Add filesystem breadcrumbs and tracing configuration options - Register FilesystemAspect in ConfigProvider for AOP - Enable tracking of file operations like read, write, delete, move, copy - Supports configuration via SENTRY_BREADCRUMBS_FILESYSTEM and SENTRY_TRACING_SPANS_FILESYSTEM env vars
Walkthrough新增文件系统相关的 Sentry 集成:在发布配置中加入两个开关( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as 调用方
participant FS as Flysystem 适配器
participant AOP_B as FilesystemAspect (Breadcrumbs)
participant AOP_T as FilesystemAspect (Tracing)
participant Switcher as Switcher (配置开关)
participant Sentry as Sentry (Integration / Tracing)
Caller->>FS: 调用文件系统方法()
FS-->>AOP_B: AOP 拦截(面包屑层)process()
AOP_B->>Switcher: 查询 breadcrumbs.filesystem
alt breadcrumbs 开启
AOP_B->>Sentry: addBreadcrumb(op: "file.{method}", 描述, 数据)
end
FS-->>AOP_T: AOP 拦截(追踪层)process()
AOP_T->>Switcher: 查询 tracing.spans.filesystem
alt tracing 开启
AOP_T->>Sentry: Start Span(op:"file.{method}", description, data, origin:"auto.filesystem")
AOP_T->>FS: ProceedingJoinPoint.process()(在 Span 内执行)
Sentry-->>AOP_T: Finish Span
else tracing 关闭
AOP_T->>FS: ProceedingJoinPoint.process()(直接执行)
end
FS-->>Caller: 返回结果或抛出异常
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 3
🧹 Nitpick comments (1)
src/sentry/src/Aspect/FilesystemAspect.php (1)
48-69: 读操作的面包屑量级风险;建议可配置化/采样当前同时拦截 read/readStream/listContents 等高频读操作,可能导致单事件面包屑过多(Sentry 默认上限),影响信噪比与体积。
建议:
- 默认仅记录写/删/权限变更类操作;读操作通过额外开关或采样率控制;
- 或新增白名单配置:sentry.breadcrumbs.filesystem_operations = ['write','delete',...],在 process 中判断 method 是否命中。
如果需要,我可以补一个基于配置的白名单实现示例。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/sentry/publish/sentry.php(2 hunks)src/sentry/src/Aspect/FilesystemAspect.php(1 hunks)src/sentry/src/ConfigProvider.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/sentry/src/Aspect/FilesystemAspect.php (2)
src/sentry/src/Integration.php (1)
Integration(28-158)src/sentry/src/Switcher.php (2)
Switcher(19-137)isBreadcrumbEnabled(38-41)
src/sentry/src/ConfigProvider.php (1)
src/sentry/src/Aspect/FilesystemAspect.php (1)
FilesystemAspect(20-70)
🪛 GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
src/sentry/src/Aspect/FilesystemAspect.php
[failure] 22-22:
Property FriendsOfHyperf\Sentry\Aspect\FilesystemAspect::$classes overriding property Hyperf\Di\Aop\AbstractAspect::$classes (array) should also have native type array.
🪛 GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
src/sentry/src/Aspect/FilesystemAspect.php
[failure] 22-22:
Property FriendsOfHyperf\Sentry\Aspect\FilesystemAspect::$classes overriding property Hyperf\Di\Aop\AbstractAspect::$classes (array) should also have native type array.
🪛 GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
src/sentry/src/Aspect/FilesystemAspect.php
[failure] 22-22:
Property FriendsOfHyperf\Sentry\Aspect\FilesystemAspect::$classes overriding property Hyperf\Di\Aop\AbstractAspect::$classes (array) should also have native type array.
⏰ 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). (6)
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.7
- 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
🔇 Additional comments (2)
src/sentry/publish/sentry.php (1)
66-66: 新增 breadcrumbs.filesystem 配置项:LGTM默认开启符合预期,与 Switcher::isBreadcrumbEnabled('filesystem') 对应。
src/sentry/src/ConfigProvider.php (1)
25-25: 将 FilesystemAspect 注册进 aspects:LGTM按现有开关设计由 Aspect 内部自检是否开启,符合项目惯例。
若计划保留 sentry.tracing.spans.filesystem,请确认是否也需要注册 Tracing\Aspect\FilesystemAspect(目前未见该类)。一旦实现,可在此处追加:
Tracing\Aspect\ElasticsearchAspect::class, + Tracing\Aspect\FilesystemAspect::class, Tracing\Aspect\GrpcAspect::class,
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
🧹 Nitpick comments (2)
src/sentry/src/Aspect/FilesystemAspect.php (2)
58-74: 调整描述构建逻辑以匹配实际参数结构由于 FilesystemAdapter 方法的参数结构,需要调整参数访问方式。大多数方法的第一个参数是
path,应该使用索引访问而不是关联数组键名。更新描述构建逻辑:
$description = match ($method) { - 'move', 'copy' => sprintf( - 'from "%s" to "%s"', - $arguments['source'] ?? '', - $arguments['destination'] ?? '' - ), 'write', 'writeStream', 'read', 'readStream', 'setVisibility', 'visibility', 'delete', 'deleteDirectory', 'createDirectory', 'fileExists', 'directoryExists', 'listContents', 'lastModified', 'fileSize', - 'mimeType', => $arguments['path'] ?? '', + 'mimeType' => $arguments[0] ?? '', default => '', };
75-107: 更新数据构建逻辑以使用正确的参数访问方式同样需要调整数据构建部分的参数访问方式,使用数组索引而不是关联键名来访问方法参数。
更新数据构建逻辑:
$data = match ($method) { - 'move', 'copy' => [ - 'from' => $arguments['source'] ?? '', - 'to' => $arguments['destination'] ?? '', - ], 'write', 'writeStream' => [ - 'path' => $arguments['path'] ?? '', - 'config' => $arguments['config'] ?? null, + 'path' => $arguments[0] ?? '', + 'config' => $arguments[2] ?? null, ], 'read', 'readStream' => [ - 'path' => $arguments['path'] ?? '', + 'path' => $arguments[0] ?? '', ], 'setVisibility' => [ - 'path' => $arguments['path'] ?? '', - 'visibility' => $arguments['visibility'] ?? '', + 'path' => $arguments[0] ?? '', + 'visibility' => $arguments[1] ?? '', ], 'visibility', 'delete', 'deleteDirectory' => [ - 'path' => $arguments['path'] ?? '', + 'path' => $arguments[0] ?? '', ], 'createDirectory' => [ - 'path' => $arguments['path'] ?? '', - 'config' => $arguments['config'] ?? null, + 'path' => $arguments[0] ?? '', + 'config' => $arguments[1] ?? null, ], 'fileExists', 'directoryExists', 'listContents', 'lastModified', 'fileSize', 'mimeType' => [ - 'path' => $arguments['path'] ?? '', + 'path' => $arguments[0] ?? '', ], default => [], };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sentry/src/Aspect/FilesystemAspect.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Aspect/FilesystemAspect.php (2)
src/sentry/src/Integration.php (1)
Integration(28-158)src/sentry/src/Switcher.php (2)
Switcher(19-137)isBreadcrumbEnabled(38-41)
🪛 GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
src/sentry/src/Aspect/FilesystemAspect.php
[failure] 22-22:
Property FriendsOfHyperf\Sentry\Aspect\FilesystemAspect::$classes overriding property Hyperf\Di\Aop\AbstractAspect::$classes (array) should also have native type array.
🪛 GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
src/sentry/src/Aspect/FilesystemAspect.php
[failure] 22-22:
Property FriendsOfHyperf\Sentry\Aspect\FilesystemAspect::$classes overriding property Hyperf\Di\Aop\AbstractAspect::$classes (array) should also have native type array.
🪛 GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
src/sentry/src/Aspect/FilesystemAspect.php
[failure] 22-22:
Property FriendsOfHyperf\Sentry\Aspect\FilesystemAspect::$classes overriding property Hyperf\Di\Aop\AbstractAspect::$classes (array) should also have native type array.
⏰ 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). (6)
- 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.1 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.7
🔇 Additional comments (6)
src/sentry/src/Aspect/FilesystemAspect.php (6)
43-46: 构造函数实现正确构造函数正确地注入了
Switcher依赖,使用了 PHP 8.0+ 的构造器属性推广语法,符合现代 PHP 编程实践。
109-117: 面包屑记录和方法执行逻辑正确面包屑的创建和记录方式正确,使用了适当的级别和类型。最后调用原始方法执行也是正确的做法。
22-22: 修复类型声明以匹配父类需要为
$classes属性添加array类型声明以匹配父类AbstractAspect的类型提示,否则在 PHP 8.1+ 下会导致 CI 失败。应用此修复:
- public $classes = [ + public array $classes = [
23-23: 移除不存在的适配器方法拦截根据 Flysystem v3 的 FilesystemAdapter 接口定义,该接口包含 fileExists、directoryExists、write、writeStream、read 等方法,但不包含 move、copy 和 has 方法。has 方法已被 fileExists 和 directoryExists 替代,而 move 和 copy 操作在 FilesystemOperator 层实现,其中 move 调用 copy 和 delete 方法。
从
$classes数组中移除不存在的方法:'League\Flysystem\*\*Adapter::listContents', - 'League\Flysystem\*\*Adapter::move', - 'League\Flysystem\*\*Adapter::copy',如果需要拦截 move/copy 操作,应改为拦截 FilesystemOperator 层的方法。另外第 23 行已经包含了
directoryExists,因此不需要添加has方法的拦截。Also applies to: 37-38
50-52: 确认:配置键 'filesystem' 与发布配置一致在 src/sentry/publish/sentry.php 的 breadcrumbs 下存在 'filesystem' => env('SENTRY_BREADCRUMBS_FILESYSTEM', true),且 src/sentry/src/Switcher.php::isBreadcrumbEnabled 通过 'sentry.breadcrumbs.' . $key 读取配置 — 无需修改。
55-55: 修正参数获取方式参数获取方式不正确。应该直接使用
$proceedingJoinPoint->arguments获取方法参数,而不是$proceedingJoinPoint->arguments['keys']。修复参数获取:
- $arguments = $proceedingJoinPoint->arguments['keys'] ?? []; + $arguments = $proceedingJoinPoint->arguments;Likely an incorrect or invalid review comment.
…nd extracting metadata logic
…systemAspect for clarity
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: 3
🧹 Nitpick comments (2)
src/sentry/src/Tracing/Aspect/FilesystemAspect.php (2)
55-58: 补充op规范性建议(可选)
file.{method}已可用;如需在 Sentry 中更好检索,可考虑将目录操作统一为file.dir.{method}(如file.dir.create、file.dir.delete),但非阻断。
111-118: 在 Span data 中附带适配器/实现类名(可选)为便于排查不同适配器(Local/S3/OSS)差异,建议在
data里加上实现类名,如:- return trace( - fn () => $proceedingJoinPoint->process(), + $data['class'] = $proceedingJoinPoint->className ?? null; + return trace( + static fn () => $proceedingJoinPoint->process(), SpanContext::make() ->setOp($op) ->setData($data) ->setOrigin('auto.filesystem') ->setDescription($description) );若
className不可用,可改用get_class($proceedingJoinPoint->getInstance())(按 Hyperf 版本选择)。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sentry/src/ConfigProvider.php(2 hunks)src/sentry/src/Tracing/Aspect/FilesystemAspect.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sentry/src/ConfigProvider.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Aspect/FilesystemAspect.php (1)
src/sentry/src/Switcher.php (2)
Switcher(19-137)isTracingEnabled(51-58)
⏰ 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). (6)
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.7
- 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
🔇 Additional comments (1)
src/sentry/src/Tracing/Aspect/FilesystemAspect.php (1)
49-54: 配置键路径需核对:Switcher::isTracingEnabled('filesystem')使用的 key 与发布配置是否一致?
Switcher::isTracingEnabled()读取sentry.tracing.enable.{key};PR 描述里环境变量为SENTRY_TRACING_SPANS_FILESYSTEM(spans),容易与enable路径不一致,导致开关无法生效或误判。请确认发布配置sentry.php中是否将该环境变量映射到sentry.tracing.enable.filesystem,或相应调整Switcher/配置键名以保持一致。
…n FilesystemAspect for improved flexibility
…ove and copy operations
PR #915 代码审查报告1. 变更摘要
2. 潜在风险性能风险
代码质量问题
维护性风险
3. 优化建议性能优化
配置修复
代码完善
4. 合规性检查代码风格 ✅
模块边界 ✅
文档规范 ✅
总结此 PR 为 Sentry 组件增加了有价值的文件系统监控功能,整体设计合理。主要问题集中在配置路径一致性和性能优化方面。建议在合并前解决配置问题,并考虑添加操作类型过滤功能以提升实用性。 |
#915) * feat(sentry): add filesystem operations tracking with FilesystemAspect - Add FilesystemAspect class to track Flysystem adapter operations - Add filesystem breadcrumbs and tracing configuration options - Register FilesystemAspect in ConfigProvider for AOP - Enable tracking of file operations like read, write, delete, move, copy - Supports configuration via SENTRY_BREADCRUMBS_FILESYSTEM and SENTRY_TRACING_SPANS_FILESYSTEM env vars * refactor(sentry): reorganize filesystem methods and enhance breadcrumb data structure * chore(sentry): add comment placeholder for additional adapter methods in FilesystemAspect * feat(sentry): add FilesystemAspect for tracking filesystem operations * refactor(sentry): simplify FilesystemAspect by extending base class and extracting metadata logic * refactor(sentry): update FilesystemAspect property and constructor syntax for clarity * refactor(sentry): add #[Override] attribute to process method in FilesystemAspect for clarity * refactor(sentry): add 'fileExists' method to FilesystemAspect class for completeness * refactor(sentry): add 'config' argument to move and copy operations in FilesystemAspect for improved flexibility * refactor(sentry): enhance metadata handling in FilesystemAspect for move and copy operations --------- Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Summary
Changes
New Files
src/sentry/src/Aspect/FilesystemAspect.php- New aspect class for tracking filesystem operationsModified Files
src/sentry/publish/sentry.php- Added filesystem configuration options for breadcrumbs and tracingsrc/sentry/src/ConfigProvider.php- Registered FilesystemAspect in aspects arrayFeatures
Tracked Operations
The FilesystemAspect tracks all major Flysystem adapter operations:
write,writeStreamread,readStream,listContentsdelete,move,copy,setVisibilitycreateDirectory,deleteDirectoryfileExists,directoryExists,haslastModified,fileSize,mimeType,visibilityConfiguration
Two new environment variables for fine-grained control:
SENTRY_BREADCRUMBS_FILESYSTEM- Enable/disable filesystem breadcrumbs (default: true)SENTRY_TRACING_SPANS_FILESYSTEM- Enable/disable filesystem tracing spans (default: true)Test plan
Summary by CodeRabbit