[feature] extend wasm abi, includes: injectEncodedDataToFilterChain for handle stream response body, getUpstreamHosts & setUpstreamOverrideHost for self-defined load balance policy #8
Conversation
扩展Wasm ABI以支持健康检查指标存储和上游主机操作变更文件
时序图Sequence diagram
participant Context as WasmContext
participant Host as UpstreamHost
participant HealthChecker as HealthCheckerImpl
participant Test as TestFixture
Context->>HealthChecker: getUpstreamHosts()
HealthChecker->>Host: collectEndpointMetrics()
Host-->>HealthChecker: return metrics data
HealthChecker->>Context: return JSON formatted host info
Note right of HealthChecker: Metrics storage flow:
HealthChecker->>Host: store_metrics enabled?
if yes
HealthChecker->>Host: setEndpointMetrics(response_body)
else
deactivate
endif
Test->>HealthChecker: setupLLMServiceWithExpectedResponseHC()
Test->>HealthChecker: respondBody("200", "Everything OK")
Test->>Host: verify coarseHealth() == Healthy
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
![lingma-agents[bot]](https://avatars.githubusercontent.com/in/1198481?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 1 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 1 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 0 | 次要问题，酌情优化。例如：代码格式不规范或注释缺失。 |
总计： 2 个问题
📋 评审意见详情
💡 单文件建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
📄 source/extensions/health_checkers/http/health_checker_impl.cc (1 💬)
- #if预处理块逻辑不完整 (L217-L222)
📄 test/common/upstream/health_checker_impl_test.cc (1 💬)
- 测试用例未验证核心存储逻辑 (L554-L568)
🚀 跨文件建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍 1. store_metrics字段在多个proto版本中重复定义且缺乏版本协调
在api/v2/core/health_check.proto和api/config/core/v3/health_check.proto两个不同版本的健康检查配置中都新增了store_metrics字段，且tag号均为127。这可能导致版本兼容性问题：1. 不同版本proto的字段定义未同步到所有相关组件 2. 后端处理逻辑可能无法正确识别不同版本配置中的字段。建议统一版本策略，确保新功能字段在API版本演进中得到协调处理。
📌 关键代码：
bool store_metrics = 127;
bool store_metrics = 127;
🔍 2. proxy-wasm-cpp-host依赖版本突变未验证兼容性
在bazel/repository_locations.bzl中，proxy-wasm-cpp-host依赖的版本从71f6a9e突然变更为27df305，但未在提交说明中说明变更原因。此版本跳跃可能引入：1. 与现有WASM宿主实现的兼容性问题 2. 新的安全漏洞 3. 构建系统不兼容。建议补充版本变更说明，并验证新版本与Envoy当前架构的兼容性
📌 关键代码：
version = "27df3052072dd97527945c4167ef912020a0a92e"
🔍 3. 端点指标存储存在竞态条件风险
在source/common/upstream/upstream_impl.h中，HostImpl的endpoint_metrics_ptr_使用atomic指针管理，但setEndpointMetrics方法中存在状态切换逻辑(set_backup_变量)。这种手动管理指针的方案可能导致：1. 内存泄漏(未释放旧字符串) 2. 线程可见性问题。建议改用智能指针或原子字符串类型实现线程安全存储
📌 关键代码：
std::atomic<std::string*> endpoint_metrics_ptr_;
🔍 4. HIGRESS条件编译块导致代码分散维护困难
大量核心功能(如Context::getUpstreamHosts)被包裹在HIGRESS预处理宏中，这将：1. 降低代码可读性 2. 增加维护成本 3. 可能导致功能模块隔离不足。建议将扩展功能封装为可插拔模块，或通过编译选项统一管理特性开关
📌 关键代码：
#if defined(HIGRESS)
🔍 5. 健康检查指标存储未实现清理机制
新增的endpoint_metrics存储功能未包含数据清理策略，可能导致：1. 内存持续增长 2. 过期指标数据未及时清除。建议添加指标数据生命周期管理策略，如设置最大存储时间或大小限制
🔍 6. JSON序列化未处理编码异常
在source/extensions/common/wasm/context.cc的getUpstreamHosts方法中，nlohmann::json::dump操作未捕获可能的异常(如内存不足)。当前仅捕获标准异常，但实现中可能存在其他异常未处理
📌 关键代码：
nlohmann::json j = info_map;
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@Lingma-Agent 分析这个方法的性能瓶颈并提供优化建议。
-
@Lingma-Agent 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@Lingma-Agent 请总结上述讨论并提出解决方案。
-
@Lingma-Agent 请根据讨论内容生成优化代码。
| if (parent_.store_metrics_) { | ||
| response_body_->move(data, data.length()); | ||
| } else if (parent_.response_buffer_size_ != 0) { | ||
| #else | ||
| if (parent_.response_buffer_size_ != 0) { | ||
| #endif |
There was a problem hiding this comment.
#if预处理块逻辑不完整
🟠 Critical | 🐞 Bugs
📋 问题详情
#if defined(HIGRESS)块未完全包裹条件代码，导致response_body_->move存在执行路径覆盖漏洞。
💡 解决方案
应修正为：
+#if defined(HIGRESS)
+if (parent_.store_metrics_ || parent_.response_buffer_size_ != 0) {
+#else
+if (parent_.response_buffer_size_ != 0) {
+#endif🔧 建议代码
‼️ AI 生成代码 - 请在应用前检查逻辑、规范并测试
| if (parent_.store_metrics_) { | |
| response_body_->move(data, data.length()); | |
| } else if (parent_.response_buffer_size_ != 0) { | |
| #else | |
| if (parent_.response_buffer_size_ != 0) { | |
| #endif | |
| if (parent_.store_metrics_ || parent_.response_buffer_size_ != 0) { | |
| // 合并条件判断逻辑 | |
| } | |
您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
| void setupLLMServiceWithExpectedResponseHC() { | ||
| // Response: Base64 string of "Everything OK". | ||
| std::string yaml = R"EOF( | ||
| timeout: 1s | ||
| interval: 1s | ||
| interval_jitter: 1s | ||
| unhealthy_threshold: 2 | ||
| healthy_threshold: 2 | ||
| store_metrics: true | ||
| http_health_check: | ||
| path: /metrics | ||
| )EOF"; | ||
| allocHealthChecker(yaml); | ||
| addCompletionCallback(); | ||
| } |
There was a problem hiding this comment.
测试用例未验证核心存储逻辑
🟡 Major | 🧹 Code Smells
📋 问题详情
新增的LLMServiceHealthCheckSuccess测试未验证setEndpointMetrics实际存储的值，无法确保功能正确性。
💡 解决方案
需补充：
+ auto host = cluster_->prioritySet().getMockHostSet(0)->hosts_[0];
+ EXPECT_EQ("Test Everything OK", host->getEndpointMetrics());您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
rinfx
changed the title
support wasm abi:
injectEncodedDataToFilterChain: inject encoded data while handling response bodygetUpstreamHosts: get upstream host informations, such as address, metrics, health_status, etc.setUpstreamOverrideHost: override upstream host, used for self-defined load balance policy