Skip to content

feat: support sync mode for tracemap#11593

Merged
taloric merged 2 commits intomainfrom
support_api_sync_call
Apr 10, 2026
Merged

feat: support sync mode for tracemap#11593
taloric merged 2 commits intomainfrom
support_api_sync_call

Conversation

@taloric
Copy link
Copy Markdown
Contributor

@taloric taloric commented Apr 8, 2026

This PR is for:

  • Server

@taloric taloric force-pushed the support_api_sync_call branch 3 times, most recently from e7a2cce to f165c37 Compare April 9, 2026 01:59
@taloric taloric marked this pull request as ready for review April 9, 2026 06:28
@taloric
Copy link
Copy Markdown
Contributor Author

taloric commented Apr 9, 2026

@claude review 这个 PR,用中文

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 9, 2026

Claude finished @taloric's task in 11m 3s —— View job


进度

  • 初始化评审清单并更新评论
  • 收集 PR diff 和相关文件上下文
  • 分析改动并形成中文评审意见
  • 更新 GitHub 评论并提交最终评审结果

Review

我看完这 3 个文件后,没有发现会导致功能错误的明显问题。这个 PR 的核心变化比较小,主要是把原来的 TraceMap 拆成了同步/异步两个入口,并新增了 /v1/trace_map/sync 路由,整体实现和当前占位式 service 代码是一致的。

有一个小的可维护性问题,建议顺手修一下:

  • server/querier/app/distributed_tracing/router/tracemap.go:52
    这里的注释写的是“support synchronous mode by api params”,但当前实现并不是通过 API 参数切换,而是通过新增 /v1/trace_map/sync 路由,并在 traceMap(..., synchronous bool) 里分支处理。注释和代码行为不一致,后续读代码的人容易误解。
    Fix this →

总结

  • 结论:整体 LGTM,建议修正文案注释后合并。
  • 未执行测试:这次是评审任务,没有做代码修改,也没有跑测试。

@taloric taloric requested a review from lzf575 April 9, 2026 06:30
@taloric taloric enabled auto-merge (rebase) April 9, 2026 06:30
@taloric taloric force-pushed the support_api_sync_call branch from f165c37 to 4d8f247 Compare April 10, 2026 03:19
@taloric taloric requested a review from kylewanginchina April 10, 2026 06:43
@taloric taloric merged commit c4becf4 into main Apr 10, 2026
9 checks passed
@taloric taloric deleted the support_api_sync_call branch April 10, 2026 06:46
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