fix: improve cache key handling in CacheAspect #893
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Fix key extraction for cache operations to use correct argument structure - Update span description to properly handle array keys by imploding them - Set asParent=true for better tracing hierarchy
Walkthrough合并并集中化了 CacheAspect 的键/ttl 提取与处理流程，移除私有 handleX 方法并改为 tap(...)->process() 回调中构建并在完成时设置 origin 为 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant A as CacheAspect
participant T as Tracer
participant S as Span
Note over A: 提取 keys/ttl\n按方法映射操作\n构建描述字符串
C->>A: 调用 cache 方法 (set/get/delete/...)
A->>A: $arguments = $proceedingJoinPoint->arguments['keys'] ?? []
A->>A: 根据方法提取 [$key, $ttl]
A->>T: startSpan(description=implode(', ', (array)$key), { asParent: true })
T-->>A: 返回 Span (S)
A->>A: tap(...)->process() 执行原操作
A->>A: 完成回调：使用 $result 构建 $data
A->>S: $span->setOrigin('auto.cache')->setData($data)->finish()
S-->>C: 返回原始操作结果
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
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.17)Note: Using configuration file /phpstan.neon.dist. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/sentry/src/Tracing/Aspect/CacheAspect.php (2)
79-90: 修复 match 语法错误与多键调用类型不匹配（会导致运行时错误）
- 'set', => 语法错误（多余逗号）。
- 多键分支向签名为 array 的参数传入了 string/bool，可能引发 TypeError。
- setMultiple 分支第三参应为传入的 values（输入），deleteMultiple 分支第三参不被使用但需传入 array 以满足类型。
- return tap($proceedingJoinPoint->process(), function ($value) use ($span, $method, $key) { + return tap($proceedingJoinPoint->process(), function ($value) use ($span, $method, $key, $proceedingJoinPoint) { match ($method) { - 'set', => $this->handleSet($span, $key, $value), - 'get', 'fetch' => $this->handleGet($span, $key, $value), - 'delete' => $this->handleDelete($span, $key, $value), - 'setMultiple' => $this->handleSetMultiple($span, $key, $value), - 'getMultiple' => $this->handleGetMultiple($span, $key, $value), - 'deleteMultiple' => $this->handleDeleteMultiple($span, $key, $value), + 'set' => $this->handleSet($span, (string) $key, $value), + 'get', 'fetch' => $this->handleGet($span, (string) $key, $value), + 'delete' => $this->handleDelete($span, (string) $key, $value), + 'setMultiple' => $this->handleSetMultiple( + $span, + (array) $key, + (array) ($proceedingJoinPoint->arguments['values'] ?? []) + ), + 'getMultiple' => $this->handleGetMultiple($span, (array) $key, (array) $value), + 'deleteMultiple' => $this->handleDeleteMultiple( + $span, + (array) $key, + (array) ($proceedingJoinPoint->arguments['keys'] ?? []) + ), 'clear' => $this->handleClear($span), default => null, }; });
124-151: 多键处理器签名与调用侧值类型不一致，建议对齐签名或简化参数当前 handleSetMultiple/handleDeleteMultiple 将第二个参数类型声明为 array，但调用侧原本传入的是 $value（布尔）或单个 key，易触发 TypeError。上一个评论中的调用侧修复已确保传入 array。为了避免未来回归，建议进一步简化签名，去掉未使用的 $values 参数；或将其改为 iterable。
选项 A（简化签名，去掉未用参数）：
- private function handleSetMultiple(Span $span, array $keys, array $values) + private function handleSetMultiple(Span $span, array $keys) @@ - private function handleDeleteMultiple(Span $span, array $keys, array $values) + private function handleDeleteMultiple(Span $span, array $keys)选项 B（更宽松的类型以匹配 PSR-16 可返回 iterable 的实现）：
- private function handleGetMultiple(Span $span, array $keys, array $values) + private function handleGetMultiple(Span $span, array $keys, iterable $values)另外，建议将多键场景的数据键名从 'cache.key' 调整为 'cache.keys'，以示区分（纯可选）：
- 'cache.key' => $keys, + 'cache.keys' => $keys,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/sentry/src/Tracing/Aspect/CacheAspect.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Aspect/CacheAspect.php (1)
src/sentry/src/Tracing/SpanStarter.php (1)
startSpan(32-49)
⏰ 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.3 with Swoole 6.0.2
- 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.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
- 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 5.1.7
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/sentry/src/Tracing/Aspect/CacheAspect.php (2)
66-75: 关键：入参加载错误（setMultiple 读取 keys、漏掉 fetch、缺少位置参数回退）
- setMultiple 实际参数名为 values，而非 keys；当前实现会导致 $key 为空数组或触发未定义索引。
- 单键分支未覆盖 fetch。
- 缺少对 arguments['order'][0] 的兜底，位置参数调用会拿不到 key/keys。
建议按方法名自适配参数名，并对多键返回完整数组；setMultiple 从 values 的键导出 key 列表，同时保留位置参数回退。
- [$key, $ttl] = match ($method) { - 'set', 'get', 'delete' => [ - $proceedingJoinPoint->arguments['keys']['key'] ?? 'unknown', - $proceedingJoinPoint->arguments['keys']['ttl'] ?? null, - ], - 'setMultiple', 'getMultiple', 'deleteMultiple' => [ - $proceedingJoinPoint->arguments['keys']['keys'] ?? [], - $proceedingJoinPoint->arguments['keys']['ttl'] ?? null, - ], - default => ['', null], - }; + [$key, $ttl] = match ($method) { + // 单键：优先命名参数 key，回退到按序第一个参数 + 'set', 'get', 'fetch', 'delete' => [ + (string) ($proceedingJoinPoint->arguments['keys']['key'] + ?? ($proceedingJoinPoint->arguments['order'][0] ?? 'unknown')), + $proceedingJoinPoint->arguments['keys']['ttl'] ?? null, + ], + // 多键：get/deleteMultiple 使用 keys；无 ttl + 'getMultiple', 'deleteMultiple' => [ + (array) ($proceedingJoinPoint->arguments['keys']['keys'] + ?? ($proceedingJoinPoint->arguments['order'][0] ?? [])), + null, + ], + // setMultiple：从 values 的键导出 key 列表 + 'setMultiple' => [ + array_keys((array) ($proceedingJoinPoint->arguments['keys']['values'] + ?? ($proceedingJoinPoint->arguments['order'][0] ?? []))), + $proceedingJoinPoint->arguments['keys']['ttl'] ?? null, + ], + default => ['', null], + };
78-83: 关键：startSpan 可能返回 null，需空指针保护；description 建议截断
- startSpan 在无父 Span 时返回 null，后续 $span->... 将触发 TypeError。
- 多键直接 implode 可能导致极长 description，影响可读性与性能。
- $span = $this->startSpan( - op: $op, - description: implode(', ', (array) $key), - asParent: true - ); + $descKeys = (array) $key; + $desc = implode(', ', array_slice($descKeys, 0, 10)); + if (count($descKeys) > 10) { + $desc .= ',...'; + } + $span = $this->startSpan( + op: $op, + description: $desc, + asParent: true + ); + if (! $span) { + return $proceedingJoinPoint->process(); + }
🧹 Nitpick comments (1)
src/sentry/src/Tracing/Aspect/CacheAspect.php (1)
85-94: 命中率计算不可靠（get/getMultiple）——建议改为计数指标或移除
- get 用 is_null($result) 判断命中会误判：缓存中存的确实可能是 null。
- getMultiple 用 empty($result) 判断同样不准确（如 ['k'=>0]、['k'=>false] 会被视为空）。
建议：
- get/fetch 仅记录 key；如需命中率，需与 default 比较，但此处拿不到 default。
- getMultiple 记录请求键数量与返回项数量，避免误导。
- 'get', 'fetch' => ['cache.key' => $key, 'cache.hit' => ! is_null($result)], + 'get', 'fetch' => ['cache.key' => $key], - 'getMultiple' => ['cache.key' => $key, 'cache.hit' => ! empty($result)], + 'getMultiple' => [ + 'cache.key' => $key, + 'cache.key.count' => is_countable($key) ? count($key) : null, + 'cache.item.count' => is_countable($result) ? count($result) : null, + ],
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/sentry/src/Tracing/Aspect/CacheAspect.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Aspect/CacheAspect.php (2)
src/sentry/src/Tracing/SpanStarter.php (1)
startSpan(32-49)src/sentry/src/Tracing/Aspect/RedisAspect.php (1)
process(48-118)
⏰ 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.3 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.2 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.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 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
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 (3)
src/sentry/src/Tracing/Aspect/RpcAspect.php (1)
135-141: 严重：catch/finally 中对 $span 的未判空调用可能导致致命错误
$span来自Context::get(static::SPAN)，在未创建/未采样父 Span 时可能为 null。当前在 catch 和 finally 均直接调用$span->...，会触发 fatal。建议使用 nullsafe 运算符或显式判空，保持与同仓内 CoordinatorAspect 的处理一致。建议修改如下：
} catch (Throwable $exception) { - $span->setStatus(SpanStatus::internalError()); - $span->setTags([ + $span?->setStatus(SpanStatus::internalError()); + $span?->setTags([ 'error' => true, 'exception.class' => $exception::class, 'exception.message' => $exception->getMessage(), 'exception.code' => $exception->getCode(), ]); if ($this->switcher->isTracingExtraTagEnable('exception.stack_trace')) { $data['exception.stack_trace'] = (string) $exception; } throw $exception; } finally { - $span->setOrigin('auto.rpc')->setData($data)->finish(); + $span?->setOrigin('auto.rpc')->setData($data)->finish(); }Also applies to: 148-148
src/sentry/src/Tracing/Aspect/GrpcAspect.php (1)
61-93: finally/catch 对可能为 null 的 $span 缺少空安全防护，return 分支会在 finally 中触发致命错误
startSpan()在本项目其他位置已采用?->防护（见 CoordinatorAspect），且本方法内存在if (! $span) { return $result; }的分支。PHP 在return前仍会执行finally，当前finally与catch都直接解引用$span，一旦为 null 将导致致命错误。这是高风险路径，建议最小改动地使用空安全操作符，或在进入 try/catch 前直接早退。[建议修复 方案A（最小变更，空安全）]
} catch (Throwable $exception) { - $span->setStatus(SpanStatus::internalError()); - $span->setTags([ + $span?->setStatus(SpanStatus::internalError()); + $span?->setTags([ 'error' => true, 'exception.class' => $exception::class, 'exception.message' => $exception->getMessage(), 'exception.code' => $exception->getCode(), ]); if ($this->switcher->isTracingExtraTagEnable('exception.stack_trace')) { $data['exception.stack_trace'] = (string) $exception; } throw $exception; } finally { - $span->setOrigin('auto.grpc')->setData($data)->finish(); + $span?->setOrigin('auto.grpc')->setData($data)->finish(); }[可选 方案B（结构性早退，更稳健）]
- $span = $this->startSpan('grpc.client', $method); + $span = $this->startSpan('grpc.client', $method); + if (! $span) { + return $proceedingJoinPoint->process(); + } try { $result = $proceedingJoinPoint->process(); - - if (! $span) { - return $result; - } + // 这里不再需要二次判空早退src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php (1)
73-74: 统一设置 origin，并修复异常路径未 finish Span 的问题
- sendAsync 未设置 origin；sendBatchAsync 使用 tap 导致异常时不执行回调，Span 不会 finish，且未标记错误。
- 建议两处都改为 try/catch/finally，保证异常时设置错误状态并始终 finish，同时统一设置 origin 为 auto.kafka。
针对 sendAsync（行 73）应用：
- return tap($proceedingJoinPoint->process(), fn () => $span->finish()); + try { + $result = $proceedingJoinPoint->process(); + } catch (\Throwable $exception) { + $span->setStatus(\Sentry\SpanStatus::internalError()); + $span->setTags([ + 'error' => true, + 'exception.class' => $exception::class, + 'exception.message' => $exception->getMessage(), + 'exception.code' => $exception->getCode(), + ]); + throw $exception; + } finally { + $span->setOrigin('auto.kafka')->finish(); + } + return $result;针对 sendBatchAsync（行 99）应用：
- return tap($proceedingJoinPoint->process(), fn () => $span->setOrigin('auto.kafka')->finish()); + try { + $result = $proceedingJoinPoint->process(); + } catch (\Throwable $exception) { + $span->setStatus(\Sentry\SpanStatus::internalError()); + $span->setTags([ + 'error' => true, + 'exception.class' => $exception::class, + 'exception.message' => $exception->getMessage(), + 'exception.code' => $exception->getCode(), + ]); + throw $exception; + } finally { + $span->setOrigin('auto.kafka')->finish(); + } + return $result;Also applies to: 99-100
♻️ Duplicate comments (3)
src/sentry/src/Tracing/Aspect/CacheAspect.php (3)
81-86: 为 startSpan 增加空父 Span 保护，并截断过长的 description
- 当不存在父 Span 时 startSpan 返回 null，后续使用 $span 会致致命错误。
- 多键直接 implode 可能过长，建议只取前若干个 key 并追加省略号。
- $span = $this->startSpan( - op: $op, - description: implode(', ', (array) $key), - asParent: true - ); + $descKeys = (array) $key; + $desc = implode(', ', array_slice($descKeys, 0, 10)); + if (count($descKeys) > 10) { + $desc .= ', ...'; + } + $span = $this->startSpan( + op: $op, + description: $desc, + asParent: true + ); + if (! $span) { + return $proceedingJoinPoint->process(); + }
66-79: 缺少 fetch 的 key 提取且多处未做位置参数回退，导致描述与数据错误/缺失
- 'fetch' 未包含在单键分支，会落入 default，导致 description 为空、data 中 cache.key 为空。
- 建议对单键方法增加 fetch，并对所有分支增加到位置参数的回退（arguments['order'][0]）以增强兼容性；setMultiple 在无 values 时也应回退。
- [$key, $ttl] = match ($method) { - 'set', 'get', 'delete' => [ - $arguments['key'] ?? 'unknown', - $arguments['ttl'] ?? null, - ], + [$key, $ttl] = match ($method) { + 'set', 'get', 'fetch', 'delete' => [ + $arguments['key'] ?? ($proceedingJoinPoint->arguments['order'][0] ?? 'unknown'), + $arguments['ttl'] ?? null, + ], 'setMultiple' => [ - array_keys($arguments['values'] ?? []), + array_keys($arguments['values'] ?? ($proceedingJoinPoint->arguments['order'][0] ?? [])), $arguments['ttl'] ?? null, ], 'getMultiple', 'deleteMultiple' => [ - $arguments['keys'] ?? [], + $arguments['keys'] ?? ($proceedingJoinPoint->arguments['order'][0] ?? []), $arguments['ttl'] ?? null, ], default => ['', null], };
87-96: 避免 tap 丢失异常回调：改为 try/catch/finally，始终 finish 并标记错误
- 当前使用 tap，process() 抛异常时回调不执行，Span 不会 finish，也未标记错误，且 asParent=true 后会遗留未结束子 Span。
- 建议切换为 try/catch/finally，计算数据映射、异常分支设置状态与标签，并确保 finally 中 finish。
- return tap($proceedingJoinPoint->process(), function ($result) use ($span, $method, $key, $ttl) { - $data = match ($method) { - 'set', 'setMultiple' => ['cache.key' => $key, 'cache.ttl' => $ttl], - 'delete', 'deleteMultiple' => ['cache.key' => $key], - 'get', 'fetch' => ['cache.key' => $key, 'cache.hit' => ! is_null($result)], - 'getMultiple' => ['cache.key' => $key, 'cache.hit' => ! empty($result)], - default => [], - }; - $span->setOrigin('auto.cache')->setData($data)->finish(); - }); + $data = []; + try { + $result = $proceedingJoinPoint->process(); + $data = match ($method) { + 'set', 'setMultiple' => ['cache.key' => $key, 'cache.ttl' => $ttl], + 'delete', 'deleteMultiple' => ['cache.key' => $key], + 'get', 'fetch' => ['cache.key' => $key, 'cache.hit' => $result !== null], + 'getMultiple' => [ + 'cache.key' => $key, + 'cache.keys.count' => is_countable($key) ? count((array) $key) : 0, + 'cache.result.count' => is_countable($result) ? count((array) $result) : 0, + ], + default => [], + }; + } catch (\Throwable $exception) { + $span->setStatus(\Sentry\SpanStatus::internalError()); + $span->setTags([ + 'error' => true, + 'exception.class' => $exception::class, + 'exception.message' => $exception->getMessage(), + 'exception.code' => $exception->getCode(), + ]); + if ($this->switcher->isTracingExtraTagEnable('exception.stack_trace')) { + $data['exception.stack_trace'] = (string) $exception; + } + throw $exception; + } finally { + $span->setOrigin('auto.cache')->setData($data)->finish(); + } + return $result;
🧹 Nitpick comments (7)
src/sentry/src/Tracing/Aspect/RpcAspect.php (1)
110-115: 可选：默认记录完整 RPC 参数存在敏感信息泄露风险
$data['rpc.arguments']未受开关控制，可能包含 PII/密钥。建议与rpc.result、exception.stack_trace相同，通过开关控制或做脱敏。可参考：
- $data['rpc.arguments'] = $proceedingJoinPoint->arguments['keys']; + if ($this->switcher->isTracingExtraTagEnable('rpc.arguments')) { + $data['rpc.arguments'] = $proceedingJoinPoint->arguments['keys']; + }src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
147-147: 可选：在完成前对 headers/敏感数据做脱敏链式设置
origin没问题。但当前记录了完整的请求/响应头与可选响应体，默认可能包含 Authorization/Cookie/X-Api-Key 等敏感信息。建议在写入$data前做白/黑名单脱敏，或受开关控制。示例（放在构建
$data后）：$redact = static function (array $headers): array { foreach (['authorization','cookie','set-cookie','x-api-key'] as $h) { foreach ([$h, ucwords($h, '-')] as $k) { if (isset($headers[$k])) $headers[$k] = ['<redacted>']; } } return $headers; }; $data['http.request.headers'] = $redact($data['http.request.headers']); if (isset($data['response.headers'])) { $data['response.headers'] = $redact($data['response.headers']); }src/sentry/src/Tracing/Aspect/AsyncQueueJobMessageAspect.php (1)
91-91: 建议与全局风格保持一致：在 finally 一次性链式设置 origin 与 data 后再 finish大多数文件采用 setOrigin(...)->setData($data)->finish() 的收尾风格。此处可直接链入 setData，便于统一与减少未来改动时的遗漏风险。
- $span->setOrigin('auto.queue')->finish(); + $span->setOrigin('auto.queue')->setData($data)->finish();src/sentry/src/Tracing/Listener/TracingRedisListener.php (1)
97-99: 请确认 Redis 的 origin 命名一致性本文件使用 'auto.redis'；而同 PR 中的 Redis Aspect 似乎采用 'auto.cache.redis'。建议统一一处命名（例如全部使用 'auto.redis' 或全部使用 'auto.cache.redis'），以便后续按 origin 聚合分析。
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (1)
67-69: 拼写错误：db.elasticserach应为db.elasticsearch这会影响 Sentry 中的 op 分类检索与聚合，建议更正。
- 'db.system' => 'elasticsearch', - 'db.operation.name' => $proceedingJoinPoint->methodName, - 'http.request.method' => '', // TODO + 'db.system' => 'elasticsearch', + 'db.operation.name' => $proceedingJoinPoint->methodName, + 'http.request.method' => '', // TODO @@ - 'db.elasticserach', + 'db.elasticsearch',src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php (1)
51-54: 统一 op 命名与数据字段，提升可观测性一致性
- 目前 sendAsync 使用 op=topic.send，sendBatchAsync 使用 op=kafka.send_batch，建议统一，例如 kafka.produce 或 messaging.kafka.produce，便于检索与看板聚合。
- sendBatchAsync 未设置任何 span data，建议至少补充 messaging.system= kafka 与 messaging.operation= publish。
示例（放在各自 startSpan 之后）：
- $span = $this->startSpan( - 'kafka.send_batch', - sprintf('%s::%s', $proceedingJoinPoint->className, $proceedingJoinPoint->methodName) - ); + $span = $this->startSpan( + 'kafka.produce', + sprintf('%s::%s', $proceedingJoinPoint->className, $proceedingJoinPoint->methodName) + ); + $span && $span->setData([ + 'messaging.system' => 'kafka', + 'messaging.operation' => 'publish', + ]);Also applies to: 80-83
src/sentry/src/Tracing/Aspect/CacheAspect.php (1)
83-83: 避免原样记录敏感 key，建议做脱敏/截断或哈希缓存 key 往往包含业务标识或 PII，建议在 description 与 data 中只记录前缀、哈希或限定长度，降低泄露风险。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
src/sentry/src/Tracing/Aspect/AmqpProducerAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/AsyncQueueJobMessageAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/CacheAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/CoordinatorAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/CoroutineAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/DbAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/GrpcAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/RedisAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/RpcAspect.php(1 hunks)src/sentry/src/Tracing/Listener/TracingAmqpListener.php(1 hunks)src/sentry/src/Tracing/Listener/TracingAsyncQueueListener.php(1 hunks)src/sentry/src/Tracing/Listener/TracingCommandListener.php(1 hunks)src/sentry/src/Tracing/Listener/TracingCrontabListener.php(1 hunks)src/sentry/src/Tracing/Listener/TracingDbQueryListener.php(1 hunks)src/sentry/src/Tracing/Listener/TracingKafkaListener.php(1 hunks)src/sentry/src/Tracing/Listener/TracingRedisListener.php(1 hunks)src/sentry/src/Tracing/Listener/TracingRequestListener.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
src/sentry/src/Tracing/Listener/TracingKafkaListener.php (1)
src/sentry/class_map/SentrySdk.php (2)
SentrySdk(24-65)getCurrentHub(51-54)
src/sentry/src/Tracing/Listener/TracingRedisListener.php (1)
src/sentry/src/Switcher.php (1)
isTracingExtraTagEnable(53-56)
src/sentry/src/Tracing/Listener/TracingDbQueryListener.php (1)
src/sentry/src/Tracing/SpanStarter.php (1)
startSpan(32-49)
src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php (4)
src/sentry/src/Tracing/Aspect/CoordinatorAspect.php (1)
process(37-66)src/sentry/src/Tracing/Aspect/CoroutineAspect.php (1)
process(40-103)src/sentry/src/Tracing/Aspect/DbAspect.php (1)
process(44-129)src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
process(46-155)
src/sentry/src/Tracing/Listener/TracingCommandListener.php (1)
src/sentry/class_map/SentrySdk.php (2)
SentrySdk(24-65)getCurrentHub(51-54)
src/sentry/src/Tracing/Listener/TracingRequestListener.php (2)
src/sentry/src/Tracing/SpanStarter.php (1)
startSpan(32-49)src/sentry/class_map/SentrySdk.php (2)
SentrySdk(24-65)getCurrentHub(51-54)
src/sentry/src/Tracing/Listener/TracingAsyncQueueListener.php (1)
src/sentry/class_map/SentrySdk.php (2)
SentrySdk(24-65)getCurrentHub(51-54)
src/sentry/src/Tracing/Listener/TracingCrontabListener.php (1)
src/sentry/class_map/SentrySdk.php (2)
SentrySdk(24-65)getCurrentHub(51-54)
src/sentry/src/Tracing/Listener/TracingAmqpListener.php (1)
src/sentry/class_map/SentrySdk.php (2)
SentrySdk(24-65)getCurrentHub(51-54)
src/sentry/src/Tracing/Aspect/CacheAspect.php (2)
src/sentry/src/Tracing/SpanStarter.php (1)
startSpan(32-49)src/sentry/src/Tracing/Aspect/RedisAspect.php (1)
process(48-117)
src/sentry/src/Tracing/Aspect/AmqpProducerAspect.php (6)
src/sentry/src/Tracing/Aspect/CoordinatorAspect.php (1)
process(37-66)src/sentry/src/Tracing/Aspect/CoroutineAspect.php (1)
process(40-103)src/sentry/src/Tracing/Aspect/DbAspect.php (1)
process(44-129)src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (1)
process(59-109)src/sentry/src/Tracing/Aspect/GrpcAspect.php (1)
process(37-95)src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php (1)
process(46-155)
⏰ 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.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 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
- 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.3 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.2 with Swoole 6.0.2
🔇 Additional comments (13)
src/sentry/src/Tracing/Listener/TracingKafkaListener.php (1)
119-121: LGTM：统一链式设置 origin/data/tags将
setOrigin('auto.kafka')->setData($data)->setTags($tags)统一到一次链式调用，语义清晰，且在前置已判空/采样的情况下安全。src/sentry/src/Tracing/Aspect/AmqpProducerAspect.php (1)
78-78: LGTM：完成前设置 origin 并 finish 的链式调用与本 PR 其它 Aspect/Listener 一致化，且前置已确保
$span存在，调用安全。src/sentry/src/Tracing/Aspect/DbAspect.php (1)
125-125: LGTM：补充 origin 并合并调用合并为链式 setOrigin(...)->setData($data)->finish() 与整体改动方向一致，无功能性风险。
src/sentry/src/Tracing/Listener/TracingAsyncQueueListener.php (1)
113-113: LGTM：补充 origin 并与 setData/setTags 链式设置与其他 Listener/Aspect 的收尾一致，便于后续检索与归因。
src/sentry/src/Tracing/Listener/TracingAmqpListener.php (1)
125-125: LGTM：补充 origin 并与 setData/setTags 链式设置符合本 PR 的统一收尾模式。
src/sentry/src/Tracing/Listener/TracingRedisListener.php (1)
85-91: LGTM：异常路径下链式设置状态与标签链式写法更简洁；保持与其余文件一致。
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (1)
105-106: 统一使用 setOrigin 链式收尾，改动合理链式调用在保持语义清晰的同时减少了重复代码，且本方法在进入 try/catch 前已保障
$span非空，安全性可接受。src/sentry/src/Tracing/Aspect/CoordinatorAspect.php (1)
64-65: 使用?->并链式设置 origin/data/finish，稳妥且一致空安全操作符避免了潜在的空指针问题，且与本 PR 其它 Aspect 的风格保持一致。
src/sentry/src/Tracing/Listener/TracingCrontabListener.php (1)
100-106: 为 Transaction 设置auto.crontaborigin 并链式设置 data/tags，收尾逻辑清晰与采样检查与最终
finish(microtime(true))的顺序配合正确，保持一致性 👍src/sentry/src/Tracing/Aspect/CoroutineAspect.php (1)
96-101: 在 finally 中补充setOrigin('auto.coroutine')并在外层及时结束父 Span，流程合理
- finally 先写入 data/origin，defer 回调后置
finish()，顺序正确；父级 span 加上 origin 再结束也与“create”阶段语义匹配。- 无需额外
finish()于 finally，避免与 defer 重复。src/sentry/src/Tracing/Listener/TracingRequestListener.php (2)
129-129: 为 Transaction 设置 origin 的位置正确先设置 origin 再 setData，有助于统一来源标注，便于后续检索与分析。
134-135: 在结束子 span 前补充 origin — 一致性良好与本 PR 其它 tracing 变更保持一致，便于快速定位自动采集来源。
src/sentry/src/Tracing/Listener/TracingDbQueryListener.php (1)
107-113: 链式设置 origin/data/时间戳 — OK起止时间与数据收集保持不变，新增 origin 标注统一，符合本次 PR 的一致性目标。
| } finally { | ||
| $span->setData($data); | ||
| $span->finish(); | ||
| $span->setOrigin('auto.cache.redis')->setData($data)->finish(); |
There was a problem hiding this comment.
严重：finally 对 $span 的未判空调用可能 fatal
startSpan() 可能返回 null；即使在 try 内 if (! $span) return $result;，finally 仍会执行，现有代码会在 null 上调用方法。应使用 nullsafe。
建议：
- $span->setOrigin('auto.cache.redis')->setData($data)->finish();
+ $span?->setOrigin('auto.cache.redis')->setData($data)->finish();🤖 Prompt for AI Agents
In src/sentry/src/Tracing/Aspect/RedisAspect.php around line 113, the finally
block calls methods on $span without null-checking, which can cause a fatal if
startSpan() returned null; update the finally to use the nullsafe operator (e.g.
$span?->setOrigin(...)?->setData(...)?->finish()) or guard with an if ($span)
check so methods are only invoked when $span is not null.
Comment on lines
+117
to
+121
| $transaction | ||
| ->setOrigin('auto.command') | ||
| ->setStatus($exitCode == SymfonyCommand::SUCCESS ? SpanStatus::ok() : SpanStatus::internalError()) | ||
| ->setData($data) | ||
| ->setTags($tags); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
最终 setStatus 可能覆盖异常状态
若前面已捕获异常并 setStatus(internalError)，但 exitCode 为 SUCCESS，这里的链式 setStatus 会把状态改回 ok，导致误报。建议综合异常与退出码计算最终状态：
- $transaction
- ->setOrigin('auto.command')
- ->setStatus($exitCode == SymfonyCommand::SUCCESS ? SpanStatus::ok() : SpanStatus::internalError())
- ->setData($data)
- ->setTags($tags);
+ $finalStatus = (isset($exception) && $exception) || $exitCode !== SymfonyCommand::SUCCESS
+ ? SpanStatus::internalError()
+ : SpanStatus::ok();
+ $transaction
+ ->setOrigin('auto.command')
+ ->setStatus($finalStatus)
+ ->setData($data)
+ ->setTags($tags);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $transaction | |
| ->setOrigin('auto.command') | |
| ->setStatus($exitCode == SymfonyCommand::SUCCESS ? SpanStatus::ok() : SpanStatus::internalError()) | |
| ->setData($data) | |
| ->setTags($tags); | |
| $finalStatus = (isset($exception) && $exception) || $exitCode !== SymfonyCommand::SUCCESS | |
| ? SpanStatus::internalError() | |
| : SpanStatus::ok(); | |
| $transaction | |
| ->setOrigin('auto.command') | |
| ->setStatus($finalStatus) | |
| ->setData($data) | |
| ->setTags($tags); |
🤖 Prompt for AI Agents
In src/sentry/src/Tracing/Listener/TracingCommandListener.php around lines 117
to 121, the chained setStatus call unconditionally uses the exit code and can
overwrite a previously set internalError from a caught exception; determine a
final status first (e.g., if an exception was captured OR $exitCode !==
SymfonyCommand::SUCCESS then SpanStatus::internalError() else SpanStatus::ok())
and call setStatus exactly once with that computed status so exception-derived
errors are not lost.
Comment on lines
+165
to
+174
| $transaction->setStatus(SpanStatus::internalError()) | ||
| ->setTags([ | ||
| 'error' => true, | ||
| 'exception.class' => $exception::class, | ||
| 'exception.code' => $exception->getCode(), | ||
| 'exception.message' => $exception->getMessage(), | ||
| ]) | ||
| ->setData([ | ||
| 'exception.stack_trace' => (string) $exception, | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
直接记录完整异常栈可能泄露敏感信息，建议加开关控制
与 TracingCommandListener 中的做法保持一致，建议仅在明确开启时才写入异常栈到 data，避免默认情况下把敏感信息（token、路径、环境变量等）发往 APM。可按下列方式调整：
- $transaction->setStatus(SpanStatus::internalError())
- ->setTags([
- 'error' => true,
- 'exception.class' => $exception::class,
- 'exception.code' => $exception->getCode(),
- 'exception.message' => $exception->getMessage(),
- ])
- ->setData([
- 'exception.stack_trace' => (string) $exception,
- ]);
+ $transaction->setStatus(SpanStatus::internalError())
+ ->setTags([
+ 'error' => true,
+ 'exception.class' => $exception::class,
+ 'exception.code' => $exception->getCode(),
+ 'exception.message' => $exception->getMessage(),
+ ]);
+ if ($this->switcher->isTracingExtraTagEnable('exception.stack_trace')) {
+ $transaction->setData([
+ 'exception.stack_trace' => (string) $exception,
+ ]);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $transaction->setStatus(SpanStatus::internalError()) | |
| ->setTags([ | |
| 'error' => true, | |
| 'exception.class' => $exception::class, | |
| 'exception.code' => $exception->getCode(), | |
| 'exception.message' => $exception->getMessage(), | |
| ]) | |
| ->setData([ | |
| 'exception.stack_trace' => (string) $exception, | |
| ]); | |
| $transaction->setStatus(SpanStatus::internalError()) | |
| ->setTags([ | |
| 'error' => true, | |
| 'exception.class' => $exception::class, | |
| 'exception.code' => $exception->getCode(), | |
| 'exception.message' => $exception->getMessage(), | |
| ]); | |
| if ($this->switcher->isTracingExtraTagEnable('exception.stack_trace')) { | |
| $transaction->setData([ | |
| 'exception.stack_trace' => (string) $exception, | |
| ]); | |
| } |
🤖 Prompt for AI Agents
In src/sentry/src/Tracing/Listener/TracingRequestListener.php around lines 165
to 174, the code unconditionally writes the full exception stack trace into the
transaction data; change this to follow TracingCommandListener by gating writing
the stack trace behind a configurable flag (default disabled). Read the same
config key used by TracingCommandListener (or add a new boolean option if none
exists), and only call setData(['exception.stack_trace' => (string)$exception])
when that flag is true; otherwise omit the stack trace (or write a non-sensitive
placeholder). Ensure the flag is documented and defaults to false so stack
traces are not sent unless explicitly enabled.
xuanyanwow
approved these changes
Sep 2, 2025
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/sentry/src/Tracing/Aspect/RpcAspect.php (1)
144-144: finally 中的链式 null-safe 同样存在返回值风险，建议拆成独立调用与上方 catch 的建议一致，避免任何一个方法非 fluent 时导致整条链失效；也提升可读性。
- $span?->setOrigin('auto.rpc')->setData($data)->finish(); + $span?->setOrigin('auto.rpc'); + $span?->setData($data); + $span?->finish();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/sentry/src/Tracing/Aspect/GrpcAspect.php(1 hunks)src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php(2 hunks)src/sentry/src/Tracing/Aspect/RpcAspect.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/sentry/src/Tracing/Aspect/KafkaProducerAspect.php
- src/sentry/src/Tracing/Aspect/GrpcAspect.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Aspect/RpcAspect.php (1)
src/sentry/src/Switcher.php (1)
isTracingExtraTagEnable(53-56)
🔇 Additional comments (1)
src/sentry/src/Tracing/Aspect/RpcAspect.php (1)
131-137: 拆分 null-safe 链式调用，避免依赖返回类型；统一 Tag 值为字符串
- 拆分
$span?->setStatus()->setTags()为两句，规避非 fluent 返回类型导致的链式调用致命错误。- 将所有标签值强制转换为字符串，避免
bool/int隐式转换差异。建议改为：
- $span?->setStatus(SpanStatus::internalError()) - ->setTags([ - 'error' => true, - 'exception.class' => $exception::class, - 'exception.message' => $exception->getMessage(), - 'exception.code' => $exception->getCode(), - ]); + $span?->setStatus(SpanStatus::internalError()); + $span?->setTags([ + 'error' => 'true', + 'exception.class' => $exception::class, + 'exception.message' => $exception->getMessage(), + 'exception.code' => (string) $exception->getCode(), + ]);由于自动化搜索未能确认
setStatus()/setTags()的返回类型，请手动验证 Sentry PHP SDK 中这两个方法是否支持链式调用，并检查项目中对setTags()的其他用法。
huangdijia
added a commit
that referenced
this pull request
Sep 2, 2025
* fix: improve cache key handling in CacheAspect - Fix key extraction for cache operations to use correct argument structure - Update span description to properly handle array keys by imploding them - Set asParent=true for better tracing hierarchy * fix: refactor CacheAspect to improve key handling and reduce redundancy * fix: consolidate cache operation handling in CacheAspect for improved clarity * fix: refactor argument handling in CacheAspect for improved clarity and consistency * fix: streamline span finalization in RedisAspect for improved clarity * fix: enhance span finalization in AsyncQueueJobMessageAspect to set origin * fix: set origin for spans in various aspects for improved tracing context * fix: simplify span error handling and ensure origin is set in RpcAspect * fix: streamline span error handling in GrpcAspect for improved clarity * fix: set origin for spans in KafkaProducerAspect for improved tracing context --------- Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com>
This was referenced Sep 3, 2025
This was referenced Sep 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
新特性
重构