feat(ai-proxy): support first byte timeout for llm streaming requests #2652
Conversation
|
|
添加LLM流式请求的首包超时支持变更概述新功能
依赖更新
配置调整
变更文件
时序图sequenceDiagram
participant PC as ProviderConfig
PC->>PC: handleRequestBody()
PC->>PC: isStreamingAPI()
alt 当isStreamingAPI返回true且firstByteTimeout不为0时
PC->>Envoy: 设置x-envoy-upstream-rq-first-byte-timeout-ms头
opt 设置失败时
PC->>Log: 记录错误日志
end
else 否则
PC->>PC: 继续原有处理流程
end
💡 小贴士与 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 | 0 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 0 | 次要问题，酬情优化。例如：代码格式不规范或注释缺失。 |
总计: 1 个问题
📋 评审意见详情
💡 代码实现建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
🔹 plugins/wasm-go/extensions/ai-proxy/provider/provider.go (1 💬)
🚀 架构设计建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍1. 配置项验证缺失导致潜在数值溢出风险
新增的firstByteTimeout配置项使用uint32类型，但未在配置解析时进行数值范围校验。当用户配置超过uint32最大值（4294967295）时会导致数值截断，可能引发超时时间不符合预期的系统性问题。建议在ProviderConfig.FromJson方法中添加数值范围校验逻辑，确保配置值在0到4294967295之间。
📌 关键代码
c.firstByteTimeout = uint32(json.Get("firstByteTimeout").Uint())配置值超过uint32最大值时导致数值截断，实际生效的超时时间可能与配置值不符，引发不可预期的请求超时行为
🔍2. 流式API判断逻辑存在硬编码扩展性问题
isStreamingAPI函数通过硬编码方式列举所有流式API名称，当新增流式API时需要修改该函数，违反开放封闭原则。建议将流式API列表抽离为可配置的常量集合或注册表，便于扩展和维护。当前实现中已包含9种API名称，后续扩展将导致频繁修改核心逻辑。
📌 关键代码
switch apiName {
case ApiNameCompletion,
ApiNameChatCompletion,
ApiNameImageGeneration,
...// 其他8个枚举值
}新增流式API时需修改核心判断逻辑，增加维护成本并可能引入遗漏配置的风险
🔍3. 跨配置项逻辑冲突风险
现有配置包含timeout和firstByteTimeout两个超时配置，但未明确两者的作用域关系。需确认这两个超时配置是否会产生逻辑冲突（如总超时与首包超时的叠加效应），建议在配置文档中明确说明两者的协同工作机制及优先级规则。
📌 关键代码
// 现有配置字段
timeout uint32 `...`
firstByteTimeout uint32 `...`未定义的超时配置交互可能导致请求处理逻辑不符合预期，引发不可预测的超时行为
🔍4. 缺少配置变更热加载验证
新增配置项firstByteTimeout未验证动态配置更新时的行为。需要确保当通过热加载更新该配置值时，能正确应用到后续请求处理中，避免配置更新后出现状态不一致的情况。
动态配置更新时可能无法及时生效，导致配置变更无法按预期生效
🔍5. 日志输出包含敏感配置信息
在841行直接输出firstByteTimeout数值到调试日志，可能在日志收集系统中暴露配置信息。建议对敏感配置值进行脱敏处理后再记录日志。
📌 关键代码
log.Debugf("[firstByteTimeout] %d", c.firstByteTimeout)调试日志可能包含配置数值，存在信息泄露风险
审查详情
📒 文件清单 （1 个文件）
📝 变更: 1 个文件
📝 变更文件:
plugins/wasm-go/extensions/ai-proxy/provider/provider.go
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@lingma-agents 分析这个方法的性能瓶颈并提供优化建议。
-
@lingma-agents 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@lingma-agents 请总结上述讨论并提出解决方案。
-
@lingma-agents 请根据讨论内容生成优化代码。
| c.timeout = defaultTimeout | ||
| } | ||
| // first byte timeout | ||
| c.firstByteTimeout = uint32(json.Get("firstByteTimeout").Uint()) |
There was a problem hiding this comment.
为firstByteTimeout字段添加数值范围验证，防止配置值超出uint32最大值导致截断
🟠 Critical | 🐞 Bugs
📋 问题详情
当JSON配置中firstByteTimeout的值超过uint32最大值（4294967295）时，直接转换会导致数值截断为0。这可能使配置失效并引发隐蔽的逻辑错误，例如当用户配置4294967296时会被错误地解析为0，导致首包超时未生效。
💡 解决方案
当用户配置的firstByteTimeout值超过uint32最大值时，需主动截断并记录警告：
-c.firstByteTimeout = uint32(json.Get("firstByteTimeout").Uint())
+value := json.Get("firstByteTimeout").Uint()
+maxUint32 := ^uint32(0)
+if value > uint64(maxUint32) {
+ log.Warnf("firstByteTimeout value %d exceeds maximum allowed %d, using %d", value, maxUint32, maxUint32)
+ value = uint64(maxUint32)
+}
+c.firstByteTimeout = uint32(value)您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
OxalisCu
force-pushed
the
first-byte-timeout
branch
from
d2aa345 to
5cacc15
Compare
Signed-off-by: OxalisCu <[email protected]>
OxalisCu
force-pushed
the
first-byte-timeout
branch
from
5cacc15 to
0e96101
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2652 +/- ##
===========================================
+ Coverage 35.91% 46.05% +10.14%
===========================================
Files 69 81 +12
Lines 11576 13020 +1444
===========================================
+ Hits 4157 5996 +1839
+ Misses 7104 6679 -425
- Partials 315 345 +30 🚀 New features to boost your workflow:
|
| ) (types.Action, error) { | ||
| // add the first byte timeout header to the request | ||
| if c.firstByteTimeout != 0 && c.isStreamingAPI(apiName, body) { | ||
| err := proxywasm.ReplaceHttpRequestHeader("x-envoy-upstream-rq-first-byte-timeout-ms", strconv.FormatUint(uint64(c.firstByteTimeout), 10)) |
There was a problem hiding this comment.
@OxalisCu Envoy 支持使用 x-envoy-upstream-rq-first-byte-timeout-ms 这个 header 来控制首包超时吗？我在文档里好像没看到这个参数：https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/router_filter#http-headers-consumed-from-downstreams
There was a problem hiding this comment.
…alibaba#2652) Signed-off-by: OxalisCu <[email protected]>
5 participants
support first byte timeout for llm streaming requests