-
-
Notifications
You must be signed in to change notification settings - Fork 27
refactor(sentry): remove exception message and stack trace data from tracing spans #939
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
…tracing spans - Remove exception.message and exception.stack_trace data from all tracing spans - Remove exception.stack_trace extra tag from default configuration - Simplify exception handling in tracing aspects and listeners - Remove Feature dependency from Tracer class as it's no longer needed This reduces the amount of sensitive data captured in tracing spans while maintaining the essential exception metadata (class, code) as tags.
Walkthrough本次变更统一移除了对异常 message 与 stack_trace 的数据附加与公开标记;删除了 Tracer 的 Feature 构造器与相关特性判断逻辑。异常仍打上 exception.class 与 exception.code 标签,控制流与错误状态设置保持不变。 Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Tracer
participant Span
Caller->>Tracer: trace(fn)
Tracer->>Span: startSpan()
rect rgb(235, 245, 255)
note over Tracer,Span: 正常执行路径
Tracer-->>Span: 标准标记/属性
end
alt 发生异常
Tracer-->>Span: setTag(exception.class, code)
Tracer-->>Span: setStatus(internalError), markError()
note over Tracer,Span: 不再写入 data.exception.message / stack_trace
Tracer-->>Caller: 重新抛出异常
else 正常完成
Tracer-->>Span: finish()
Tracer-->>Caller: 返回结果
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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. 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
🧹 Nitpick comments (2)
src/sentry/src/Tracing/Tracer.php (2)
90-94: 可选:仅在异常码存在时再打exception.code标签,减少噪声部分异常返回默认码
0/空字符串时,该标签价值有限。可在存在有效码时再设置,降低标签基数。- $span->setStatus(SpanStatus::internalError()) - ->setTags([ - 'error' => 'true', - 'exception.class' => $exception::class, - 'exception.code' => (string) $exception->getCode(), - ]); + $tags = [ + 'error' => 'true', + 'exception.class' => $exception::class, + ]; + $code = $exception->getCode(); + if ($code !== 0 && $code !== '' && $code !== null) { + $tags['exception.code'] = (string) $code; + } + $span->setStatus(SpanStatus::internalError())->setTags($tags);
40-41: 确认合并策略:是否希望强制覆盖/保留已有coroutine.id?当前用数组并集
['coroutine.id'=>...] + getData(),会“保留左侧、忽略右侧同名键”。若期望“以新值覆盖旧值”,应改为array_merge(getData(), ['coroutine.id'=>...])。是否有场景需要调用方自定义并覆盖该字段?请确认后选定一致策略。
Also applies to: 80-81
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/sentry/publish/sentry.php(0 hunks)src/sentry/src/Tracing/Aspect/CoroutineAspect.php(0 hunks)src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php(0 hunks)src/sentry/src/Tracing/Listener/EventHandleListener.php(0 hunks)src/sentry/src/Tracing/Tracer.php(1 hunks)
💤 Files with no reviewable changes (4)
- src/sentry/publish/sentry.php
- src/sentry/src/Tracing/Listener/EventHandleListener.php
- src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php
- src/sentry/src/Tracing/Aspect/CoroutineAspect.php
🧰 Additional context used
📓 Path-based instructions (3)
src/*/src/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use the namespace pattern FriendsOfHyperf{ComponentName} in all component PHP source files
Files:
src/sentry/src/Tracing/Tracer.php
{src,tests}/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to PSR-12 coding standards across PHP code
Files:
src/sentry/src/Tracing/Tracer.php
src/*/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
New components must follow the standard component structure under src/{component-name}/ including .gitattributes, .github, LICENSE, README.md, composer.json, and a src/ subdirectory
Files:
src/sentry/src/Tracing/Tracer.php
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Tracer.php (2)
src/sentry/src/Function.php (1)
trace(48-53)src/sentry/src/Tracing/SpanStarter.php (1)
trace(57-62)
⏰ 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). (9)
- 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.2 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
- 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 5.1.7
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
🔇 Additional comments (1)
src/sentry/src/Tracing/Tracer.php (1)
82-100: 脚本未发现对exception.message或exception.stack_trace的设置,请人工确认是否仍有遗漏。
…tracing spans (#939) - Remove exception.message and exception.stack_trace data from all tracing spans - Remove exception.stack_trace extra tag from default configuration - Simplify exception handling in tracing aspects and listeners - Remove Feature dependency from Tracer class as it's no longer needed This reduces the amount of sensitive data captured in tracing spans while maintaining the essential exception metadata (class, code) as tags. Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
Summary
Background
This change reduces the amount of potentially sensitive data captured in tracing spans while maintaining the essential exception metadata (class and code) as tags. The exception message and stack trace can contain sensitive information that shouldn't be included in tracing data.
Changes Made
exception.stack_tracefrom extra_tags in default configTest Plan
Summary by CodeRabbit