feat(mcp-proxy): add BKAIDev Agent Trace for MCP Gateway observability - #2665
Conversation
wklken
left a comment
There was a problem hiding this comment.
PR #2665 Code Review 汇总报告
由 主 agent (claude-sonnet-4-6) 直接 review(codex-internal 遭遇 429 限流,claude-internal 非交互模式工具调用受限,故由主 agent 代为执行并输出汇总报告)
变更概述
本 PR 为 MCP Gateway 的 mcp-proxy 组件添加 BKAIDev Agent Trace 可观测性支持,核心设计是引入一条与现有 OTEL 全局 tracing 完全隔离的独立链路追踪管道。主要变更包括:
- 新增
bkaidtrace基础设施包(pkg/infra/bkaidtrace/):独立的 OTLP/HTTP Tracer Provider,使用sync.Once保证初始化幂等性,提供Init/Enabled/StartSpan/Extract/Inject/Shutdown等接口,以及测试专用的ResetForTest/SetTestProvider。 - 新增
BkAIDevTraceMiddleware(pkg/mcp/middleware.go):MCP 方法级 span,采集 app_code、bk_username、client_ip、gateway_name、tool_name、itsm_flex 等属性。 - 新增
BkAIDevTraceContextMiddleware(pkg/middleware/bkaidtrace.go):HTTP 层中间件,从traceparentheader 提取上下文,无 header 时生成新的随机 SpanContext 作为 parent。 - 新增 ItsmFlex 解析(
pkg/middleware/mcp_header.go+pkg/util/context.go):解析X-Bkapi-ItsmFlexJSON header,提取 agent.info.code、agent.session.caller_executor 等字段注入 context。 - 配置扩展:
Config新增BkAIDevTrace字段(Enable/Endpoint/ServiceName/Token),启动时条件初始化。 - 优雅关闭:
server.go在 HTTP Server Shutdown 后调用bkaidtrace.Shutdown,确保 trace 数据刷新。 - 路由注册:用户态/应用态路由在
bkaidtrace.Enabled()为 true 时添加 HTTP 中间件。 - 测试覆盖:新增
init_test.go(293行)、middleware_test.go追加 160 行、bkaidtrace_test.go(104行)、mcp_header_test.go追加 45 行、context_test.go追加 40 行。
问题列表
🔴 Blocking
无 Blocking 级别问题。
🟠 Major
[M1] bkaidtrace.Shutdown 使用了已经取消(或即将超时)的 context
// server.go
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
if err := srv.Shutdown(ctx); err != nil {
logging.GetLogger().Fatalf("Server Shutdown: %s", err)
}
if err := bkaidtrace.Shutdown(ctx); err != nil { // ← 此时 ctx 可能已超时
logging.GetLogger().Errorf("BkAIDev trace shutdown error: %s", err)
}
// catching ctx.Done(). timeout of 5 seconds.
<-ctx.Done()srv.Shutdown(ctx) 结束时 5s context 可能剩余时间已很短甚至已超时,导致 bkaidtrace.Shutdown(ctx) 无法完整刷新 buffered spans。建议为 bkaidtrace shutdown 单独创建一个独立的 context(如 context.WithTimeout(context.Background(), 3*time.Second)),或在 Fatalf 之后 early return(当前 Fatalf 会 exit,所以实际上 Shutdown 不会被调用到,这是另一个问题)。
另外注意:当 srv.Shutdown(ctx) 返回 error 时调用 Fatalf 会直接 exit,bkaidtrace.Shutdown 永远不会被执行到。需要确认这是否是期望行为,或应改为 Errorf + continue。
[M2] NewSpanContext 在 crypto/rand 失败时的 fallback 存在种子碰撞风险
// init.go - NewSpanContext
r := mrand.New(mrand.NewSource(time.Now().UnixNano()))
binary.BigEndian.PutUint64(traceID[:8], uint64(r.Int63()))
binary.BigEndian.PutUint64(traceID[8:], uint64(r.Int63()))traceID 和 spanID 各自新建了一个独立的 mrand.New(mrand.NewSource(time.Now().UnixNano()))。在高并发场景下,两次调用时间戳相同,两个 source 种子相同,导致 r.Int63() 返回相同序列,traceID 的低 64 位可能等于 spanID。虽然 crypto/rand 极少失败,但 fallback 路径设计有缺陷,应该复用同一个 r 实例,或传入已创建的 r。
🟡 Minor
[m1] BkAIDevTraceContextMiddleware 测试中的 TraceID 大小写校验可能不稳定
Expect(spanCtx.TraceID().String()).To(Equal("11223344556677889900aabbccddeeff"))OTEL 的 TraceID.String() 返回的是小写十六进制字符串,而测试中期望值包含大写字母(如 FADb7A1c9)。这个测试可能在不同版本的 otel SDK 下失败,建议使用 strings.ToLower() 或直接使用小写 traceID 字符串。
[m2] Init 中 otlptracehttp.WithInsecure() 缺少文档说明
client := otlptracehttp.NewClient(
otlptracehttp.WithEndpoint(cfg.Endpoint),
otlptracehttp.WithInsecure(), // 强制使用 HTTP 而非 HTTPS
)WithInsecure() 意味着 Token 会以明文通过 HTTP 传输。应在注释或文档中明确说明这是设计决策(BKAIDev trace 集群在内网环境,无需 TLS),避免未来维护者误以为安全疏漏。
[m3] BkAIDevTraceMiddleware 中 span == nil 的防御判断与 StartSpan 语义不一致
ctx, span := bkaidtrace.StartSpan(ctx, spanName)
if span == nil {
return next(ctx, method, req)
}StartSpan 在 globalTracer == nil 时返回 (ctx, nil),但调用前已经有 if !bkaidtrace.Enabled() 的检查。理论上不会走到 span == nil 分支,但保留了 double-check。这是防御性编程,问题不大,但可以考虑统一:要么去掉 Enabled() 检查,要么去掉 span == nil 检查,避免逻辑冗余。
[m4] 缺少 Streamable HTTP 路由的 BkAIDevTrace 中间件注册说明
router.go 中对 SSE 路由和应用态路由都加了 BkAIDevTraceContextMiddleware,但代码注释中没有说明为什么 Streamable HTTP 路由(seeRouter.GET("/mcp"), seeRouter.POST("/mcp"))共享同一个 router group,即中间件是否也覆盖了 Streamable HTTP 路径。从代码来看确实覆盖了(同 group),但可以加注释避免歧义。
[m5] constant.go 中新增的 BkAIDevTraceContext CtxKey 未被实际使用
BkAIDevTraceContext CtxKey = "bk_ai_dev_trace_context"此常量定义了但在整个 diff 中没有找到使用处。如果是为未来预留,建议添加注释说明;如果是误提交,应删除。
💬 Nit
[n1] export.go 中 ResetForTest 直接操作包级变量,建议加 _test.go 后缀或通过 build tag 限制
ResetForTest 和 SetTestProvider 作为测试辅助函数放在 export.go 中,但该文件不带 _test.go 后缀,意味着这两个函数会被编译进生产二进制。虽然调用方只在测试里调用,但从封装角度看,可以通过 Go 的 export_test.go 惯用法来暴露内部状态给测试,同时不污染生产代码。
[n2] mcp_header_test.go 中有一行多余的空行
It("should handle missing X-Bkapi-ItsmFlex header", func() {
...
c.Request = httptest.NewRequest(http.MethodGet, "/test", nil)
mw := middleware.MCPServerHeaderMiddleware() // ← 上方有一个空行,可以去掉[n3] BkAIDevTraceContextMiddleware 函数命名与包内其他中间件风格一致,但注释中"independent BKAIDev trace propagator"表述有歧义
可以改为"BKAIDev's isolated trace propagator"更清晰。
优点
- 隔离设计清晰:通过独立的 Tracer Provider 而不是污染全局 OTEL 实例,避免与现有 tracing 互相干扰,架构决策合理。
- 测试覆盖全面:新增约 640 行测试代码,涵盖
Init、Enabled、StartSpan、Extract/Inject、NewSpanContext、Shutdown以及中间件的各种场景(成功/失败/tool_name/ItsmFlex),测试质量高。 sync.Once保证初始化幂等:防止多次调用Init导致 Tracer Provider 泄露,设计正确。- 无 traceparent 时生成随机 SpanContext:确保下游 MCP span 始终有有效的 trace parent,而不是创建多个孤立的 root span,链路连续性好。
- 错误属性采集完整:span 上同时记录了
status、latency_ms、error_code(JSONRPC error code),方便排查问题。 - 优雅关闭处理:在 server shutdown 后调用
bkaidtrace.Shutdown,确保 buffered span 不丢失(见 [M1] 有改进空间)。 - ItsmFlex 解析与 trace 属性结合:将 agent 信息(
caller_executor、agent_code)注入 span,对 AI Agent 场景的链路追踪非常有价值。
综合建议
整体实现质量较高,设计思路清晰,测试覆盖充分。主要需要关注两个 Major 问题:
- [M1]
server.go中bkaidtrace.Shutdown的 context 生命周期问题,建议使用独立 context; - [M2]
NewSpanContextfallback 路径的随机数种子碰撞问题,建议复用同一r实例。
Minor 问题中 [m1] 的测试大小写问题和 [m5] 未使用的常量需要确认是否修复。其余问题属于代码质量优化,可在后续迭代中处理。
由 主 agent (claude-sonnet-4-6) 直接 review(codex-internal 429 限流,claude-internal 非交互工具调用受限)| [from openclaw-internal]
Why this change was needed: The MCP Gateway lacked independent observability for BKAIDev Agent interactions. Operations teams needed a way to trace agent-to-tool call flows, including caller identity, latency, error codes, and upstream agent metadata (via X-Bkapi-ItsmFlex header), without interfering with the existing project-level OpenTelemetry tracing. What changed: - Added independent OTLP/HTTP trace provider in pkg/infra/bkaidtrace with its own TracerProvider, propagator, and lifecycle management, fully isolated from the project's existing tracing infrastructure - Added Gin middleware (BkAIDevTraceContextMiddleware) to extract W3C traceparent from incoming requests or generate fresh span contexts when absent - Added MCP-level middleware (BkAIDevTraceMiddleware) that creates spans per MCP method call with rich attributes: app_code, bk_username, client_ip, client_id, gateway_name, mcp_server_name, tool_name, request_id, x_request_id, trace_id, caller_executor, agent_code, latency_ms, status, and error_code - Added X-Bkapi-ItsmFlex header parsing to extract agent identity fields (agent code, agent name, caller executor, executor) - Added BkAIDevTrace config struct with enable/endpoint/token/service name fields - Added graceful shutdown of bkaidtrace provider in server shutdown - Comprehensive unit tests covering all new modules (25+ test cases) - Applied gofumpt/goimports-reviser formatting fixes to existing files Problem solved: BKAIDev Agent trace spans are now independently reported to a dedicated OTLP endpoint, enabling full observability of agent-driven MCP tool calls without coupling to or polluting the project's own tracing pipeline. Co-authored-by: claude <noreply@anthropic.com>
3851942 to
eda40cd
Compare
TencentBlueKing#2665) Why this change was needed: The MCP Gateway lacked independent observability for BKAIDev Agent interactions. Operations teams needed a way to trace agent-to-tool call flows, including caller identity, latency, error codes, and upstream agent metadata (via X-Bkapi-ItsmFlex header), without interfering with the existing project-level OpenTelemetry tracing. What changed: - Added independent OTLP/HTTP trace provider in pkg/infra/bkaidtrace with its own TracerProvider, propagator, and lifecycle management, fully isolated from the project's existing tracing infrastructure - Added Gin middleware (BkAIDevTraceContextMiddleware) to extract W3C traceparent from incoming requests or generate fresh span contexts when absent - Added MCP-level middleware (BkAIDevTraceMiddleware) that creates spans per MCP method call with rich attributes: app_code, bk_username, client_ip, client_id, gateway_name, mcp_server_name, tool_name, request_id, x_request_id, trace_id, caller_executor, agent_code, latency_ms, status, and error_code - Added X-Bkapi-ItsmFlex header parsing to extract agent identity fields (agent code, agent name, caller executor, executor) - Added BkAIDevTrace config struct with enable/endpoint/token/service name fields - Added graceful shutdown of bkaidtrace provider in server shutdown - Comprehensive unit tests covering all new modules (25+ test cases) - Applied gofumpt/goimports-reviser formatting fixes to existing files Problem solved: BKAIDev Agent trace spans are now independently reported to a dedicated OTLP endpoint, enabling full observability of agent-driven MCP tool calls without coupling to or polluting the project's own tracing pipeline. Co-authored-by: claude <noreply@anthropic.com>
What changed
pkg/infra/bkaidtracewith its own TracerProvider, propagator, and lifecycle management, fully isolated from the project's existing tracing infrastructureBkAIDevTraceContextMiddleware) to extract W3C traceparent from incoming requests or generate fresh span contexts when absentBkAIDevTraceMiddleware) that creates spans per MCP method call with rich attributes: app_code, bk_username, client_ip, client_id, gateway_name, mcp_server_name, tool_name, request_id, x_request_id, trace_id, caller_executor, agent_code, latency_ms, status, and error_codeX-Bkapi-ItsmFlexheader parsing to extract agent identity fields (agent code, agent name, caller executor, executor)BkAIDevTraceconfig struct with enable/endpoint/token/service name fieldsWhy this change was needed
The MCP Gateway lacked independent observability for BKAIDev Agent interactions. Operations teams needed a way to trace agent-to-tool call flows, including caller identity, latency, error codes, and upstream agent metadata (via X-Bkapi-ItsmFlex header), without interfering with the existing project-level OpenTelemetry tracing.
Problem solved
BKAIDev Agent trace spans are now independently reported to a dedicated OTLP endpoint, enabling full observability of agent-driven MCP tool calls without coupling to or polluting the project's own tracing pipeline.