Skip to content

Conversation

@huangdijia
Copy link
Contributor

@huangdijia huangdijia commented Nov 20, 2025

Summary

  • Add server address, port, HTTP method, and full URL tracing to Elasticsearch spans
  • Support both legacy Elasticsearch\Client and modern Elastic\Elasticsearch\Client implementations
  • Use tap() helper for cleaner result processing with side effects
  • Gracefully handle errors when extracting connection metadata

Test plan

  • Verify Elasticsearch spans include server address and port
  • Test with both Elasticsearch client versions (if available)
  • Confirm no performance regression in Elasticsearch operations
  • Ensure errors in metadata extraction don't affect query results

Summary by CodeRabbit

  • Bug 修复

    • 改进了 Elasticsearch 追踪的错误处理与容错,降低因追踪增强导致的异常风险
  • 功能改进

    • 追踪数据中不再使用占位符,更多填充了真实的服务器地址、端口和请求信息
    • 增强了对不同版本 Elasticsearch 客户端的识别与支持,提升追踪准确性和可观测性

✏️ Tip: You can customize this high-level summary in your review settings.

…erations

This enhancement adds comprehensive server connection metadata to Elasticsearch spans, including server address, port, HTTP method, and full URL. It supports both legacy Elasticsearch\Client and modern Elastic\Elasticsearch\Client implementations.

Changes:
- Extract server address, port, and request method from Elasticsearch clients
- Add support for both Elasticsearch client versions
- Use tap() helper for cleaner result processing with side effects
- Gracefully handle errors when extracting connection metadata
- Remove TODO comments as the functionality is now implemented
@coderabbitai
Copy link

coderabbitai bot commented Nov 20, 2025

Walkthrough

将 Elasticsearch 追踪切面主流程用 tap() 包裹以支持事后处理,新增对两种 Elasticsearch 客户端(v7/v8)的元数据提取路径,加入 Throwable 处理及两个受保护方法 getV7DatagetV8Data 用于构建 span 额外字段。

Changes

内聚体 / 文件(s) 变更摘要
Elasticsearch 追踪增强
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php
将主处理逻辑包装在 tap() 中;引入 Throwabletap;保留 elasticsearch.result 标签;添加基于客户端类型的防御性 try/catch 元数据提取;移除硬编码占位符并新增受保护方法 getV7Data($client): arraygetV8Data($client): array 来填充 server.addressserver.porthttp.request.methodurl.full 等字段。
配置文件路径列表调整(无功能变化)
phpstan.neon.dist
将路径项 - src/telescope-elasticsearch/*/*.php 的移除与新增位置互换,表现上等同(可能为换行或顺序调整),未改变接口或逻辑。

Sequence Diagram(s)

sequenceDiagram
    participant JP as JoinPoint
    participant Aspect as ElasticsearchAspect
    participant Client as Elasticsearch Client
    participant Transport as Transport
    participant Span as Tracing Span

    JP->>Aspect: 执行切面逻辑
    Aspect->>JP: 调用 proceedingJoinPoint.process()
    JP->>Client: 运行 Elasticsearch 操作
    Client->>Transport: 读取传输状态 / 最后请求
    Client-->>JP: 返回结果
    JP-->>Aspect: 结果回传(通过 tap 保留返回值)

    rect rgb(220, 235, 255)
        Note over Aspect,Span: 防御性元数据提取(v7 / v8)
        Aspect->>Span: 记录 elasticsearch.result
        Aspect->>Transport: 读取 server.address / server.port / request method / url
        Aspect->>Span: 设置 server.address、server.port、http.request.method、url.full
    end

    Aspect-->>JP: 返回原始结果
Loading

估计代码审查工作量

🎯 4 (复杂) | ⏱️ ~45 分钟

需重点检查:

  • getV7DatagetV8Data 的边界情况与空值处理;
  • try/catch 是否足够防御不同客户端实现或 transport 状态缺失;
  • tap() 的使用是否在所有异常/早返回路径中保持语义一致。

Possibly related PRs

Poem

🐰
弹性索引在跳跃,追踪光芒绕指尖,
元数据如萝卜香,悄悄填满每个 span,
tap 轻敲,异常低语,日志花园更鲜甜。

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 标题准确地总结了PR的主要变更:为Elasticsearch操作添加服务器地址和HTTP方法追踪。
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/elasticsearch-server-address-tracing

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2784d4b and af87d8c.

📒 Files selected for processing (2)
  • phpstan.neon.dist (1 hunks)
  • src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (3 hunks)
✅ Files skipped from review due to trivial changes (1)
  • phpstan.neon.dist
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (3)
src/sentry/src/Tracing/Tracer.php (1)
  • trace (70-93)
src/sentry/src/Function.php (1)
  • trace (48-53)
src/sentry/src/Feature.php (1)
  • isTracingTagEnabled (48-51)
🔇 Additional comments (1)
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (1)

19-22: Throwable 与 tap 引入合理,对应本次追踪增强需求

新增 use Throwable; 以及 use function Hyperf\Tappable\tap; 与下方 try/catch 和 tap 包装使用保持一致,语义清晰,没有发现问题。

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 41ce93b and 2784d4b.

📒 Files selected for processing (1)
  • src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (4)
src/sentry/src/Tracing/Tracer.php (1)
  • trace (70-93)
src/sentry/src/Function.php (2)
  • trace (48-53)
  • feature (24-27)
src/sentry/src/Tracing/Aspect/RedisAspect.php (1)
  • process (47-94)
src/sentry/src/Feature.php (1)
  • isTracingTagEnabled (48-51)
🪛 GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php

[failure] 79-79:
Call to an undefined method Elastic\Transport\Transport::getLastConnection().


[failure] 79-79:
Access to property $transport on an unknown class Elasticsearch\Client.


[failure] 78-78:
Match arm comparison between class-string&literal-string and 'Elasticsearch\Client' is always false.


[failure] 77-77:
Access to constant class on an unknown class Elasticsearch\Client.


[failure] 76-76:
PHPDoc tag @var for variable $client contains unknown class Elasticsearch\Client.

🪛 GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php

[failure] 79-79:
Call to an undefined method Elastic\Transport\Transport::getLastConnection().


[failure] 79-79:
Access to property $transport on an unknown class Elasticsearch\Client.


[failure] 78-78:
Match arm comparison between class-string&literal-string and 'Elasticsearch\Client' is always false.


[failure] 77-77:
Access to constant class on an unknown class Elasticsearch\Client.


[failure] 76-76:
PHPDoc tag @var for variable $client contains unknown class Elasticsearch\Client.

⏰ 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). (16)
  • GitHub Check: Test on PHP 8.1 with Swoole 5.1.8
  • GitHub Check: Test on PHP 8.1 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.2 with Swoole 5.1.8
  • 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.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 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.3 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.1.2
🔇 Additional comments (1)
src/sentry/src/Tracing/Aspect/ElasticsearchAspect.php (1)

65-72: 使用 tap 包裹 process() 并按特性开关记录 elasticsearch.result 的做法很合理

这里沿用了 RedisAspect 中相同的模式,通过 tap($proceedingJoinPoint->process(), ...) 在不改变返回值的前提下给 span 附加数据,并且只有在 elasticsearch.result tracing tag 开启时才编码结果写入,开销可控,结构也与现有 Redis 实现保持一致。

@huangdijia huangdijia merged commit b0d0691 into main Nov 20, 2025
22 checks passed
@huangdijia huangdijia deleted the feat/elasticsearch-server-address-tracing branch November 20, 2025 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants