perf(mcp-proxy): optimize runtime performance and improve code quality - #2568
Conversation
Why this change was needed: The mcp-proxy service had several performance bottlenecks and code quality issues affecting production efficiency: each tool call created a new HTTP connection pool, audit logs could grow unbounded with large request/response bodies, the SSE body logger caused memory leaks, and Sentry was flooded with non-actionable client disconnect errors. What changed: - Refactored LoadMCPServer into smaller testable functions (checkNeedLoad, prefetchServerConfigs, applyServerChanges, cleanupStaleMCPServers) - Introduced shared HTTP Transport connection pool for tool calls instead of per-call allocation - Added truncateJSON for audit log body/response size limiting (10KB/100KB) - Added maskSensitiveHeaders to redact JWT tokens in logs - Filtered non-actionable errors (context.Canceled, client disconnect) from Sentry reporting via shouldReportToSentry - Removed bodyLogWriter from SSE logger middleware to prevent memory leaks - Added MetricMiddleware, SessionMetricMiddleware, TracingMiddleware for MCP SDK-level observability - Enhanced Sentry module with Recovery middleware, Flush on shutdown, and Enabled() check - Added startup duration logging for all init phases - Configured GORM log level and DB connection pool parameters - Added pprof toggle via config and basic auth credentials - Added comprehensive unit tests for all new/modified internal functions (208+ test cases across proxy, mcp, sentry packages) Problem solved: Eliminates per-request connection pool overhead, prevents unbounded log growth, stops memory leaks in SSE streaming, and reduces Sentry noise from client disconnects. The codebase is now more maintainable with well-tested, single-responsibility functions.
P1 fixes: - Make sharedTransport configurable via mcpServer.transport config - Fix Panic -> Panicf in router.go (Panic does not support format verbs) P2 fixes: - Enhance SessionMetricMiddleware comments on gauge limitations - Add semaphore to prefetchServerConfigs to limit concurrency (default 20) - Use sync/atomic.Bool for sentry enabled state (thread-safety) - Move sentry Flush to defer in server.go shutdown path P3 fixes: - Optimize checkNeedLoad tool name lookup from O(m*n) to O(m+n) via map - Add init ordering dependency comment in database/init.go - Improve bodyLogWriter removal rationale comment in middleware/logger.go
wklken
left a comment
There was a problem hiding this comment.
Code Review
变更概述:MCP-proxy 服务性能优化和代码质量改进,包括连接池复用、审计日志截断、错误过滤、内存泄漏修复等
发现的问题
-
src/mcp-proxy/pkg/infra/proxy/proxy.go:260 -
truncateJSON函数缺少错误处理,当 JSON 序列化失败时返回的错误信息可能包含敏感数据- 建议:对错误信息进行脱敏处理
-
src/mcp-proxy/pkg/infra/proxy/proxy.go:728 - SSE 连接未正确关闭,可能导致资源泄漏
- 建议:在适当的时机调用
client.CloseIdleConnections()
- 建议:在适当的时机调用
-
src/mcp-proxy/pkg/mcp/middleware.go:167 - Sentry 错误报告逻辑中,
fields变量在错误报告时未正确传递- 建议:确保所有必要的上下文信息都传递给 Sentry
优点
- 性能优化显著:引入共享 HTTP Transport 连接池,避免每次工具调用创建新连接
- 内存管理改进:添加审计日志截断功能,防止大请求/响应体导致内存增长
- 错误处理完善:过滤非关键错误(如客户端断开连接),减少 Sentry 噪音
- 代码结构清晰:将
LoadMCPServer重构为多个小函数,提高可测试性 - 监控增强:添加 Metric、SessionMetric、Tracing 中间件,提升可观测性
- 测试覆盖全面:新增 200+ 测试用例,覆盖 proxy、mcp、sentry 包
总结
这是一个高质量的优化 PR,解决了多个生产环境中的性能瓶颈和稳定性问题。代码重构合理,测试覆盖充分,性能改进效果显著。建议合并,但需要修复上述发现的几个小问题。
由 Claude 自动 review | 基于默认 Review 规范 [from openclaw-internal]
wklken
left a comment
There was a problem hiding this comment.
Code Review
变更概述:本次 PR (#2568) 主要进行了架构优化和性能改进:
- 引入日志截断 (LogTruncate) 配置,避免大对象日志导致内存和 IO 压力
- 实现共享 HTTP Transport 以复用连接池,减少连接建立开销
- 重构 MCP server 加载逻辑,采用并发预取 + 串行应用的两阶段模式
- 统一用户态和应用态的 MCPProxy 实例,消除重复加载
- 优化 Sentry 错误过滤,过滤客户端错误 (-32700, -32600, -32601, -32602)
- 移除 bodyLogWriter 解决 SSE 长连接场景的内存泄漏
- 新增数据库日志输出
- 依赖清理:移除 raven-go,全面迁移至 sentry-go
发现的问题
严重问题
-
sharedTransport 线程安全风险 (src/mcp-proxy/pkg/infra/proxy/proxy.go:1227-1245)
- 问题:
sharedTransport是全局包级变量,InitSharedTransport通过直接赋值修改,没有任何并发保护 - 风险:虽然注释说明"必须在启动前调用一次",但如果启动顺序发生变化或存在并发初始化场景,可能导致 data race
- 建议:使用
sync.Once确保只初始化一次,或在启动初始化阶段通过 panic 检测重复调用
- 问题:
-
Sentry 过滤逻辑不完整 (src/mcp-proxy/pkg/mcp/mcp.go:3664-3680)
- 问题:
shouldReportToSentry只检查jsonrpc.Error类型,对于嵌套 error 的处理使用errors.As是正确的,但未覆盖所有可能的错误类型 - 缺失:对于非 JSON-RPC 错误(如 HTTP 客户端错误、数据库错误等)会全部上报,可能产生大量噪音
- 建议:考虑基于 error 类型的白名单或黑名单策略,例如过滤
*net.OpError、context.Cancelled 等
- 问题:
中等问题
-
LogTruncate 默认值在两处定义 (src/mcp-proxy/pkg/config/config.go:154-166, 323-366)
- 问题:
LogTruncate结构体字段默认值在Load函数中硬编码,而 getter 方法中也定义了常量 - 维护风险:两处需要保持同步,容易导致不一致
- 建议:统一使用常量,
Load函数中引用常量而非硬编码
- 问题:
-
移除
defer client.CloseIdleConnections()可能导致连接资源占用 (src/mcp-proxy/pkg/infra/proxy/proxy.go:1440)- 问题:移除了
client.CloseIdleConnections()的 defer 调用 - 风险:虽然引入了共享 Transport,但
logTransport内部仍可能持有资源引用,长时间运行可能导致资源累积 - 建议:评估 logTransport 是否需要清理,或在 MCPProxy 层面增加定期连接池清理机制
- 问题:移除了
-
配置项
maxConcurrentPrefetch无上限检查 (src/mcp-proxy/pkg/config/config.go:341)- 问题:
MaxConcurrentPrefetch默认值为 20,但没有设置上限 - 风险:如果用户设置过大的值(如 1000),可能耗尽数据库连接池或导致 goroutine 爆炸
- 建议:添加合理性校验,如限制在 1-100 范围内
- 问题:
轻微问题
-
配置文件 template 缺少注释说明 TLS 安全风险 (src/mcp-proxy/config.yaml.tpl:87)
- 问题:
insecureSkipVerify: true的默认值存在安全风险,但注释说明不够突出 - 建议:明确标注生产环境必须设置为 false,或改默认值为 false
- 问题:
-
测试覆盖率:新增的 benchmark 测试很全面,但对一些边界情况(如并发 cleanupStaleMCPServers)的单元测试可以进一步加强
-
命名一致性:
mcpInitStart等变量命名使用了驼峰式,但其他地方多用下划线(如initStart),建议统一风格 -
日志消息格式:
duration=%s的格式在多处使用,建议统一为duration=%v或使用专门的日志字段 -
数据库 nil 检查注释多余 (src/mcp-proxy/pkg/infra/database/init.go:565-567)
- 如果
config.G确实保证非 nil,则 nil 检查和冗长注释可以移除,保持代码简洁
- 如果
优点
- 架构设计清晰:MCP server 加载采用"并发预取 + 串行应用"两阶段模式,既提升了性能又保证了线程安全
- 性能优化显著:共享 Transport 和并发预取预计能显著降低内存和 CPU 开销
- Sentry 优化合理:过滤客户端错误可以有效减少 Sentry 报警噪音
- 内存泄漏修复彻底:移除 bodyLogWriter 并配合 MCP 层日志中间件,从根源解决了 SSE 场景的内存泄漏
- 测试覆盖充分:新增的单元测试、benchmark 测试覆盖全面,且通过 export_test 暴露内部函数进行测试
- 代码结构改进:函数职责更加清晰,CleanupAll/CleanupStale 等 API 设计合理
- 依赖清理到位:移除 raven-go 统一使用 sentry-go,减少了依赖复杂度
- 启动耗时监控:各组件初始化增加耗时日志,便于性能观察和诊断
总结
总体而言,这是一个高质量的性能优化 PR,解决了多个实际痛点(内存泄漏、性能开销、监控噪音)。主要架构决策合理,测试覆盖充分。
需要关注的核心风险是 sharedTransport 的并发安全性,建议通过 sync.Once 增强保护。其他问题多为优化建议或边界情况加强,不影响核心功能的正确性。
建议合并,但优先处理 sharedTransport 线程安全问题和 Sentry 过滤逻辑的补充。
由 Claude 自动 review | 基于项目规范
…tch cleanup - Add LogTruncate config with safe-default Getters, replacing hardcoded audit/API log truncation limits with configurable values - Remove unused output schema code and StructuredContent assignment - Add batch cleanup methods (CleanupAll/CleanupStale) to reduce lock contention - Improve truncateJSON fallback and add prefetch error logging - Add tests: LogTruncate Getters, Load defaults, InitSharedTransport, CleanupAll/CleanupStale
ec47a06 to
3a7dddc
Compare
|
另外:新的变更及约定(打日志),看下是否有新的内容需要更新到 AGENTS.md |
|
另外: ai review report 仔细看下 |
- Rename MaxBodySize/MaxResponseSize to AuditLogMaxBodySize/AuditLogMaxResponseSize for clarity - Move maskSensitiveHeaders to pkg/util/mask.go for reusability - Protect sharedTransport initialization with sync.Once - Replace hardcoded LogTruncate defaults with named constants - Add upper bound check (cap at 100) for MaxConcurrentPrefetch - Update AGENTS.md with new pkg/util docs and config conventions
- Export truncateJSON as util.TruncateJSON for reusability - Move truncateJSON tests to pkg/util/mask_test.go - Move maskSensitiveHeaders tests to pkg/util/mask_test.go - Update proxy.go to use util.TruncateJSON
|
review again |
TencentBlueKing#2568) * perf(mcp-proxy): optimize runtime performance and improve code quality Why this change was needed: The mcp-proxy service had several performance bottlenecks and code quality issues affecting production efficiency: each tool call created a new HTTP connection pool, audit logs could grow unbounded with large request/response bodies, the SSE body logger caused memory leaks, and Sentry was flooded with non-actionable client disconnect errors. What changed: - Refactored LoadMCPServer into smaller testable functions (checkNeedLoad, prefetchServerConfigs, applyServerChanges, cleanupStaleMCPServers) - Introduced shared HTTP Transport connection pool for tool calls instead of per-call allocation - Added truncateJSON for audit log body/response size limiting (10KB/100KB) - Added maskSensitiveHeaders to redact JWT tokens in logs - Filtered non-actionable errors (context.Canceled, client disconnect) from Sentry reporting via shouldReportToSentry - Removed bodyLogWriter from SSE logger middleware to prevent memory leaks - Added MetricMiddleware, SessionMetricMiddleware, TracingMiddleware for MCP SDK-level observability - Enhanced Sentry module with Recovery middleware, Flush on shutdown, and Enabled() check - Added startup duration logging for all init phases - Configured GORM log level and DB connection pool parameters - Added pprof toggle via config and basic auth credentials - Added comprehensive unit tests for all new/modified internal functions (208+ test cases across proxy, mcp, sentry packages) Problem solved: Eliminates per-request connection pool overhead, prevents unbounded log growth, stops memory leaks in SSE streaming, and reduces Sentry noise from client disconnects. The codebase is now more maintainable with well-tested, single-responsibility functions. * fix(mcp-proxy): address code review findings (P1/P2/P3) P1 fixes: - Make sharedTransport configurable via mcpServer.transport config - Fix Panic -> Panicf in router.go (Panic does not support format verbs) P2 fixes: - Enhance SessionMetricMiddleware comments on gauge limitations - Add semaphore to prefetchServerConfigs to limit concurrency (default 20) - Use sync/atomic.Bool for sentry enabled state (thread-safety) - Move sentry Flush to defer in server.go shutdown path P3 fixes: - Optimize checkNeedLoad tool name lookup from O(m*n) to O(m+n) via map - Add init ordering dependency comment in database/init.go - Improve bodyLogWriter removal rationale comment in middleware/logger.go * fix(mcp-proxy): opt mcp schema * fix(mcp-proxy): configurable log truncation, remove output schema, batch cleanup - Add LogTruncate config with safe-default Getters, replacing hardcoded audit/API log truncation limits with configurable values - Remove unused output schema code and StructuredContent assignment - Add batch cleanup methods (CleanupAll/CleanupStale) to reduce lock contention - Improve truncateJSON fallback and add prefetch error logging - Add tests: LogTruncate Getters, Load defaults, InitSharedTransport, CleanupAll/CleanupStale * fix: address PR review comments for mcp-proxy optimization - Rename MaxBodySize/MaxResponseSize to AuditLogMaxBodySize/AuditLogMaxResponseSize for clarity - Move maskSensitiveHeaders to pkg/util/mask.go for reusability - Protect sharedTransport initialization with sync.Once - Replace hardcoded LogTruncate defaults with named constants - Add upper bound check (cap at 100) for MaxConcurrentPrefetch - Update AGENTS.md with new pkg/util docs and config conventions * refactor: move truncateJSON to pkg/util as TruncateJSON - Export truncateJSON as util.TruncateJSON for reusability - Move truncateJSON tests to pkg/util/mask_test.go - Move maskSensitiveHeaders tests to pkg/util/mask_test.go - Update proxy.go to use util.TruncateJSON
Why this change was needed:
The mcp-proxy service had several performance bottlenecks and code quality issues affecting production efficiency: each tool call created a new HTTP connection pool, audit logs could grow unbounded with large request/response bodies, the SSE body logger caused memory leaks, and Sentry was flooded with non-actionable client disconnect errors.
What changed:
Problem solved:
Eliminates per-request connection pool overhead, prevents unbounded log growth, stops memory leaks in SSE streaming, and reduces Sentry noise from client disconnects. The codebase is now more maintainable with well-tested, single-responsibility functions.
Description
Fixes # (issue)
Checklist