feat(trace): end-to-end OpenTelemetry request tracing - #1341
Conversation
LLLLKKKK
left a comment
There was a problem hiding this comment.
AI Code Review - PR #1341
Status: BLOCKING
Summary: P0/0 · P1/1 · P2/16 · P3/15
Reviewed: commit 3fc322e290af · 2026-08-27 18:57 UTC+8
Blocking Issues
P1
- 遥测关闭时仍创建结算 task,把全量流式请求的 RPC 取消推迟最多 5.1 秒且无回滚开关 @
rtp_llm/cpp/model_rpc/model_rpc_client.py:966- 建议:把「finished 帧后延迟取消」与遥测解耦并提供回滚手段:
create_task与normal_finish_seen赋值加client_span is not None前置条件,should_cancel的抑制条件改为client_settlement_task is None(语义:只有 settle task 接管了 iterator 才不在 finally 取消),使遥测开关同时成为该新行为的开关、无遥测时精确回到改动前语义。若该「不让物理流无限存活」的约束确实需要覆盖全量流量,请拆为独立提交、把RPC_SETTLE_TIMEOUT_SECONDS/RPC_CLEANUP_TIMEOUT_SECONDS提为可配置项,并补一条真实 grpc.aio 用例:servicer 在 finished 帧后延迟关流、调用方仍在async for,断言请求不被自身定时取消。同时为 task 保留句柄,在finally中对未完成者显式cancel()(span 由 guard 兜底结算),避免请求结束后仍有游离 task 持有response_iterator。
- 建议:把「finished 帧后延迟取消」与遥测解耦并提供回滚手段:
Non-blocking Suggestions
P2
- opentelemetry 可用性契约自相矛盾,两个 py_test 在公共 lock 下确定性失败而非跳过 @
rtp_llm/cpp/model_rpc/test/model_rpc_client_test.py:735- 建议:明确取其一并使四处注释自洽:(1)若公共 lock 确实不带 opentelemetry,则给
ModelRpcClientGrpcMetadataTest与TestDependencyContract加上与其他三处一致的skipUnless(tracing.OTEL_AVAILABLE, ...),并把test_tracing.py的 docstring 改为与arch_select.bzl一致;(2)若这两个 target 必须在所有 lock 上验证真实链路,则让公共telemetry_test_deps()真正声明 opentelemetry,并同步修正arch_select.bzl:186与rtp_llm/BUILD:453-455的「保持绿色」表述。无论哪种,都请把server.start()/insecure_channel()之后的清理纳入try/finally或contextlib.AsyncExitStack,保证任何提前失败都能停 server、关 channel 并复位 telemetry。
- 建议:明确取其一并使四处注释自洽:(1)若公共 lock 确实不带 opentelemetry,则给
- returned_sequence_count 三元两支求值恒等,注释描述的区分不存在且与 Python 口径分叉 @
rtp_llm/cpp/model_rpc/LocalRpcServer.cc:236- 建议:直接写成
std::max(stream->numReturnSequences(), 1)并删除三元表达式,同步修正注释只保留「beam 宽度不计入对外 usage、按 num_return_sequences 聚合」这一条真实约束。同时统一 beam 场景的 C++/Python 口径(要么 Python 也按num_return_sequences聚合,要么 C++ 固定按单一主序列计数),把该口径写进TraceAttributes.h或attributes.py注释作为单一事实来源,并补一条 beam 请求的 usage 断言用例。
- 建议:直接写成
- enqueue 等待时长单位由微秒改为毫秒,WorkerStatus 对外取值缩小约 1000 倍 @
rtp_llm/cpp/model_rpc/RpcServerRuntimeMeta.h:89- 建议:保留该修复(单位本就应为 ms),但在 PR description 中单列这一行为变更,并与 FlexLB/调度侧确认是否存在按旧(微秒)量级校准的 hang 判定阈值或排序逻辑需同步调整。若下游存在此类硬编码阈值,建议把该修复拆成独立 commit,便于单独回滚、灰度与回归定位。
- Decode 加载 KV 失败回传的 gRPC status 改为错误码映射,会触发 Prefill 侧精确 ErrorCode 被覆写 @
rtp_llm/cpp/model_rpc/DecodeRpcServer.cc:313- 建议:在 PR description 中显式说明该跨节点终态语义变更(含新旧版本混跑影响)。若需保留 Prefill 对精确 ErrorCode 的解析,可在
CLIENT_GRPC_RET_IF_ERROR的RESOURCE_EXHAUSTED分支加上「仅当调用方尚未拿到业务 error_code 时才覆盖」的条件,或在 Decode 侧为加载失败一并附带ErrorDetailsPB(与PrefillBatchRpcServer.cc一致)。并补充覆盖:(1)Decode load 失败映射到RESOURCE_EXHAUSTED时 Prefill 最终 ErrorCode 不被DECODE_MALLOC_FAILED覆写;(2)CACHE_STORE_LOAD_BUFFER_TIMEOUT→ DEADLINE_EXCEEDED 时 Prefill 仍归类为业务错误而非请求级 deadline。若仅为让 span 拿到更精确分类,则应保持回传 INTERNAL,分类信息只写入 span 属性。
- 建议:在 PR description 中显式说明该跨节点终态语义变更(含新旧版本混跑影响)。若需保留 Prefill 对精确 ErrorCode 的解析,可在
- arch_config 新增 telemetry_test_deps() 扩展点,需确认内部所有 override 变体已同步 @
arch_config/arch_select.bzl:185- 建议:在 PR description 中注明内部
arch_config全部 override 变体已同步新增telemetry_test_deps(),或提供对应改动链接;并确认内部实现返回值与开源侧空列表在语义上兼容(内部 lock 提供 opentelemetry 运行时)。函数注释可补一句返回值契约:必须返回 list。
- 建议:在 PR description 中注明内部
- wheel 把 opentelemetry 变成强制依赖,两个 pin 相差 29 个 minor 且无 lock 与导入用例校验 @
rtp_llm/BUILD:702- 建议:三选一并在 PR description 记录:(1) 移入
extras_require(如rtp_llm[trace]),与tracing.py的可选降级语义对齐;(2) 若必须默认安装,把两个 pin 对齐到同一 minor 并在公共deps/的 pip 输入中同步声明,使 wheel metadata 与构建 lock 同源;(3) 至少补一个最小 py_test,在带该组合的 lock 下真实执行from opentelemetry.exporter.otlp.proto.http.trace_exporter import OTLPSpanExporter并构造一次 exporter(test_tracing.py:79已有雏形,但其依赖来自telemetry_test_deps()而非 wheel 声明的组合)。并在注释写明 exporter 停在 1.15.0 的确切约束来源与已实测通过的组合。
- 建议:三选一并在 PR description 记录:(1) 移入
- P→D CLIENT span 的阶段耗时属性在重试时跨 attempt 累加 @
rtp_llm/cpp/model_rpc/PrefillGenerateContext.cc:149- 建议:两种方案择一:(1) 在
reset()中重置stat_info的各*_rt_us累加字段(需先确认 kmonitor 侧期望单次还是累计语义,若依赖累计则不可动);(2) 保留累计语义,在closeGrpcStream()中记录上次写入的基线只写增量,或把属性名改为明确的累计语义并同步修正注释。建议在RpcWriterCancellationTest.cc补一条「两次 attempt 后第二个 span 的 allocate_rt_us 不含第一次」的用例。
- 建议:两种方案择一:(1) 在
- PhaseTiming 装配在 C++ 三处、span 结算序列在 Python 五处重复,且含一个恒假子条件 @
rtp_llm/cpp/model_rpc/LocalRpcServer.cc:221- 建议:在
PhaseSpanSynthesizer.h增加makePhaseTiming(const GenerateStream::TimeInfo&, int64_t request_id, int64_t synthesis_end_us)工厂,三个 handler 只传PhaseRole与error_type,并把 Local/Prefill 共用的error_type三元式收敛为与phaseErrorType同层的辅助函数。Python 侧抽出统一收尾函数_finalize_client_span(span, outputs, status, *, include_all_sequences, error=None, error_type=""),各分支只决定差异参数;同时移除 :1034 中恒假的engine_finished子条件。
- 建议:在
- PD 分离判定在 Python 侧重复实现,且 rtp_llm.pd_sep 存在两个事实来源 @
rtp_llm/cpp/model_rpc/model_rpc_client.py:56- 建议:优先复用引擎权威值:只在 AuxInfo 缺失(如错误提前返回)时使用客户端预测,并在代码中显式注明 precedence。若必须保留 Python 副本,请补一条跨语言 parity 测试(参考仓内
has_num_beams()↔hasNumBeams()的做法),或把该判定下沉为GenerateConfig上的单一方法供两侧共享。
- 建议:优先复用引擎权威值:只在 AuxInfo 缺失(如错误提前返回)时使用客户端预测,并在代码中显式注明 precedence。若必须保留 Python 副本,请补一条跨语言 parity 测试(参考仓内
- RtpLLMOp 重复实现 RoleType→字符串映射,新增角色会静默落到 unknown 且无编译期提示 @
rtp_llm/cpp/pybind/multi_gpu_gpt/RtpLLMOp.cc:353- 建议:复用现有映射并只做大小写转换(
roleTypeToString(role_type)后std::transform(..., ::tolower)),或在RoleTypes.h补一个roleTypeToLowerString()供两侧共用,使枚举扩展只需改一处。
- 建议:复用现有映射并只做大小写转换(
- master 返回空地址列表时 route_source 被误标为 master+domain_fallback @
rtp_llm/server/backend_rpc_server_visitor.py:425- 建议:不以返回值推断 master 是否真正贡献地址,改以地址集合的实际变化为依据:调用
get_master_route_addrs前记录len(input.generate_config.role_addrs),仅当长度增加时才置master_route_succeeded = True与route_source = "master"。空列表场景会正确落到"domain_fallback",并顺带为该异常保留可观测信号。
- 建议:不以返回值推断 master 是否真正贡献地址,改以地址集合的实际变化为依据:调用
- route span 未把取消归一化为 Cancelled,客户端断连计入路由错误率 @
rtp_llm/server/backend_rpc_server_visitor.py:488- 建议:在 except 分支补一条与其他埋点一致的分类:对
asyncio.CancelledError/GeneratorExit置route_error_type = "Cancelled",其余非FtRuntimeException走一个固定值(如"RouteError"),使取值域保持有界,并在_FakeRouteSpan记录error_type后补断言。
- 建议:在 except 分支补一条与其他埋点一致的分类:对
- route span 新单测的 Fake 丢弃 finish 调用,成功路径与 RouteError 分类零覆盖 @
rtp_llm/server/test/backend_rpc_server_visitor_test.py:117- 建议:让
_FakeRouteSpan.finish记录调用次数与error/error_type到实例字段;三个用例断言finish恰好一次,失败用例断言error_type == "RouteError";补充纯master(master 一次补全全部 role)与domain_fallback(master 返回connection_failed)两个用例;断言处统一加assertIn(route_span.attributes[trace_attrs.RTP_LLM_ROUTE_SOURCE], trace_attrs.RTP_LLM_ROUTE_SOURCE_VALUES),让声明的枚举契约真正生效。
- 建议:让
- 遥测关闭相关用例依赖调度时序,且与同文件另一用例的期望互相矛盾 @
rtp_llm/cpp/model_rpc/test/model_rpc_client_test.py:1286- 建议:按最终确定的行为统一两个用例:若采纳前述建议在
client_span is None时不创建 task,请把 :1286 改为直接断言不变量本身(暴露可观测的任务句柄并断言为空),并在断言前await asyncio.sleep(0)排除「只是还没被调度」,同时把 :1258 的无 span 分支改为断言不取消;若保留无条件 settlement,请把 :1286 改名为「未开启遥测时也会结算 RPC 终态」并断言code_waited is True,避免测试固化调度时序。另建议扩展 :722 的真实 grpc.aio 用例:servicer 在 finished 帧后延迟关流,断言客户端仍能读到 EOF、span 状态为成功。
- 建议:按最终确定的行为统一两个用例:若采纳前述建议在
- dash_sc telemetry 生命周期用例把 role/rank 接线整体 mock,fail-open 用例零断言 @
rtp_llm/dash_sc/test/app_test.py:192- 建议:第一个用例改为 patch 更内层的
bg_app.init_telemetry(保留_init_trace_telemetry真实实现),断言assert_called_once_with("dash_sc", 0),_shutdown_trace_telemetry同理断言shutdown_telemetry被调用一次,让角色与 rank 这一可观测性索引键真正被覆盖;第二个用例用with self.assertLogs(level="WARNING")断言「异常被吞掉的同时确实留下可排查日志」。
- 建议:第一个用例改为 patch 更内层的
- WORKSPACE 内联第三方 http_archive 绕过既有依赖分层与内源覆盖点 @
WORKSPACE:5- 建议:将该
http_archive迁入deps/http.bzl的http_deps()(patches 标签用clean_dep(...),与deps/git.bzl对zlib.BUILD的写法一致),或按 xgrammar 先例新增3rdparty/opentelemetry_cpp/repositories.bzl;WORKSPACE只保留opentelemetry_cpp_deps()调用并维持它在http_deps()/git_deps()之后的现有次序,覆盖点与依赖优先级两者都能保住。同时建议在repo_mapping处补一行注释:本仓 gRPC 仓库名为grpc(deps/git.bzl:223)而上游 otel BUILD 引用@com_github_grpc_grpc,当前仅用 HTTP exporter 故其不在可达图内;若接入 OTLP gRPC exporter 必须补"@com_github_grpc_grpc": "@grpc",否则会静默链入第二份 gRPC。
- 建议:将该
P3
- request_id 双键在多处硬编码,绕过 attributes 单一注册表与 parity 测试 @
rtp_llm/frontend/frontend_server.py:456- 建议:将这 4 行改为
trace_attrs.REQUEST_ID/trace_attrs.RTP_LLM_REQUEST_ID,并把测试内的字面量同步换成常量引用,使重命名在导入期或测试中立即暴露。
- 建议:将这 4 行改为
- 抢占终结路径未传 attempt_error_override,被抢占的 P→D CLIENT span 会被标为成功 @
rtp_llm/cpp/model_rpc/PrefillGenerateContext.cc:302- 建议:改为
closeGrpcStream(ErrorCodeToString(ErrorCode::PRIORITY_PREEMPTED)),使被抢占的尝试带上稳定的error.type;并在RpcWriterCancellationTest.cc补一条「抢占终结 + Finish()==OK」的用例,断言 span 状态为 kError。
- 建议:改为
- 重试时若 span 创建失败,pd_client_span_guard 保留已结算的旧 guard @
rtp_llm/cpp/model_rpc/PrefillGenerateContext.h:159- 建议:在
reset()中显式pd_client_span_guard.reset(),或在remoteAllocateResource改为「先无条件 reset 旧 guard,再按需创建新 guard」,让该成员始终只表示当前 attempt。
- 建议:在
- getRpcConnection 中 try/catch 包裹的是已判空的 host 解引用,属不可达防御代码 @
rtp_llm/cpp/model_rpc/PrefillRpcServer.cc:272- 建议:删除 try/catch 直接赋值(函数入口 :220-221 已先清零两个字段,重试不会残留上一轮 endpoint;
startChildClientSpan内部亦有 endpoint 校验)。若想减少重复状态,可只保留decode_addr并在startChildClientSpan调用点解析出 address/port,或反之只保留trace_server_address/trace_server_port并由其拼出decode_addr。
- 建议:删除 try/catch 直接赋值(函数入口 :220-221 已先清零两个字段,重试不会残留上一轮 endpoint;
- GRPC_RET_IF_ERROR 用魔法字符串 "0" 判断请求是否已获得 request_id @
rtp_llm/cpp/model_rpc/DecodeRpcServer.cc:42- 建议:改用意图直接的条件,例如
decode_context.request_id == 0(与reportEarlyFinishTask中已有判定一致),或在DecodeGenerateContext上暴露hasRequestIdentity()访问器,避免字符串字面量与构造细节耦合。
- 建议:改用意图直接的条件,例如
- localGenerate 对 server_context 未判空,与同一 diff 内相邻两处写法不一致 @
rtp_llm/cpp/model_rpc/DecodeRpcServer.cc:326- 建议:统一为一处 helper,例如在
GenerateContext上提供bool isTransportCancelled() const { return server_context && server_context->IsCancelled(); },三处统一调用;或在RemoteGenerate入口用RTP_LLM_CHECK_WITH_INFO一次性确立不变量后三处都直接解引用并去掉宏内判空。
- 建议:统一为一处 helper,例如在
- 空 content 帧的 token 增量被吞掉,前端 TPOT 分母偏小 @
rtp_llm/frontend/frontend_server.py:656- 建议:把「是否推进游标」与「是否上报」绑定:
visible_outputs == 0时不要提前推进last_observed_output_tokens(留给下一个可见帧一并上报),或在visible_outputs == 0但增量为正时也上报该增量。请同步修正该用例的期望值,使可见 token 总数与completion_tokens一致。
- 建议:把「是否推进游标」与「是否上报」绑定:
- protobuf 黑名单过滤把 map key 复制成魔法字符串 @
3rdparty/protobuf/BUILD:890- 建议:改为遍历 key 并把例外项提到命名常量,例如
_PROTOC_LIB_ONLY_PROTOS = ["compiler_plugin"],再写[name + "_proto" for name in WELL_KNOWN_PROTO_MAP if name not in _PROTOC_LIB_ONLY_PROTOS],让「哪些 WKT 由 :protoc_lib 而非 :protobuf 提供」这一意图只存在一处。
- 建议:改为遍历 key 并把例外项提到命名常量,例如
- in_memory_span_exporter 被加入包级共享 test_deps,实际只有一个 cc_test 使用 @
rtp_llm/cpp/model_rpc/test/BUILD:51- 建议:与同包
query_converter_test(deps = test_deps + [...])的既有写法一致,把该 exporter 从test_deps移到rpc_writer_cancellation_test自己的deps上。
- 建议:与同包
- OTel 裁剪 patch 缺少来源与退出条件说明 @
patches/opentelemetry_cpp/0001-trace-only-otlp-recordable.patch:1- 建议:在 patch 顶部补 3-5 行注释头:目标上游版本与 commit、裁剪原因(trace-only signal,规避 metrics proto 依赖)、明确声明「上游 metric exporter 目标在本仓内不可构建,如需接入 metrics 请先移除本 patch」、以及可下线条件(例如上游提供 trace-only 构建开关后即可删除)。
- 端点校验用例依赖 span 导出顺序做下标索引,未复用同目录已有的按名查找 @
rtp_llm/cpp/telemetry/test/telemetry_test.cc:516- 建议:在本文件也提供按名查找的小工具(或与另两个测试文件共用一份),把
spans[i]换成findSpan(spans, "endpoint_" + std::to_string(i)),让断言与导出顺序解耦。
- 建议:在本文件也提供按名查找的小工具(或与另两个测试文件共用一份),把
- 用字面量 getattr 读取动态挂载的 rtp_error_code @
rtp_llm/server/backend_rpc_server_visitor.py:484- 建议:在
FtRuntimeException上显式声明rtp_error_code: Optional[int] = None,:476 改为普通赋值,此处改为int(e.rtp_error_code if e.rtp_error_code is not None else e.exception_type),消除字面量属性探测并让类型检查覆盖该字段。
- 建议:在
- QueryConverter.cc 仅有一处与本 PR 无关的空行删除 @
rtp_llm/cpp/model_rpc/QueryConverter.cc:195- 建议:从本 PR 回退该文件的改动,保持 diff 只包含与追踪功能相关的内容;若确有格式统一需求,放到独立的格式化 commit 中。
- dash_grpc_comparer 中 cancel_requested 为无效赋值 @
rtp_llm/test/smoke/dash_grpc_comparer.py:411- 建议:删除
cancel_requested = True这一行,只保留从cancel_state回填的赋值;若确实要为ValueError路径保留「已发起取消」的标记,请写进SmokeException的诊断信息而不是留一个被覆盖的局部变量。
- 建议:删除
- 测试新增未被任何用例引用的辅助脚手架 @
rtp_llm/cpp/engine_base/stream/test/GenerateStreamTest.cc:96- 建议:删除
createBeamStream(),待真正需要 beam 场景用例时再随用例一并加入;若确实计划在后续 PR 接入,请在本 PR 中同时补上使用它的用例,使脚手架始终有至少一个调用点保证其有效性。
- 建议:删除
Checklist Findings (22 fail / 54 total)
General Principles Checklist
- [6.1] Architecture — 依赖方向:无循环依赖/跨层惊喜 → issue
arch_config 新增 telemetry_test_deps() 扩展点,需确认内部所有 override 变体已同步
开源侧新增telemetry_test_deps()返回[],被 4 个 BUILD 通过load("@arch_config//:arch_select.bzl", "telemetry_test_deps")引用:rtp_llm/cpp/model_rpc/test/BUILD:2、rtp_llm/telemetry/test/BUILD:1、rtp_llm/frontend/test/BUILD:1、rtp_llm/dash_sc/test/BUILD:1。WORKSPACE:36-39将@arch_config声明为local_repository,内部构建以--override_repository替换;若内部副本(含 PPU/ROCm 等变体)未同步声明同名 symbol,这 4 个 BUILD 会在 load 阶段直接失败(file does not contain symbol),影响面不止测试目标本身。本次评审工作区不含内部代码,无法自证已同步。 - [6.1] Architecture — 兼容性:外部 HTTP/RPC API、持久数据、配置、环境迁移安全 → issue
wheel 把 opentelemetry 变成强制依赖,两个 pin 相差 29 个 minor 且无 lock 与导入用例校验
whl_reqs新增opentelemetry-sdk==1.44.0与opentelemetry-exporter-otlp-proto-http==1.15.0(注释:为保住 protobuf==4.25)。opentelemetry-python 的 api/sdk/exporter 按同版本号同步发布,两个 pin 相差 29 个 minor,1.15 时代 exporter 依赖的若干opentelemetry.sdk.environment_variables常量在后续版本已迁移。全仓无任何 lock/requirements 出现 opentelemetry,wheel metadata 与构建依赖不同源;exporter 还会拉入未被仓内 lock 解析的opentelemetry-proto==1.15.0。同时rtp_llm/BUILD:447-455刻意不声明requirement()、tracing.py导入失败退化 no-op,说明设计上它是可选依赖,wheel 却把它变成强制依赖,两处语义相反;expor - [6.1] Architecture — 分层边界:新概念在正确层级,不泄漏内部 → issue
WORKSPACE 内联第三方 http_archive 绕过既有依赖分层与内源覆盖点
改动后io_opentelemetry_cpp是WORKSPACE中唯一的内联http_archive(:5-19);其余第三方归档全部集中在@rtp_deps的deps/http.bzl/deps/git.bzl,需要后续*_deps()宏的依赖走仓内包装文件(3rdparty/xgrammar/repositories.bzl,:49-51 有先例)。更实际的后果是:@rtp_deps是local_repository(:31-34),也是内源构建通过--override_repository替换的标准覆盖点;OTel 声明移出该边界后,内源侧无法再通过替换@rtp_deps改用内部镜像或另一版本,只能反过来 patch 顶层WORKSPACE。 - [6.1] Architecture — 可观测性:日志/指标/超时可操作、非噪声 → issue
空 content 帧的 token 增量被吞掉,前端 TPOT 分母偏小
observed_token_delta一旦计算就同步推进last_observed_output_tokens(:653-656),但只有visible_outputs > 0时才调用record_frontend_output_tokens(:658-660)。当某帧usage.completion_tokens已增长而delta.content为空串时,该增量被消费却不上报,后续帧只能看到剩余差值。test_streaming_latency_counts_only_visible_output_tokens(frontend_server_test.py:172-218)的响应序列中第 2 帧为content: ""且completion_tokens: 1,期望token_counts == [1, 2]合计 3,而该序列最终completion_tokens为 4——这 1 个 token 的丢失已被固化进期望值。由于该累计值是 TPOT 分母,偏小会使前端 TPOT 系统性偏大。 - [6.1] Architecture — 回滚路径:风险行为存在运维回滚手段 → issue
OTel 裁剪 patch 缺少来源与退出条件说明
patch 直接以diff --git a/exporters/otlp/BUILD开头,没有任何 header:既未记录针对的上游版本(WORKSPACE:7 的版本注释只在调用方),也未说明裁剪动机与可以删除该 patch 的条件。它从共享的:otlp_recordable目标中移除了otlp_metric_utils.{cc,h}、otlp_preferred_temporality.h与 metrics proto 依赖,副作用是 archive 内依赖这些头/源的 OTLP metric exporter 目标自此不可构建;未来若有人尝试接入 OTel metrics,只会得到一个来源不明的编译错误。 - [6.1] Architecture — 状态不变量:创建/更新/失败/重试/回滚路径有效 → issue
重试时若 span 创建失败,pd_client_span_guard 保留已结算的旧 guard
该成员注释声明「Recreated per retry attempt in remoteAllocateResource」,但remoteAllocateResource(PrefillRpcServer.cc:360-378)先把上一次 attempt 的 guard 标记"Retry"并 finish(:361-365),随后仅在client_span != nullptr时才重建(:374-377)。PrefillGenerateContext::reset()(:190-205)重置了grpc_stream_closed/last_grpc_stream_closed_status,但不清空pd_client_span_guard。因此当采样丢弃或 span 创建返回 nullptr 时,新 attempt 会带着一个已 finish 的旧 guard 进入closeGrpcStream,其中的setAttribute/finish全部落空(幂等无副作用),但该成员状态与实际 attempt 不再对应,与注释不符。 - [6.1] Architecture — 错误语义:fail-fast/retry/fallback/silent 行为显式 → issue
用字面量 getattr 读取动态挂载的 rtp_error_code
int(getattr(e, "rtp_error_code", e.exception_type))以字符串字面量做属性探测。根因是rtp_error_code并非FtRuntimeException的声明字段,而是 :476 动态挂载的。当前取值安全(:474 已保证error_code is not None,ExceptionType是 IntEnum),但该写法对静态类型检查不可见,字段被重命名或改为非 int 时不会有告警,且新增异常类型时无法从类定义看出这条隐式契约。 - [6.1] Quality — Mega-PR 已拆分为独立变更 → issue
enqueue 等待时长单位由微秒改为毫秒,WorkerStatus 对外取值缩小约 1000 倍
旧代码把stream->getTimeInfo().wait_time_us直接传给makeTaskInfo(..., int64_t waiting_time_ms),现改为time_info.wait_time_us / 1000。该字段经EngineScheduleInfo::TaskInfo::waiting_time_ms暴露到rtp_llm/server/worker_status.py:40(注释for master check server is hang or not)并由 FlexLB 消费;在dequeue覆盖之前,RUNNING 任务上报的正是 enqueue 写入的值,因此运行中任务的上报量级突然缩小约 1000 倍。修正本身正确(captureStreamRuntimeSnapshot:244 一直是/1000,两侧此前不一致)且有单测,但属与遥测主题无关的对外可观测行为变更,PR 未见迁移说明。 - [6.1] Quality — 逻辑变更未混入无关格式化 → issue
QueryConverter.cc 仅有一处与本 PR 无关的空行删除
该文件在本 PR 中只有一个 hunk(21→20 行),全部改动是在end_think_token_ids循环(:193-195)与role_addrs循环(:196)之间删掉一个空行,无任何逻辑变化。在一个跨 81 个文件的 PR 里,这类纯格式抖动会稀释 diff 信号,也让后续git blame/bisect 多出一个无意义的接触点。 - [6.1] Software Engineering — DRY:重复非平凡逻辑被抽取或显式复用 → issue
protobuf 黑名单过滤把 map key 复制成魔法字符串
新的blacklisted_protos写作[proto[0] + "_proto" for proto in WELL_KNOWN_PROTO_MAP.items() if proto[0] != "compiler_plugin"]。"compiler_plugin"与WELL_KNOWN_PROTO_MAP的 key 是同一份信息的两处副本:若上游 vendored map 的 key 改名,过滤条件会静默失效,compiler_plugin_proto被一并列入黑名单,依赖它的cc_proto_library会退化成缺少plugin.pb.cc的链接错误,而这类错误的现场信息与真实原因相距很远。另外此处用.items()后按下标取 key,可直接遍历 dict 取 key。 - [6.1] Software Engineering — ISP:调用方不依赖无关大接口 → issue
in_memory_span_exporter 被加入包级共享 test_deps,实际只有一个 cc_test 使用
@io_opentelemetry_cpp//exporters/memory:in_memory_span_exporter被追加到包级共享列表test_deps(:39-52),该列表被local_rpc_server_test、prefill_rpc_server_test、decode_rpc_server_test、response_buffer_test、prefill_batch_rpc_server_test、rpc_server_runtime_meta_test、query_converter_test等多个 target 共用。但全目录搜索显示只有RpcWriterCancellationTest.cc:12真正包含该 exporter 的头文件。其余 target 因此多链接一个与自身无关的测试期依赖,既扩大重建触发面,也让「哪个测试需要 OTel」在 BUILD 里读不出来。 - [6.1] Software Engineering — KISS/YAGNI:无投机性抽象 → issue
测试新增未被任何用例引用的辅助脚手架
GenerateStreamBuilder::createBeamStream()(:96 起,含 KVCacheManager 初始化与 beam 配置)在整个文件中没有任何调用点(全文件搜索仅命中定义处),属于为未来用例预留的投机性脚手架。未被引用的测试构造器会随生产代码演进而静默腐坏,下一个使用者拿到的是从未被验证过的初始化路径。 - [6.1] Software Engineering — SRP:模块/类职责单一 → issue
PhaseTiming 装配在 C++ 三处、span 结算序列在 Python 五处重复,且含一个恒假子条件
LocalRpcServer.cc:221-230、PrefillRpcServer.cc:771-780、DecodeRpcServer.cc:1424-1433三处逐字段抄写同一份PhaseTiming(9 行完全相同赋值),仅PhaseRole与error_type计算不同,其中 Local(:242-246)与 Prefill(:781-785)的error_type三元式亦逐字重复。Python 侧_record_client_rpc_status+_record_client_span_usage+_record_client_span_latency+finish(...)的组合在 :982、:1025、:1053、:1084、:1100 共 5 处重复。任一字段新增都需人工比对 3~5 处,漏改不会有编译错误或测试失败。另 :1034 的engine_finished or ...中engine_finished在该分支恒为假:为真意味着 :966 已创建 task,外层 `cl - [6.1] Tests — 分布式/跨平台变更有对应覆盖 → issue
Decode 加载 KV 失败回传的 gRPC status 改为错误码映射,会触发 Prefill 侧精确 ErrorCode 被覆写
旧代码恒返回grpc::StatusCode::INTERNAL,现为grpc::Status(transErrorCodeToGrpc(error_info.code()), ...)。RpcErrorCode.h:13-18把MALLOC_FAILED/DECODE_MALLOC_FAILED/PRIORITY_PREEMPTED映射为RESOURCE_EXHAUSTED。Prefill 侧remoteLoadCacheEnd先从 PB 解析出精确error_code(PrefillRpcServer.cc:493)再走CLIENT_GRPC_RET_IF_ERROR(:501),而该宏 :147-149 有if (status.error_code() == RESOURCE_EXHAUSTED) new_error_code = DECODE_MALLOC_FAILED;。改动前该分支在此路径永不触发;改动后精确 ErrorCode(尤其PRIORITY_PREEMPTED)会被覆盖,且该终态不携带 `Er - [6.1] Tests — 新逻辑有聚焦单测 + 相关集成/smoke 测试 → issue
端点校验用例依赖 span 导出顺序做下标索引,未复用同目录已有的按名查找
ClientSpanCanonicalizesAndValidatesEndpoint(:484)在ASSERT_EQ(spans.size(), cases.size() + 1)之后用spans[i]与cases[i]逐一对应(:517-527),隐含假设 InMemorySpanExporter 的导出顺序严格等于End()的调用顺序。虽然 span 名已按"endpoint_" + i编好,同目录的phase_span_synthesizer_test.cc:52(findSpan)与grpc_propagation_test.cc:41(findSpanByName)都提供了按名查找工具,本文件却没有采用;一旦 BSP 批次切分或导出顺序变化,失败信息会表现为某个用例的属性断言错乱,而不是直观的「找不到该 span」。 - [6.1] Tests — 边界 case 覆盖(空、单元素、最大值) → issue
空 content 帧的 token 增量被吞掉,前端 TPOT 分母偏小
observed_token_delta一旦计算就同步推进last_observed_output_tokens(:653-656),但只有visible_outputs > 0时才调用record_frontend_output_tokens(:658-660)。当某帧usage.completion_tokens已增长而delta.content为空串时,该增量被消费却不上报,后续帧只能看到剩余差值。test_streaming_latency_counts_only_visible_output_tokens(frontend_server_test.py:172-218)的响应序列中第 2 帧为content: ""且completion_tokens: 1,期望token_counts == [1, 2]合计 3,而该序列最终completion_tokens为 4——这 1 个 token 的丢失已被固化进期望值。由于该累计值是 TPOT 分母,偏小会使前端 TPOT 系统性偏大。
RTP-LLM Checklist
- [I] 代码质量 — RAII span 作用域匹配 → issue
重试时若 span 创建失败,pd_client_span_guard 保留已结算的旧 guard
该成员注释声明「Recreated per retry attempt in remoteAllocateResource」,但remoteAllocateResource(PrefillRpcServer.cc:360-378)先把上一次 attempt 的 guard 标记"Retry"并 finish(:361-365),随后仅在client_span != nullptr时才重建(:374-377)。PrefillGenerateContext::reset()(:190-205)重置了grpc_stream_closed/last_grpc_stream_closed_status,但不清空pd_client_span_guard。因此当采样丢弃或 span 创建返回 nullptr 时,新 attempt 会带着一个已 finish 的旧 guard 进入closeGrpcStream,其中的setAttribute/finish全部落空(幂等无副作用),但该成员状态与实际 attempt 不再对应,与注释不符。 - [I] 代码质量 — 删除或重命名内部 file、registry entry、model name、metric enum、op binding、plugin symbol 时,必须全仓搜索消费者,并提供替代实现、迁移说明或 smoke 覆盖;只有暴露到 HTTP/RPC/config/persisted format 时才按外部兼容性处理 → issue
OTel 裁剪 patch 缺少来源与退出条件说明
patch 直接以diff --git a/exporters/otlp/BUILD开头,没有任何 header:既未记录针对的上游版本(WORKSPACE:7 的版本注释只在调用方),也未说明裁剪动机与可以删除该 patch 的条件。它从共享的:otlp_recordable目标中移除了otlp_metric_utils.{cc,h}、otlp_preferred_temporality.h与 metrics proto 依赖,副作用是 archive 内依赖这些头/源的 OTLP metric exporter 目标自此不可构建;未来若有人尝试接入 OTel metrics,只会得到一个来源不明的编译错误。 - [I] 代码质量 — 同一功能用统一工具函数 → issue
端点校验用例依赖 span 导出顺序做下标索引,未复用同目录已有的按名查找
ClientSpanCanonicalizesAndValidatesEndpoint(:484)在ASSERT_EQ(spans.size(), cases.size() + 1)之后用spans[i]与cases[i]逐一对应(:517-527),隐含假设 InMemorySpanExporter 的导出顺序严格等于End()的调用顺序。虽然 span 名已按"endpoint_" + i编好,同目录的phase_span_synthesizer_test.cc:52(findSpan)与grpc_propagation_test.cc:41(findSpanByName)都提供了按名查找工具,本文件却没有采用;一旦 BSP 批次切分或导出顺序变化,失败信息会表现为某个用例的属性断言错乱,而不是直观的「找不到该 span」。
Python Static-First Checklist
- [P.A] 静态结构与类型纪律 — 禁止 getattr/setattr literal 访问 → issue
用字面量 getattr 读取动态挂载的 rtp_error_code
int(getattr(e, "rtp_error_code", e.exception_type))以字符串字面量做属性探测。根因是rtp_error_code并非FtRuntimeException的声明字段,而是 :476 动态挂载的。当前取值安全(:474 已保证error_code is not None,ExceptionType是 IntEnum),但该写法对静态类型检查不可见,字段被重命名或改为非 int 时不会有告警,且新增异常类型时无法从类定义看出这条隐式契约。 - [P.B] 错误处理 — 资源获取用 with context manager → issue
opentelemetry 可用性契约自相矛盾,两个 py_test 在公共 lock 下确定性失败而非跳过
ModelRpcClientGrpcMetadataTest(:721)无 skip 装饰器,run()内直接from opentelemetry.sdk...in_memory_span_exporter import InMemorySpanExporter;其依赖取自telemetry_test_deps()(test/BUILD:32),公共实现返回[]并注释「公共 pip lock 没有 opentelemetry」(arch_select.bzl:186-190),rtp_llm/BUILD:453-455亦声明「两个构建都保持绿色」。telemetry/test/test_tracing.py:73按其模块 docstring(:10-11)刻意不加 skip 直接assertTrue(tracing.OTEL_AVAILABLE),其 docstring 又称 opentelemetry 来自执行镜像 site-packages——与 arch_select.bzl 注释直接冲突。同仓其余同类用例全部有守卫:`fronte - [P.G] 测试规范 — mock/fake/stub 不得替代本次声称覆盖的生产边界 → issue
dash_sc telemetry 生命周期用例把 role/rank 接线整体 mock,fail-open 用例零断言
test_start_initializes_dash_role_and_shutdowns用patch.object整体替换bg_app._init_trace_telemetry/_shutdown_trace_telemetry,断言只有两条无参assert_called_once_with()(:237-238),因此写进用例名的「dash role」完全未被验证。真正的角色/rank 契约在rtp_llm/dash_sc/app.py:70的init_telemetry("dash_sc", 0),被 mock 完全遮蔽——角色误改为frontend或 rank 误传,本用例仍通过,但线上 dash_sc trace 会被错误归到别的 service 组件下。紧邻的test_init_failure_is_fail_open(:241-245)只调用_init_trace_telemetry()而无任何断言:即使把 app.py:72 的logging.warning删成完全静默吞异常,用例照样绿。
Strengths
- span 生命周期设计有显式不变量:
GenerateContext.h:64把trace_span_guard刻意声明在error_status(:55)之后,利用成员逆序析构保证 guard 析构时仍能读到最终 gRPC 状态,并在注释中写明这一容易被后续改动破坏的隐性约束;phase_span_scope与pd_client_span_guard的相对声明顺序同样使CHECK_ERROR_STATUS提前 return、异常展开、~PrefillGenerateContext()中的closeGrpcStream()都落在正确时序。 - 严格 fail-open 契约:
startRpcServerSpan/startChildClientSpan/synthesizeChildSpan全部try/catch(...)兜底,TelemetryRuntime::isActive()前置短路,RequestSpanGuard::finish()由原子标志保证 exactly-once 且析构noexcept,tracing.py在OTEL_AVAILABLE为假时整体退化 no-op;PhaseSpanSynthesisScope对回调抛异常与回调为nullptr两种情形都有针对性用例。 - 属性值一律取自闭集:
grpcStatusCodeName/grpcStatusDescription/ErrorCodeToString,显式避免把error_message()原始文本写进 span,兼顾低基数与敏感信息不外泄;attributes.py:103-126建立 C++/Python 单一注册表并配 parity 测试,把两侧漂移从口头约定变成可失败的测试。 RpcServerRuntimeMeta把stream->getTimeInfo()/prefixLength()/statusInfo()全部移出 meta 写锁(captureStreamRuntimeSnapshot,:235-250),消除「meta 锁 → stream 锁」嵌套;dequeue(:183)、commitDequeueSnapshot(:268)与markPriorityPreemptionCanceled(:152)新增stream == stream身份校验,修掉 request_id 被替换流复用时误删运行态条目的 ABA 缺陷,并有正面用例锁定。GRPC_RET_IF_ERROR改写为do { ... } while (false)并把code/msg各求值一次,消除悬挂 else 与参数重复求值;phaseErrorType/generateRequestReadFailureStatus提为无副作用静态函数,配合-fno-access-control得到直接的分类单测覆盖。- 测试用真实边界而非纯 mock:
grpc_propagation_test.cc起真实 in-process gRPC server 并用findSpanByName校验导出 SpanData 的 trace_id/parent_span_id;phase_span_synthesizer_test.cc对三种 PhaseRole、zero-wait 跳过、非法窗口、三处失败截断做 µs→ns 精确值断言;model_rpc_client_test.py:722用真实 grpc.aio 验证 traceparent 跨进程边界;GenerateStreamTest.cc:1064的DoneGuard+ 双向握手 + 墙钟兜底使生产者失败不会挂死整个 target。 - 生命周期挂载位置正确:
TelemetryRuntime::init(RtpLLMOp.cc:373)在model_rpc_service_->init(:384)与BuildAndStart()(:418)之前,不存在 span 先于 runtime 产生的窗口,且置于gil_scoped_acquire内;shutdown()受is_server_shutdown_保护只执行一次,在gil_scoped_release作用域内做有界 flush,不持 GIL 阻塞 Python 退出。 - 观测指标做可信度约束而非硬填:
_record_client_span_latency在多序列 first_token 不一致、output_len非正、cost_time < ttft时不写,_record_client_span_usage(:210-215)对input_len不一致或output_len非正直接跳过;route_span刻意创建在主动限流检查之前(:369-372),使被 throttle 的请求仍留下带queue_length/queue_reject_threshold的诊断 span。 - 依赖引入次序谨慎:
opentelemetry_cpp_deps()(WORKSPACE:55)排在http_deps()(:43)/git_deps()(:47)之后,使 protobuf/absl/curl/boringssl 由本仓先行 pin 定、OTel 内部maybe()退化为 no-op;repo_mapping = {"@zlib": "@zlib_archive"}正确复用唯一一份已 pin 的 zlib;3rdparty/protobuf/BUILD:883-889用注释解释了 bazel 6 下blacklisted_protos需要 ProtoInfo 及必须放行compiler_plugin_proto的原因。
| # frame is the data-plane completion boundary; keep terminal | ||
| # observation off that path and settle the span independently. | ||
| normal_finish_seen = True | ||
| if client_settlement_task is None: |
There was a problem hiding this comment.
[P1] 遥测关闭时仍创建结算 task,把全量流式请求的 RPC 取消推迟最多 5.1 秒且无回滚开关
:959-977 的 create_task(_settle_client_span_after_rpc(...)) 无 client_span is not None 前置条件;start_client_span 在遥测关闭或 state 为空时返回 (None, []),task 照样创建,_settle_client_span_after_rpc(:138-150)仍 await asyncio.wait_for(code(), 5.0),超时后 response_iterator.cancel() 再等 0.1s。同时 should_cancel 新增 not normal_finish_seen(:1111),finished 帧后不再在 finally 立即取消。:1233-1259 的用例名 ..._with_or_without_trace 表明这是有意设计,但该行为不受 RTP_LLM_OTEL_TRACE_ENABLE 控制;对迭代到 EOF 的调用方,trailer 延迟 >5s 会把已交付全部 token...
建议: 把「finished 帧后延迟取消」与遥测解耦并提供回滚手段:create_task 与 normal_finish_seen 赋值加 client_span is not None 前置条件,should_cancel 的抑制条件改为 client_settlement_task is None(语义:只有 settle task 接管了 iterator 才不在 finally 取消),使遥测开关同时成为该新行为的开关、无遥测时精确回到改动前语义。若该「不让物理流无限存活」的约束确实需要覆盖全量流量,请拆为独立提交、把 RPC_SETTLE_TIMEOUT_SECONDS/RPC_CLEANUP_TIMEOUT_SECONDS 提为可配置项,并补一条真实 grpc.aio 用例:servicer 在 finished 帧后延迟关流、调用方仍在 async for,断言请求不被自身定时取消。同时为 task 保留句柄,在 finally 中对未完成者显式 cancel()(span 由 guard 兜底结算),避免请求结束后仍有游离 task 持有 response_iterator。
| await server.start() | ||
| channel = grpc.aio.insecure_channel(f"127.0.0.1:{port}") | ||
|
|
||
| from opentelemetry.sdk.trace.export.in_memory_span_exporter import ( |
There was a problem hiding this comment.
[P2] opentelemetry 可用性契约自相矛盾,两个 py_test 在公共 lock 下确定性失败而非跳过
ModelRpcClientGrpcMetadataTest(:721)无 skip 装饰器,run() 内直接 from opentelemetry.sdk...in_memory_span_exporter import InMemorySpanExporter;其依赖取自 telemetry_test_deps()(test/BUILD:32),公共实现返回 [] 并注释「公共 pip lock 没有 opentelemetry」(arch_select.bzl:186-190),rtp_llm/BUILD:453-455 亦声明「两个构建都保持绿色」。telemetry/test/test_tracing.py:73 按其模块 docstring(:10-11)刻意不加 skip 直接 assertTrue(tracing.OTEL_AVAILABLE),其 docstring 又称 opentelemetry 来自执行镜像 site-packages——与 arch_select.bzl 注释直接冲突。同仓其余同类用例全部有守卫:`fro...
建议: 明确取其一并使四处注释自洽:(1)若公共 lock 确实不带 opentelemetry,则给 ModelRpcClientGrpcMetadataTest 与 TestDependencyContract 加上与其他三处一致的 skipUnless(tracing.OTEL_AVAILABLE, ...),并把 test_tracing.py 的 docstring 改为与 arch_select.bzl 一致;(2)若这两个 target 必须在所有 lock 上验证真实链路,则让公共 telemetry_test_deps() 真正声明 opentelemetry,并同步修正 arch_select.bzl:186 与 rtp_llm/BUILD:453-455 的「保持绿色」表述。无论哪种,都请把 server.start()/insecure_channel() 之后的清理纳入 try/finally 或 contextlib.AsyncExitStack,保证任何提前失败都能停 server、关 channel 并复位 telemetry。
Checklist: [P.B] 资源获取用 with context manager
| // Beam rows are an internal search width; Fusion exposes one primary | ||
| // sequence (the remaining candidates live in beam_responses). Only | ||
| // ordinary multi-return requests aggregate all active rows. | ||
| const auto returned_sequence_count = stream->hasNumBeams() ? std::max(stream->numReturnSequences(), 1) : |
There was a problem hiding this comment.
[P2] returned_sequence_count 三元两支求值恒等,注释描述的区分不存在且与 Python 口径分叉
stream->hasNumBeams() ? std::max(stream->numReturnSequences(), 1) : stream->currentBatchSize()。currentBatchSize() = batchSize(outputTokenLen()),而 batchSize() 在 !hasNumBeams() 分支同样 return std::max(numReturnSequences(), 1)(GenerateStream.cc:334-340),且两处判定是同一谓词(:385-387)。两分支恒等、三元为死分支,:233-235 注释声称的「只有普通多返回请求才聚合所有活跃行」在代码里不成立。更进一步,Python 侧 include_all_sequences = not has_num_beams()(model_rpc_client.py:871)使 beam 请求只统计 generate_outputs[:1];当 num_beams>1 且 num_return_sequences>1 时...
建议: 直接写成 std::max(stream->numReturnSequences(), 1) 并删除三元表达式,同步修正注释只保留「beam 宽度不计入对外 usage、按 num_return_sequences 聚合」这一条真实约束。同时统一 beam 场景的 C++/Python 口径(要么 Python 也按 num_return_sequences 聚合,要么 C++ 固定按单一主序列计数),把该口径写进 TraceAttributes.h 或 attributes.py 注释作为单一事实来源,并补一条 beam 请求的 usage 断言用例。
| auto new_task = makeTaskInfo(TaskIdentity{identity.request_id, batch_id}, | ||
| stream->prefixLength(), | ||
| stream->inputLength(), | ||
| time_info.wait_time_us / 1000); |
There was a problem hiding this comment.
[P2] enqueue 等待时长单位由微秒改为毫秒,WorkerStatus 对外取值缩小约 1000 倍
旧代码把 stream->getTimeInfo().wait_time_us 直接传给 makeTaskInfo(..., int64_t waiting_time_ms),现改为 time_info.wait_time_us / 1000。该字段经 EngineScheduleInfo::TaskInfo::waiting_time_ms 暴露到 rtp_llm/server/worker_status.py:40(注释 for master check server is hang or not)并由 FlexLB 消费;在 dequeue 覆盖之前,RUNNING 任务上报的正是 enqueue 写入的值,因此运行中任务的上报量级突然缩小约 1000 倍。修正本身正确(captureStreamRuntimeSnapshot:244 一直是 /1000,两侧此前不一致)且有单测,但属与遥测主题无关的对外可观测行为变更,PR 未见迁移说明。
建议: 保留该修复(单位本就应为 ms),但在 PR description 中单列这一行为变更,并与 FlexLB/调度侧确认是否存在按旧(微秒)量级校准的 hang 判定阈值或排序逻辑需同步调整。若下游存在此类硬编码阈值,建议把该修复拆成独立 commit,便于单独回滚、灰度与回归定位。
Checklist: [6.1] Mega-PR 已拆分为独立变更
| reportEarlyFinishTask(decode_context, | ||
| static_cast<int64_t>(error_info.code()), | ||
| "decode load cache from prefill failed: " + error_info.ToString()); | ||
| decode_context.error_status = grpc::Status(transErrorCodeToGrpc(error_info.code()), error_info.ToString()); |
There was a problem hiding this comment.
[P2] Decode 加载 KV 失败回传的 gRPC status 改为错误码映射,会触发 Prefill 侧精确 ErrorCode 被覆写
旧代码恒返回 grpc::StatusCode::INTERNAL,现为 grpc::Status(transErrorCodeToGrpc(error_info.code()), ...)。RpcErrorCode.h:13-18 把 MALLOC_FAILED/DECODE_MALLOC_FAILED/PRIORITY_PREEMPTED 映射为 RESOURCE_EXHAUSTED。Prefill 侧 remoteLoadCacheEnd 先从 PB 解析出精确 error_code(PrefillRpcServer.cc:493)再走 CLIENT_GRPC_RET_IF_ERROR(:501),而该宏 :147-149 有 if (status.error_code() == RESOURCE_EXHAUSTED) new_error_code = DECODE_MALLOC_FAILED;。改动前该分支在此路径永不触发;改动后精确 ErrorCode(尤其 PRIORITY_PREEMPTED)会被覆盖,且该终态不携带 ...
建议: 在 PR description 中显式说明该跨节点终态语义变更(含新旧版本混跑影响)。若需保留 Prefill 对精确 ErrorCode 的解析,可在 CLIENT_GRPC_RET_IF_ERROR 的 RESOURCE_EXHAUSTED 分支加上「仅当调用方尚未拿到业务 error_code 时才覆盖」的条件,或在 Decode 侧为加载失败一并附带 ErrorDetailsPB(与 PrefillBatchRpcServer.cc 一致)。并补充覆盖:(1)Decode load 失败映射到 RESOURCE_EXHAUSTED 时 Prefill 最终 ErrorCode 不被 DECODE_MALLOC_FAILED 覆写;(2)CACHE_STORE_LOAD_BUFFER_TIMEOUT → DEADLINE_EXCEEDED 时 Prefill 仍归类为业务错误而非请求级 deadline。若仅为让 span 拿到更精确分类,则应保持回传 INTERNAL,分类信息只写入 span 属性。
Checklist: [6.1] 分布式/跨平台变更有对应覆盖
| def internal_deps(): | ||
| return [] | ||
|
|
||
| def telemetry_test_deps(): |
There was a problem hiding this comment.
[P2] arch_config 新增 telemetry_test_deps() 扩展点,需确认内部所有 override 变体已同步
开源侧新增 telemetry_test_deps() 返回 [],被 4 个 BUILD 通过 load("@arch_config//:arch_select.bzl", "telemetry_test_deps") 引用:rtp_llm/cpp/model_rpc/test/BUILD:2、rtp_llm/telemetry/test/BUILD:1、rtp_llm/frontend/test/BUILD:1、rtp_llm/dash_sc/test/BUILD:1。WORKSPACE:36-39 将 @arch_config 声明为 local_repository,内部构建以 --override_repository 替换;若内部副本(含 PPU/ROCm 等变体)未同步声明同名 symbol,这 4 个 BUILD 会在 load 阶段直接失败(file does not contain symbol),影响面不止测试目标本身。本次评审工作区不含内部代码,无法自证已同步。
建议: 在 PR description 中注明内部 arch_config 全部 override 变体已同步新增 telemetry_test_deps(),或提供对应改动链接;并确认内部实现返回值与开源侧空列表在语义上兼容(内部 lock 提供 opentelemetry 运行时)。函数注释可补一句返回值契约:必须返回 list。
Checklist: [6.1] 依赖方向:无循环依赖/跨层惊喜
|
|
||
| EXPECT_TRUE(TelemetryRuntime::shutdown(5000)); | ||
| auto spans = span_data->GetSpans(); | ||
| ASSERT_EQ(spans.size(), cases.size() + 1); |
There was a problem hiding this comment.
[P3] 端点校验用例依赖 span 导出顺序做下标索引,未复用同目录已有的按名查找
ClientSpanCanonicalizesAndValidatesEndpoint(:484)在 ASSERT_EQ(spans.size(), cases.size() + 1) 之后用 spans[i] 与 cases[i] 逐一对应(:517-527),隐含假设 InMemorySpanExporter 的导出顺序严格等于 End() 的调用顺序。虽然 span 名已按 "endpoint_" + i 编好,同目录的 phase_span_synthesizer_test.cc:52(findSpan)与 grpc_propagation_test.cc:41(findSpanByName)都提供了按名查找工具,本文件却没有采用;一旦 BSP 批次切分或导出顺序变化,失败信息会表现为某个用例的属性断言错乱,而不是直观的「找不到该 span」。
建议: 在本文件也提供按名查找的小工具(或与另两个测试文件共用一份),把 spans[i] 换成 findSpan(spans, "endpoint_" + std::to_string(i)),让断言与导出顺序解耦。
Checklist: [6.1] 新逻辑有聚焦单测 + 相关集成/smoke 测试;[I] 同一功能用统一工具函数
| if isinstance(e, FtRuntimeException): | ||
| route_span.set_attribute( | ||
| trace_attrs.RTP_LLM_ERROR_CODE, | ||
| int(getattr(e, "rtp_error_code", e.exception_type)), |
There was a problem hiding this comment.
[P3] 用字面量 getattr 读取动态挂载的 rtp_error_code
int(getattr(e, "rtp_error_code", e.exception_type)) 以字符串字面量做属性探测。根因是 rtp_error_code 并非 FtRuntimeException 的声明字段,而是 :476 动态挂载的。当前取值安全(:474 已保证 error_code is not None,ExceptionType 是 IntEnum),但该写法对静态类型检查不可见,字段被重命名或改为非 int 时不会有告警,且新增异常类型时无法从类定义看出这条隐式契约。
建议: 在 FtRuntimeException 上显式声明 rtp_error_code: Optional[int] = None,:476 改为普通赋值,此处改为 int(e.rtp_error_code if e.rtp_error_code is not None else e.exception_type),消除字面量属性探测并让类型检查覆盖该字段。
Checklist: [6.1] 错误语义:fail-fast/retry/fallback/silent 行为显式;[P.A] 禁止 getattr/setattr literal 访问
| query_info.cancel_after_response_count is not None | ||
| and response_count >= query_info.cancel_after_response_count | ||
| ): | ||
| cancel_requested = True |
There was a problem hiding this comment.
[P3] dash_grpc_comparer 中 cancel_requested 为无效赋值
cancel_requested = True(:411)之后紧接着执行 cancel()、code()、classify_dash_cancel(),随即在 :426 被 cancel_requested = cancel_state.cancel_requested 覆盖;ValueError 路径(:421-425)则直接 raise SmokeException。因此该赋值在任何路径下都不会被读到,读代码时容易误以为它是为异常路径准备的兜底值。
建议: 删除 cancel_requested = True 这一行,只保留从 cancel_state 回填的赋值;若确实要为 ValueError 路径保留「已发起取消」的标记,请写进 SmokeException 的诊断信息而不是留一个被覆盖的局部变量。
3fc322e to
b3de40f
Compare
LLLLKKKK
left a comment
There was a problem hiding this comment.
AI Code Review - PR #1341
Status: LGTM
Summary: P0/0 · P1/0 · P2/21 · P3/16
Reviewed: commit b3de40f53118 · 2026-08-27 21:11 UTC+8
lgtm ready to ci
Non-blocking Suggestions
P2
- opentelemetry 可用性契约自相矛盾,真实 gRPC 链路用例缺少跳过守卫 @
rtp_llm/cpp/model_rpc/test/model_rpc_client_test.py:793- 建议:对齐已有约定:把该 SDK import 放进
try/except ImportError并self.skipTest(...),或给ModelRpcClientGrpcMetadataTest中依赖 SDK 的用例加@unittest.skipUnless(tracing.OTEL_AVAILABLE, ...);test_trace_disabled_full_consumer_waits_for_real_grpc_terminal不依赖 SDK,应保持无守卫以避免公共构建退化为 all-skip。同时修正arch_select.bzl:186-189与rtp_llm/BUILD:453-455的注释,使三处对「运行时来源」的描述自洽。
- 建议:对齐已有约定:把该 SDK import 放进
- 遥测结算 task 会取消已交付完成的 RPC,并把自身清理取消标为 RpcError @
rtp_llm/cpp/model_rpc/model_rpc_client.py:145- 建议:让结算任务只观测、不干预:引擎已上报 finished 时不要
cancel(),或先等待「迭代已结束」事件再取消;把「自身超时取消」与status is None用独立低基数error_type(如SettleTimeout)或成功 +rtp_llm.rpc_terminal_unsettled=true表达,不要复用RpcError,否则线上无法区分真实远端故障与自身清理。同时把RPC_SETTLE_TIMEOUT_SECONDS提为可配置项(或与timeout_ms挂钩),并把游离 task 收敛到 client 实例级集合便于close()统一取消。
- 建议:让结算任务只观测、不干预:引擎已上报 finished 时不要
- Decode 加载 KV 失败回传的 gRPC status 改为错误码映射,会覆写 Prefill 侧精确 ErrorCode @
rtp_llm/cpp/model_rpc/DecodeRpcServer.cc:313- 建议:在 PR 描述中把该 P→D 终态契约变更单列为独立行为变更,并列出受影响指标(
cancel_qps、error_qps、error_code分布);补一条用例固化「decode 返回RESOURCE_EXHAUSTED/DEADLINE_EXCEEDED时 Prefill 最终上报的 ErrorCode」。若目的仅是让 span 的error.type更精确,更小的改法是保留INTERNAL、只把error_info.code()写进 span 属性——phaseErrorType已能独立从error_info识别DependencyFailure。
- 建议:在 PR 描述中把该 P→D 终态契约变更单列为独立行为变更,并列出受影响指标(
- enqueue 等待时长单位由微秒改为毫秒,WorkerStatus 对外取值缩小约 1000 倍 @
rtp_llm/cpp/model_rpc/RpcServerRuntimeMeta.h:89- 建议:拆为独立 commit,或至少在 PR 描述与发布说明中单列为「行为变更:WorkerStatus running 条目 waiting_time_ms 由 us 修正为 ms」,并与 FlexLB / 主控 hang 判定的维护方确认阈值;若需灰度,可先并行暴露新字段再切换旧字段。
- 同一 trace 内 Prefill 与 Decode SERVER span 各自完整申报 gen_ai.usage.* @
rtp_llm/cpp/model_rpc/PrefillRpcServer.cc:789- 建议:明确
gen_ai.usage.*的唯一权威写入点(建议只在面向客户端的请求入口 span 上写),PD 节点级数值改用rtp_llm.*私有 key,或在 Prefill/Decode span 上补一个节点作用域标识属性;在rtp_llm/telemetry/README.md写明聚合口径,并补一条 PD 场景断言,验证同一 trace 内gen_ai.usage.input_tokens不会被重复申报。
- 建议:明确
- returned_sequence_count 三元两支求值恒等,注释描述的区分不存在且与 Python 口径分叉 @
rtp_llm/cpp/model_rpc/LocalRpcServer.cc:236- 建议:删掉恒等三元;若确需区分 beam 与多返回,请改成真正不同的取值并修正注释。output 口径改为遍历实际返回序列累加各自长度,与
_record_client_span_usage统一;拿不到逐序列长度时跳过 usage 属性而非写入外推值,并补一条num_return_sequences > 1且长度不等的用例。
- 建议:删掉恒等三元;若确需区分 beam 与多返回,请改成真正不同的取值并修正注释。output 口径改为遍历实际返回序列累加各自长度,与
- PhaseTiming 装配在 C++ 三处、span 结算序列在 Python 五处重复,且含一处死赋值 @
rtp_llm/cpp/model_rpc/LocalRpcServer.cc:212- 建议:C++ 侧在
PhaseSpanSynthesizer.h提供phaseTimingFromTimeInfo(const TimeInfo&, int64_t request_id),并把三处回调收敛为以PhaseRole与 error_type 分类回调为参数的公共工厂;Python 侧抽出单一_settle_client_span(span, status, outputs, *, error=None, error_type=""),各 except 分支只提供 error/error_type,finally只做一次兜底调用,并删除 :1076 的死赋值。
- 建议:C++ 侧在
- PD 分离判定在 Python 侧重复实现,rtp_llm.pd_sep 存在两个事实来源 @
rtp_llm/cpp/model_rpc/model_rpc_client.py:56- 建议:优先让服务端把实际分流结果作为可观测数据回传(Prefill SERVER span 已能写入
rtp_llm.pd_sep),客户端只读取不重算;若必须保留客户端重算,请补一个跨语言一致性测试,并在注释中登记 C++ 侧文件与行号锚点。
- 建议:优先让服务端把实际分流结果作为可观测数据回传(Prefill SERVER span 已能写入
- wheel 声明的 opentelemetry 版本组合无 lock 或用例验证,失败模式为静默丢链路 @
rtp_llm/BUILD:702- 建议:补一条最小 py_test,在带 opentelemetry 的 lock 上真实构造 OTLP HTTP exporter 并
ForceFlush,把「版本组合可用」变成可回归断言;或把 exporter 升到与 sdk 同 minor(受protobuf==4.25约束时改为写明可用区间)并钉住opentelemetry-api/opentelemetry-proto。同时在rtp_llm/telemetry/README.md记录已验证组合。
- 建议:补一条最小 py_test,在带 opentelemetry 的 lock 上真实构造 OTLP HTTP exporter 并
- arch_config 新增 telemetry_test_deps() 形成需两仓同步的硬 Starlark 契约 @
arch_config/arch_select.bzl:185- 建议:合并前确认内部
arch_config已提供同签名实现,并在 PR 描述/提交信息中标注跨仓同步依赖与合并顺序;在函数注释中写明「本函数是 @arch_config 的必需导出,新增/改名需两侧同步」。
- 建议:合并前确认内部
- WORKSPACE 内联第三方 http_archive 绕过既有依赖分层与内源覆盖点 @
WORKSPACE:5- 建议:把该
http_archive移入deps/http.bzl的http_deps(),patch 标签改为@rtp_llm//patches/opentelemetry_cpp:...;WORKSPACE 只保留load与opentelemetry_cpp_deps()调用并仍置于http_deps()/git_deps()之后,即可保持「先声明优先」的版本选择结果不变。
- 建议:把该
- OTel STL ABI 模式仅在 rocm config 钉住,其他走 crosstool wrapper 的平台留有静默 ABI 分裂隐患 @
.bazelrc:361- 建议:把
--@io_opentelemetry_cpp//api:with_cxx_stdlib=2017上提到.bazelrc的公共build段一次性钉住(rocm 特例即可删除);若只能 per-config,请在WORKSPACE:5处注明「新增平台 config 必须显式钉 STL 模式」,并考虑加一个最小链接测试让 ABI 分裂在构建期而非部署期暴露。
- 建议:把
- RtpLLMOp 重复实现 RoleType→字符串映射,新增角色会静默落到 unknown @
rtp_llm/cpp/pybind/multi_gpu_gpt/RtpLLMOp.cc:353- 建议:在
RoleTypes.h紧邻roleTypeToString()增加roleTypeToTraceName()(或对其结果小写化),维持单一来源;去掉default分支改用穷举 switch 让新增 RoleType 在编译期暴露;若必须保留兜底则加 WARNING 日志,并补一个覆盖全部枚举值的表驱动单测。
- 建议:在
- SSE 流式路径把 GeneratorExit 归类为普通错误,且在该分支内继续 yield @
rtp_llm/frontend/frontend_server.py:330- 建议:把 :306 扩为
except (asyncio.CancelledError, GeneratorExit),或在except BaseException内对GeneratorExit复用error_type="Cancelled"并直接返回、不再执行 :349 的yield;补一条覆盖生成器被aclose()关闭、断言error.type == "Cancelled"的用例。
- 建议:把 :306 扩为
- route span 未把取消归一化为 Cancelled,客户端断连计入路由错误率 @
rtp_llm/server/backend_rpc_server_visitor.py:488- 建议:在 except 分支显式归一化:
isinstance(e, (asyncio.CancelledError, GeneratorExit))时置route_error_type = "Cancelled",其余非FtRuntimeException保持现有兜底;补一个取消用例断言finish收到的error_type。
- 建议:在 except 分支显式归一化:
- P→D CLIENT span 的阶段耗时属性在重试时跨 attempt 累加 @
rtp_llm/cpp/model_rpc/PrefillGenerateContext.cc:149- 建议:在写 span 属性前减去上一 attempt 的快照基线(在
remoteAllocateResource重建 guard 时记录stat_info基线),或把这三个属性改名为明确的累计语义并在文档中写明;kmonitor 侧若依赖累计口径请保留独立字段。补一条覆盖retry_times>0时属性值的用例。
- 建议:在写 span 属性前减去上一 attempt 的快照基线(在
- route span 新单测覆盖不足,Fake 丢弃 finish 且 fixture 类型错配决定了分支走向 @
rtp_llm/server/test/backend_rpc_server_visitor_test.py:117- 建议:
_FakeRouteSpan增加finish_calls计数(或改用MagicMock),让成功路径断言恰好 finish 一次;backend_role_list改为真实RoleType枚举使分支由业务语义决定;补齐纯master与纯domain_fallback用例并用assertIn(..., trace_attrs.RTP_LLM_ROUTE_SOURCE_VALUES)做取值域校验;异常断言收紧为FtRuntimeException;顺带确认「master 成功但地址列表为空」标记为master+domain_fallback是否符合预期语义。
- 建议:
- detach 结算依赖的 grpc.aio 终态语义只有 fake 覆盖 @
rtp_llm/cpp/model_rpc/test/model_rpc_client_test.py:971- 建议:补一个真实 grpc.aio 用例:开启遥测,服务端在 finished 帧后延迟发送 trailer,客户端收到 finished 帧后立即
aclose(),断言 CLIENT span 在未发生强制取消的前提下以 OK 结束且rpc.response.status_code == "OK",把该假设变成可回归的断言而非 fake 的自证。
- 建议:补一个真实 grpc.aio 用例:开启遥测,服务端在 finished 帧后延迟发送 trailer,客户端收到 finished 帧后立即
- 测试内 gRPC server 使用无界 Shutdown/Wait,失败会放大为整 target 超时 @
rtp_llm/cpp/model_rpc/test/RpcWriterCancellationTest.cc:216- 建议:改为
server->Shutdown(std::chrono::system_clock::now() + std::chrono::seconds(5)),并把ASSERT_TRUE(server_status.has_value())移到 shutdown 之前,与grpc_propagation_test.cc的有界 shutdown 约定保持一致。
- 建议:改为
- dash_sc telemetry 生命周期用例把 role/rank 接线整体 mock,fail-open 用例零断言 @
rtp_llm/dash_sc/test/app_test.py:231- 建议:保留现有「调用点接线」断言,另加一条打桩
bg_app.init_telemetry的用例断言其被以("dash_sc", 0)调用(或直接把本用例改为 patchinit_telemetry,使名称与断言一致);test_init_failure_is_fail_open用assertLogs断言降级告警。
- 建议:保留现有「调用点接线」断言,另加一条打桩
- 新增 beam 测试脚手架从未被调用,用量聚合的 beam 分支无任何 C++ 覆盖 @
rtp_llm/cpp/engine_base/stream/test/GenerateStreamTest.cc:96- 建议:补一个使用
createBeamStream()的用例,断言 beam 场景下的聚合结果(至少覆盖hasNumBeams()为真、num_return_sequences为 1 与 >1 两种情况);若确认暂不在 C++ 侧覆盖该分支,则删除createBeamStream()与未使用的形参,避免留下误导性死脚手架。
- 建议:补一个使用
P3
- request_id 双键与 route_source 取值硬编码,绕过 attributes 单一注册表 @
rtp_llm/frontend/frontend_server.py:456- 建议:三处统一改用
trace_attrs.REQUEST_ID/RTP_LLM_REQUEST_ID;把 6 个route_source取值提升为attributes.py命名常量,或至少用RTP_LLM_ROUTE_SOURCE_VALUES在单测中做取值域断言;并考虑给 parity 测试增加「生产代码不得出现裸 attribute 字面量」的静态扫描。
- 建议:三处统一改用
- settlement task 被事件循环收尾取消时 CLIENT span 永不 end @
rtp_llm/cpp/model_rpc/model_rpc_client.py:134- 建议:在
_settle_client_span_after_rpc外层加except asyncio.CancelledError:分支,在 re-raise 之前先client_span.finish(error_type="Cancelled")(finish已幂等),使收尾期取消也能产出一条可解释的 span;同时把游离 task 收敛到 client 实例级集合,便于close()时统一处理。
- 建议:在
- 抢占终结路径未传 attempt_error_override,被抢占的 P→D CLIENT span 可能被标为成功 @
rtp_llm/cpp/model_rpc/PrefillGenerateContext.cc:302- 建议:改为
closeGrpcStream(ErrorCodeToString(ErrorCode::PRIORITY_PREEMPTED)),与错误宏路径保持同一约定;并补一条「Finish()==OK + 抢占终结」的用例断言 span 为 ERROR。
- 建议:改为
- localGenerate 对 server_context 未判空,与同一 diff 内相邻两处写法不一致 @
rtp_llm/cpp/model_rpc/DecodeRpcServer.cc:326- 建议:两处统一改为调用已有的
decode_context.isRequestCancelled()(顺带覆盖cancel_state分支);并给prepareGenerateContext的读失败分支补上同格式 WARNING,此时request_key未赋值,可用server_context->peer()作为标识,与宏内pending peer口径一致。
- 建议:两处统一改为调用已有的
- getRpcConnection 中 try/catch 包裹的是已判空的 host 解引用,属不可达防御代码 @
rtp_llm/cpp/model_rpc/PrefillRpcServer.cc:272- 建议:删除 try/catch 直接赋值;函数入口已有的清零逻辑已保证失败路径不会残留上一次连接的陈旧地址。
- markPriorityPreemptionCanceled 传入空 stream 时不再清理 running_streams_ 条目 @
rtp_llm/cpp/model_rpc/RpcServerRuntimeMeta.h:152- 建议:在
stream == nullptr分支保留原有的无条件 erase(overlay 已承载 task_info,不会丢信息);对「命中 running 条目但 stream 不匹配/为空」的两处 return 加RTP_LLM_LOG_WARNING(含 request_id),与resolveBatchId的告警风格对齐,并补一条「nullptr stream + 存在 running 条目」的单测锁定期望行为。
- 建议:在
- 用字面量 getattr 读取动态挂载的 rtp_error_code @
rtp_llm/server/backend_rpc_server_visitor.py:484- 建议:把
rtp_error_code提升为FtRuntimeException的显式可选字段(__init__默认None),调用处改为属性直读;或先取值再做isinstance(value, int)判断后写入 span,使 telemetry 代码与其他 fail-open 分支保持同样「绝不改变异常语义」的强度。
- 建议:把
- GRPC_RET_IF_ERROR 用魔法字符串 "0" 判断请求是否已获得 request_id @
rtp_llm/cpp/model_rpc/DecodeRpcServer.cc:42- 建议:改为显式的具名常量(如
kUnassignedRequestKey)或用decode_context.request_id == 0等语义更直接的判定,并在makeRequestKey附近注释登记未赋值时的初值约定。
- 建议:改为显式的具名常量(如
- 空 content 帧的 token 增量被吞掉,前端 TPOT 分母偏小 @
rtp_llm/frontend/frontend_server.py:658- 建议:把游标推进与记录绑定:仅在实际记录时更新
last_observed_output_tokens,或对visible_outputs == 0的帧把observed_token_delta暂存并累加到下一次可见帧一起上报。
- 建议:把游标推进与记录绑定:仅在实际记录时更新
- 永真的负向日志断言使 TestLogCapture 形同虚设 @
rtp_llm/cpp/model_rpc/test/RpcWriterCancellationTest.cc:273- 建议:删除该断言与
TestLogCapture构造,或改为断言生产代码真实输出的日志文本,避免留下看似有覆盖实则永真的断言。
- 建议:删除该断言与
- in_memory_span_exporter 被加入包级共享 test_deps,实际只有一个 cc_test 使用 @
rtp_llm/cpp/model_rpc/test/BUILD:51- 建议:把该依赖从共享
test_deps移到真正需要它的单个 cc_test 的deps = test_deps + [...]中,保持包级共享列表最小。
- 建议:把该依赖从共享
- 新增 telemetry 测试包未沿用仓库统一的 cc_test_wrapper @
rtp_llm/cpp/telemetry/test/BUILD:3- 建议:按仓库约定改用
cc_test_wrapper;若确有必须使用原生cc_test的理由(如保持依赖轻量、无需 torch 静态链接),请在 BUILD 顶部注释说明该例外。
- 建议:按仓库约定改用
- OTel 裁剪 patch 缺少动机与撤销条件说明,并使上游 metrics exporter 目标不可构建 @
patches/opentelemetry_cpp/0001-trace-only-otlp-recordable.patch:1- 建议:在 patch 顶部补 4-6 行说明:为何裁剪(vendored protobuf 与 metrics proto 生成冲突的具体现象)、影响面(metrics exporter 目标不可用、本仓不使用)、撤销条件(升级到哪个上游版本或改用哪种 proto 生成方式后可删除),并在
WORKSPACE的patches行加一句指向该说明的注释。
- 建议:在 patch 顶部补 4-6 行说明:为何裁剪(vendored protobuf 与 metrics proto 生成冲突的具体现象)、影响面(metrics exporter 目标不可用、本仓不使用)、撤销条件(升级到哪个上游版本或改用哪种 proto 生成方式后可删除),并在
- protobuf 黑名单过滤把 map key 复制成魔法字符串 @
3rdparty/protobuf/BUILD:890- 建议:把被排除的 key 提为文件级常量(如
_PLUGIN_PROTO_KEY = "compiler_plugin")并在推导前断言其存在于WELL_KNOWN_PROTO_MAP,使上游改名在 load 阶段即暴露。
- 建议:把被排除的 key 提为文件级常量(如
- 端点校验用例依赖 span 导出顺序做下标索引 @
rtp_llm/cpp/telemetry/test/telemetry_test.cc:516- 建议:改为按
endpoint_i名称建立 map 后查找断言,与同目录既有写法统一,使断言与导出顺序解耦并让失败信息直接定位到具体 case。
- 建议:改为按
- QueryConverter.cc 仅有一处与本 PR 无关的空行删除 @
rtp_llm/cpp/model_rpc/QueryConverter.cc:195- 建议:还原该空行,使本 PR 的 diff 文件列表只包含与追踪改造相关的文件;若确为 clang-format 顺带产生,建议单独提一个格式化 commit。
Checklist Findings (23 fail / 54 total)
General Principles Checklist
- [6.1] Architecture — 依赖方向:无循环依赖/跨层惊喜 → issue
arch_config 新增 telemetry_test_deps() 形成需两仓同步的硬 Starlark 契约
已核对共 4 个包从@arch_config//:arch_select.bzlload 该符号:rtp_llm/cpp/model_rpc/test/BUILD:2、rtp_llm/telemetry/test/BUILD:1、rtp_llm/frontend/test/BUILD:1、rtp_llm/dash_sc/test/BUILD:1。内部构建通过--override_repository替换@arch_config,若内部arch_select.bzl未同步定义同名函数,这些包会在 Bazel load 阶段直接name 'telemetry_test_deps' is not defined硬失败(不同于tracing.py的运行时降级),且回滚需同时回滚两个仓库。 - [6.1] Architecture — 兼容性:外部 HTTP/RPC API、持久数据、配置、环境迁移安全 → issue
arch_config 新增 telemetry_test_deps() 形成需两仓同步的硬 Starlark 契约
已核对共 4 个包从@arch_config//:arch_select.bzlload 该符号:rtp_llm/cpp/model_rpc/test/BUILD:2、rtp_llm/telemetry/test/BUILD:1、rtp_llm/frontend/test/BUILD:1、rtp_llm/dash_sc/test/BUILD:1。内部构建通过--override_repository替换@arch_config,若内部arch_select.bzl未同步定义同名函数,这些包会在 Bazel load 阶段直接name 'telemetry_test_deps' is not defined硬失败(不同于tracing.py的运行时降级),且回滚需同时回滚两个仓库。 - [6.1] Architecture — 分层边界:新概念在正确层级,不泄漏内部 → issue
WORKSPACE 内联第三方 http_archive 绕过既有依赖分层与内源覆盖点
io_opentelemetry_cpp是 WORKSPACE 中唯一直接声明的http_archive(为此第 3 行专门 loadhttp_archive,全文仅此一处;其余为两个local_repository+ 各*_deps()调用),而全部三方仓集中在@rtp_deps的deps/http.bzl/deps/git.bzl。内源构建通过--override_repository替换@rtp_deps来重定向或重钉三方依赖,WORKSPACE 顶层声明拿不到这个覆盖点:内源/PPU/ROCm 若需换镜像源或版本,只能直接改这个与开源同步的文件。deps/git.bzl已证明在@rtp_deps内用@rtp_llm//...标签引用 patch 完全可行,因此当前位置不存在技术必要性。 - [6.1] Architecture — 可观测性:日志/指标/超时可操作、非噪声 → issue
空 content 帧的 token 增量被吞掉,前端 TPOT 分母偏小
:649-656 先无条件把last_observed_output_tokens推进到最新的output_tokens,再在 :658 用if visible_outputs > 0:决定是否record_frontend_output_tokens(visible_tokens)。当某一帧有 token 增长但无可见输出(例如仅含 reasoning/空 content 的增量帧),该帧的增量既不会被记录、也无法在下一帧补回(游标已推进),前端 TPOT 的 token 计数被系统性低估,进而抬高rtp_llm.frontend.time_per_output_token_ms。 - [6.1] Architecture — 回滚路径:风险行为存在运维回滚手段 → issue
OTel 裁剪 patch 缺少动机与撤销条件说明,并使上游 metrics exporter 目标不可构建
该 patch 从上游exporters/otlp:otlp_recordable中删除otlp_metric_utils.cc/.h、otlp_preferred_temporality.h与metrics_service_proto_cc(三处 hunk 均只做删除),而同 BUILD 的otlp_http_metric_exporter/otlp_grpc_metric_exporter依赖这些源文件,patch 之后无法构建。patch 文件本身无任何头部说明,WORKSPACE:11与空的patches/opentelemetry_cpp/BUILD也没有注释;相比之下同一 PR 的.bazelrc:354-360对 ROCm STL 固定写了详尽的 WHY。下次升级 OTel 时,维护者无法判断该裁剪是为规避 vendored protobuf 的 metrics proto 生成,还是已可撤销。 - [6.1] Architecture — 状态不变量:创建/更新/失败/重试/回滚路径有效 → issue
markPriorityPreemptionCanceled 传入空 stream 时不再清理 running_streams_ 条目
新实现要求running != end && has_stream_snapshot && running->second.stream == stream才 erase(:152-155),else if (has_stream_snapshot)只更新 overlay(:156-161)。当stream == nullptr(PrefillGenerateContext.cc:304-323的finalized_stream可为空)且该 request_id 仍有 running 条目时,两个分支都不执行,条目永久驻留:running_streams_没有finished_streams_那样的容量淘汰,会持续出现在getEngineScheduleInfo()的 running 列表中虚增上报负载。dequeue(:183-185)在 stream 身份不匹配时同样静默 return,而同文件resolveBatchId在类似不一致场景下会打 WARNING。按当前调用链这些组合看似不可达,但不变量已从「实现保证 - [6.1] Architecture — 错误语义:fail-fast/retry/fallback/silent 行为显式 → issue
用字面量 getattr 读取动态挂载的 rtp_error_code
:484 用int(getattr(e, "rtp_error_code", e.exception_type))读取一个仅在 :476 动态赋值的属性,而同文件 :168-169 的既有注释明确写着「Use isinstance instead of getattr duck-typing」;FtRuntimeException.__init__也未声明该字段。已确认其唯一来源FlexlbResponse.error_code是Optional[int],因此当前不存在int()抛异常覆盖原始路由异常的风险,但这一安全性完全依赖「未来没人把非 int 赋给该属性」。 - [6.1] Quality — Mega-PR 已拆分为独立变更 → issue
enqueue 等待时长单位由微秒改为毫秒,WorkerStatus 对外取值缩小约 1000 倍
enqueue原先把wait_time_us直接作为makeTaskInfo的waiting_time_ms实参,现改为time_info.wait_time_us / 1000。修正方向正确(captureStreamRuntimeSnapshot(:244)一直是/1000,字段名即 ms),但该字段经LocalRpcServer.cc写入 WorkerStatus proto 的 running 任务列表,被rtp_llm/server/worker_status.py(注释写明用于主控判断 server 是否 hang)与 FlexLB 的waitingTimeMs消费。running 条目的对外数值一次性缩小 1000 倍,任何按旧口径标定的 hang 判定阈值、看板与告警基线都会随之改变,而本 PR 主题是 OTel 追踪,未见配套的下游确认,也未拆为独立 commit。 - [6.1] Quality — PR description 说明动机与设计 → issue
OTel 裁剪 patch 缺少动机与撤销条件说明,并使上游 metrics exporter 目标不可构建
该 patch 从上游exporters/otlp:otlp_recordable中删除otlp_metric_utils.cc/.h、otlp_preferred_temporality.h与metrics_service_proto_cc(三处 hunk 均只做删除),而同 BUILD 的otlp_http_metric_exporter/otlp_grpc_metric_exporter依赖这些源文件,patch 之后无法构建。patch 文件本身无任何头部说明,WORKSPACE:11与空的patches/opentelemetry_cpp/BUILD也没有注释;相比之下同一 PR 的.bazelrc:354-360对 ROCm STL 固定写了详尽的 WHY。下次升级 OTel 时,维护者无法判断该裁剪是为规避 vendored protobuf 的 metrics proto 生成,还是已可撤销。 - [6.1] Quality — 逻辑变更未混入无关格式化 → issue
QueryConverter.cc 仅有一处与本 PR 无关的空行删除
该文件在本 PR 中的唯一改动是end_think_token_ids循环(:193-195)与role_addrs循环(:196)之间的空行被删除,与 OTel 追踪主题无关;同一函数 :199-200 仍保留同类空行,因此也不是统一的格式化收敛。这类无关改动会让该文件出现在 diff 文件列表与 high_risk_files 路由中,抬高评审面并干扰后续git blame。 - [6.1] Software Engineering — DRY:重复非平凡逻辑被抽取或显式复用 → issue
protobuf 黑名单过滤把 map key 复制成魔法字符串
blacklisted_protos = [proto[0] + "_proto" for proto in WELL_KNOWN_PROTO_MAP.items() if proto[0] != "compiler_plugin"](:890-894)把WELL_KNOWN_PROTO_MAP的一个 key 以字面量形式复制到过滤条件里。修复方向本身正确且注释(:883-889)解释清楚,但若上游 vendored 的 map key 改名,该过滤会静默失效(不再排除compiler_plugin,或反之误排除),失败形态是 protoc 重复生成/缺失符号的链接错误,排查成本高于一次显式校验。 - [6.1] Software Engineering — ISP:调用方不依赖无关大接口 → issue
in_memory_span_exporter 被加入包级共享 test_deps,实际只有一个 cc_test 使用
@io_opentelemetry_cpp//exporters/memory:in_memory_span_exporter被加进包级共享变量test_deps(:39-52),而该列表被local_rpc_server_test、query_converter_test、broadcast_manager_test、prefill_rpc_server_test等 9 个 GPU cc_test 复用;在cpp/model_rpc/test/下检索InMemorySpanExporter,cc_test 中仅RpcWriterCancellationTest.cc使用。结果是无关测试目标都被牵连进 OTel 导出器的编译与链接,增大构建面并把 OTel 侧的构建问题扩散到与追踪无关的用例上。 - [6.1] Software Engineering — KISS/YAGNI:无投机性抽象 → issue
in_memory_span_exporter 被加入包级共享 test_deps,实际只有一个 cc_test 使用
@io_opentelemetry_cpp//exporters/memory:in_memory_span_exporter被加进包级共享变量test_deps(:39-52),而该列表被local_rpc_server_test、query_converter_test、broadcast_manager_test、prefill_rpc_server_test等 9 个 GPU cc_test 复用;在cpp/model_rpc/test/下检索InMemorySpanExporter,cc_test 中仅RpcWriterCancellationTest.cc使用。结果是无关测试目标都被牵连进 OTel 导出器的编译与链接,增大构建面并把 OTel 侧的构建问题扩散到与追踪无关的用例上。 - [6.1] Software Engineering — OCP:本地扩展点优先于修改中心逻辑 → issue
RtpLLMOp 重复实现 RoleType→字符串映射,新增角色会静默落到 unknown
:353-372 手写了一份RoleType→小写字符串 switch(pdfusion/prefill/decode/vit/frontend,default: "unknown"),而rtp_llm/cpp/config/RoleTypes.h:14-29的roleTypeToString()已是同一枚举的既有唯一映射,并已被ConfigModules.cc、LocalRpcServer.cc复用。该 role 直接决定service.name = "rtp_llm_" + role与rtp_llm.role资源属性。由于带default分支,RoleTypes.h:6-12新增枚举值时编译器不会给出-Wswitch提示,维护者按惯例只改RoleTypes.h,该副本会静默落到unknown——后果是该角色整个组件在拓扑视图里显示为rtp_llm_unknown,且当前无任何单测断言该映射。 - [6.1] Software Engineering — SRP:模块/类职责单一 → issue
PhaseTiming 装配在 C++ 三处、span 结算序列在 Python 五处重复,且含一处死赋值
LocalRpcServer.cc:212-249、PrefillRpcServer.cc:761-792、DecodeRpcServer.cc:1414-1442三处PhaseSpanSynthesisScope回调结构逐字一致:同样的三重判空、同样把TimeInfo的 8 个字段逐一搬进PhaseTiming、同样计算request_ok/error_type,差异仅为PhaseRole与 error_type 分类方式。Python 侧同理:model_rpc_client.py的 :978/:994/:1038/:1063/:1092 共五处重复_record_client_rpc_status+_record_client_span_usage+_record_client_span_latency+finish序列,并各自重复client_settlement_task is None与rpc_status is None的判空组合,enqueue因此膨胀到约 26 - [6.1] Tests — 分布式/跨平台变更有对应覆盖 → issue
OTel STL ABI 模式仅在 rocm config 钉住,其他走 crosstool wrapper 的平台留有静默 ABI 分裂隐患
WORKSPACE:5为全部平台无条件引入@io_opentelemetry_cpp,其默认 STL 模式best展开为带括号的-DOPENTELEMETRY_STL_VERSION=(__cplusplus/100)。.bazelrc:354-360已记录该值经shlex.quote后被 hipcc 分支静默丢弃,导致 nostd/std ABI 分裂与 dlopen 期 undefined symbol,并针对性加了build:rocm --@io_opentelemetry_cpp//api:with_cxx_stdlib=2017。但脆弱的默认值是全局的、修复却是 per-config 的:已核对3rdparty/cuda_config/crosstool/clang/bin/crosstool_wrapper_driver_is_not_gcc.tpl:284的 nvcc 分支存在同一pipes.quote处理,ppu 等平台亦然。当前 telemetry 目标只含.cc,风险潜伏;一旦有.cuTU - [6.1] Tests — 新逻辑有聚焦单测 + 相关集成/smoke 测试 → issue
端点校验用例依赖 span 导出顺序做下标索引
:507-511 依次创建endpoint_i子 span,:516 断言spans.size() == cases.size() + 1,随后 :517-526 用spans[i]与cases[i]一一对应。这依赖 in-memory exporter 的导出顺序与创建顺序严格一致(父 span 排在最后),一旦 BSP 批次或 exporter 实现调整,用例会以「属性 key 不存在」的形式误报,且失败信息无法指向具体 case。同目录grpc_propagation_test.cc已采用按 span 名查找的写法。 - [6.1] Tests — 边界 case 覆盖(空、单元素、最大值) → issue
markPriorityPreemptionCanceled 传入空 stream 时不再清理 running_streams_ 条目
新实现要求running != end && has_stream_snapshot && running->second.stream == stream才 erase(:152-155),else if (has_stream_snapshot)只更新 overlay(:156-161)。当stream == nullptr(PrefillGenerateContext.cc:304-323的finalized_stream可为空)且该 request_id 仍有 running 条目时,两个分支都不执行,条目永久驻留:running_streams_没有finished_streams_那样的容量淘汰,会持续出现在getEngineScheduleInfo()的 running 列表中虚增上报负载。dequeue(:183-185)在 stream 身份不匹配时同样静默 return,而同文件resolveBatchId在类似不一致场景下会打 WARNING。按当前调用链这些组合看似不可达,但不变量已从「实现保证
RTP-LLM Checklist
- [I] 代码质量 — 删除或重命名内部 file、registry entry、model name、metric enum、op binding、plugin symbol 时,必须全仓搜索消费者,并提供替代实现、迁移说明或 smoke 覆盖;只有暴露到 HTTP/RPC/config/persisted format 时才按外部兼容性处理 → issue
OTel 裁剪 patch 缺少动机与撤销条件说明,并使上游 metrics exporter 目标不可构建
该 patch 从上游exporters/otlp:otlp_recordable中删除otlp_metric_utils.cc/.h、otlp_preferred_temporality.h与metrics_service_proto_cc(三处 hunk 均只做删除),而同 BUILD 的otlp_http_metric_exporter/otlp_grpc_metric_exporter依赖这些源文件,patch 之后无法构建。patch 文件本身无任何头部说明,WORKSPACE:11与空的patches/opentelemetry_cpp/BUILD也没有注释;相比之下同一 PR 的.bazelrc:354-360对 ROCm STL 固定写了详尽的 WHY。下次升级 OTel 时,维护者无法判断该裁剪是为规避 vendored protobuf 的 metrics proto 生成,还是已可撤销。 - [I] 代码质量 — 同一功能用统一工具函数 → issue
端点校验用例依赖 span 导出顺序做下标索引
:507-511 依次创建endpoint_i子 span,:516 断言spans.size() == cases.size() + 1,随后 :517-526 用spans[i]与cases[i]一一对应。这依赖 in-memory exporter 的导出顺序与创建顺序严格一致(父 span 排在最后),一旦 BSP 批次或 exporter 实现调整,用例会以「属性 key 不存在」的形式误报,且失败信息无法指向具体 case。同目录grpc_propagation_test.cc已采用按 span 名查找的写法。
Python Static-First Checklist
- [P.A] 静态结构与类型纪律 — 字符串分发用 Enum/Literal → issue
request_id 双键与 route_source 取值硬编码,绕过 attributes 单一注册表
attributes.py:103-110声明本节是 single-source registry 并定义REQUEST_ID = "request_id"/RTP_LLM_REQUEST_ID = "rtp_llm.request_id",model_rpc_client.py:928-931正确使用常量;但frontend_server.py:456-457与backend_rpc_server_visitor.py:375-376各自硬编码字面量,测试(:373-374)也断言字面量。这两个键是链路检索索引键,改常量只会让 parity 测试失败而这两处静默保持旧值,故障形态是「上报成功但检索不到」。route_source的 6 个取值同样以字面量散落在 :377/:408/:428/:453-461,而RTP_LLM_ROUTE_SOURCE_VALUES全仓零引用;trace_attributes_parity_test.py只做 C++⊆Python 的键名子集校验,无法发现裸字面量。 - [P.A] 静态结构与类型纪律 — 禁止 getattr/setattr literal 访问 → issue
用字面量 getattr 读取动态挂载的 rtp_error_code
:484 用int(getattr(e, "rtp_error_code", e.exception_type))读取一个仅在 :476 动态赋值的属性,而同文件 :168-169 的既有注释明确写着「Use isinstance instead of getattr duck-typing」;FtRuntimeException.__init__也未声明该字段。已确认其唯一来源FlexlbResponse.error_code是Optional[int],因此当前不存在int()抛异常覆盖原始路由异常的风险,但这一安全性完全依赖「未来没人把非 int 赋给该属性」。 - [P.G] 测试规范 — mock/fake/stub 不得替代本次声称覆盖的生产边界 → issue
dash_sc telemetry 生命周期用例把 role/rank 接线整体 mock,fail-open 用例零断言
test_start_initializes_dash_role_and_shutdowns用patch.object(bg_app, "_init_trace_telemetry")(:231)/_shutdown_trace_telemetry(:233)把被测函数整体替换为 Mock,只断言init_trace.assert_called_once_with()(:237)。生产实现rtp_llm/dash_sc/app.py:66-72中_init_trace_telemetry()调用init_telemetry("dash_sc", 0),该 role 字符串直接决定导出侧service.name的拓扑归属,而用例名称虽声称覆盖 "dash role",却没有任何断言触达该参数。test_init_failure_is_fail_open(:241-245)只验证不抛异常,未断言logging.warning降级告警确有输出,即实现改成静默pass也能通过。
Strengths
- span 析构顺序经过刻意设计:
trace_span_guard声明于error_status之后、pd_client_span_guard作为派生成员先于基类 guard 析构、PhaseSpanSynthesisScope声明于 context 之后,共同保证读取到已落定的终态且子 span 不晚于父 span 结束,并覆盖CHECK_ERROR_STATUS/EXECUTE_STAGE_FUNC的全部提前 return 与异常展开路径。 - 遥测全程 fail-open:guard 析构与合成回调
noexcept吞异常,setUsageTokenAttributes(RpcTraceHelper.h:276-287)对非正 token 直接跳过,synthesizePhaseSpans对非法时序跳过而非钳制,_record_client_span_usage在input_len不一致时整体放弃写入,宁缺勿错。 RpcServerRuntimeMeta锁序与身份校验改进扎实:enqueue/dequeue/markPriorityPreemptionCanceled把captureStreamRuntimeSnapshot(内部访问 stream 互斥量)提到read_write_lock_之外消除锁序隐患;commitDequeueSnapshot在写锁内二次校验 stream 身份闭合 check-then-act 窗口;dequeue(:176-178)同时修掉空 stream 解引用;三处重复的 task_info 填充收敛为一对静态函数。GRPC_RET_IF_ERROR补上do { } while (false)包裹并让code/msg各求值一次(DecodeRpcServer.cc:36-53),消除悬挂 else 与重复求值,并补上带 request 标识的 WARNING。- 新增逻辑刻意做成可测纯函数:
phaseErrorType、generateRequestReadFailureStatus、_engine_reported_finished、_request_completed_normally均无副作用且配有边界用例;closeGrpcStream的幂等语义有精确回归(finish_calls == 1)。 grpc_propagation_test.cc走真实进程内 gRPC 并调用生产入口,覆盖「外部直写 traceparent」与「无 traceparent 必须新建 root」;TearDown 使用有界 shutdown 并注释了动机(:104-111)。- 依赖卫生良好:
rtp_llm/cpp/model_rpc/BUILD:88-90显式声明rtp_llm:telemetry并注释「不依赖消费者的传递闭包」;telemetry 仅链入libth_transformer.so,进程内单 provider 成立;.bazelrc:354-360为 ROCm 下 OTel STL ABI 分裂写了完整 WHY。 route_ips是严格行为保持型改造:span 刻意创建在主动限流检查之前使限流路径可见,except BaseException收口后原样raise,路由分支与 kmonitor 上报一字未改。
| await server.start() | ||
| channel = grpc.aio.insecure_channel(f"127.0.0.1:{port}") | ||
|
|
||
| from opentelemetry.sdk.trace.export.in_memory_span_exporter import ( |
There was a problem hiding this comment.
[P2] opentelemetry 可用性契约自相矛盾,真实 gRPC 链路用例缺少跳过守卫
test_traceparent_crosses_real_grpc_boundary 在 :793 无条件 from opentelemetry.sdk...import InMemorySpanExporter,并在 :798-801 断言 init_telemetry_for_test(...) 为真;全文件检索 skipUnless/skipTest/OTEL_AVAILABLE 零命中。该 py_test 的 OTel 运行时只由 test/BUILD:32 的 telemetry_test_deps() 提供,而 arch_config/arch_select.bzl:185-190 返回 [] 并自述「公共 pip lock 没有 opentelemetry」。同类导入的 4 个兄弟文件均有守卫:frontend/test/frontend_server_test.py:117-118 等六处、dash_sc/test/inference/servicer_test.py:3764、`dash_sc/test/prox...
建议: 对齐已有约定:把该 SDK import 放进 try/except ImportError 并 self.skipTest(...),或给 ModelRpcClientGrpcMetadataTest 中依赖 SDK 的用例加 @unittest.skipUnless(tracing.OTEL_AVAILABLE, ...);test_trace_disabled_full_consumer_waits_for_real_grpc_terminal 不依赖 SDK,应保持无守卫以避免公共构建退化为 all-skip。同时修正 arch_select.bzl:186-189 与 rtp_llm/BUILD:453-455 的注释,使三处对「运行时来源」的描述自洽。
| reportEarlyFinishTask(decode_context, | ||
| static_cast<int64_t>(error_info.code()), | ||
| "decode load cache from prefill failed: " + error_info.ToString()); | ||
| decode_context.error_status = grpc::Status(transErrorCodeToGrpc(error_info.code()), error_info.ToString()); |
There was a problem hiding this comment.
[P2] Decode 加载 KV 失败回传的 gRPC status 改为错误码映射,会覆写 Prefill 侧精确 ErrorCode
该行由固定 grpc::StatusCode::INTERNAL 改为 grpc::Status(transErrorCodeToGrpc(error_info.code()), ...)。按 RpcErrorCode.h:12-31,MALLOC_FAILED/DECODE_MALLOC_FAILED/PRIORITY_PREEMPTED → RESOURCE_EXHAUSTED,LOAD_CACHE_TIMEOUT 等 → DEADLINE_EXCEEDED,CANCELLED → CANCELLED。Prefill 侧 remoteLoadCacheEnd(PrefillRpcServer.cc:493/501)刚从 PB 解出精确 error_code 并传入 CLIENT_GRPC_RET_IF_ERROR,而宏体 :147-149 对 RESOURCE_EXHAUSTED 硬编码 new_error_code = DECODE_MALLOC_FAILED——该分支在旧行为下对本路径不可达,现在可达,会把 ...
建议: 在 PR 描述中把该 P→D 终态契约变更单列为独立行为变更,并列出受影响指标(cancel_qps、error_qps、error_code 分布);补一条用例固化「decode 返回 RESOURCE_EXHAUSTED / DEADLINE_EXCEEDED 时 Prefill 最终上报的 ErrorCode」。若目的仅是让 span 的 error.type 更精确,更小的改法是保留 INTERNAL、只把 error_info.code() 写进 span 属性——phaseErrorType 已能独立从 error_info 识别 DependencyFailure。
| auto new_task = makeTaskInfo(TaskIdentity{identity.request_id, batch_id}, | ||
| stream->prefixLength(), | ||
| stream->inputLength(), | ||
| time_info.wait_time_us / 1000); |
There was a problem hiding this comment.
[P2] enqueue 等待时长单位由微秒改为毫秒,WorkerStatus 对外取值缩小约 1000 倍
enqueue 原先把 wait_time_us 直接作为 makeTaskInfo 的 waiting_time_ms 实参,现改为 time_info.wait_time_us / 1000。修正方向正确(captureStreamRuntimeSnapshot(:244)一直是 /1000,字段名即 ms),但该字段经 LocalRpcServer.cc 写入 WorkerStatus proto 的 running 任务列表,被 rtp_llm/server/worker_status.py(注释写明用于主控判断 server 是否 hang)与 FlexLB 的 waitingTimeMs 消费。running 条目的对外数值一次性缩小 1000 倍,任何按旧口径标定的 hang 判定阈值、看板与告警基线都会随之改变,而本 PR 主题是 OTel 追踪,未见配套的下游确认,也未拆为独立 commit。
建议: 拆为独立 commit,或至少在 PR 描述与发布说明中单列为「行为变更:WorkerStatus running 条目 waiting_time_ms 由 us 修正为 ms」,并与 FlexLB / 主控 hang 判定的维护方确认阈值;若需灰度,可先并行暴露新字段再切换旧字段。
Checklist: [6.1] Mega-PR 已拆分为独立变更
| telemetry::synthesizePhaseSpans( | ||
| prefill_context.trace_span_guard->sharedSpan(), phase_timing, telemetry::PhaseRole::Prefill, request_ok); | ||
| if (request_ok && time_info.generation_done) { | ||
| telemetry::setUsageTokenAttributes( |
There was a problem hiding this comment.
[P2] 同一 trace 内 Prefill 与 Decode SERVER span 各自完整申报 gen_ai.usage.*
Prefill SERVER span 在 :789-790 用本地 stream 的 inputLength()/outputTokenLen() 调 setUsageTokenAttributes;Decode SERVER span 在 DecodeRpcServer.cc:1439-1440 用 decode 侧 stream 写入同一组 key,而 RpcTraceHelper.h:281-285 一次写入 input/prompt/output/completion/total 五个键。startRpcServerSpan 会从 server metadata 提取远端 parent,因此 PD 分离下两个 span 处在同一 trace(Decode SERVER 是 P→D CLIENT span 的子节点),prompt token 被两个 span 各自完整申报一次;而 Prefill stream 只产出首 token,其 output_tokens 无法代表整个请求。Python CLIENT span(model_rpc_cl...
建议: 明确 gen_ai.usage.* 的唯一权威写入点(建议只在面向客户端的请求入口 span 上写),PD 节点级数值改用 rtp_llm.* 私有 key,或在 Prefill/Decode span 上补一个节点作用域标识属性;在 rtp_llm/telemetry/README.md 写明聚合口径,并补一条 PD 场景断言,验证同一 trace 内 gen_ai.usage.input_tokens 不会被重复申报。
| @@ -0,0 +1,42 @@ | |||
| load("//:def.bzl", "copts") | |||
|
|
|||
| cc_test( | |||
There was a problem hiding this comment.
[P3] 新增 telemetry 测试包未沿用仓库统一的 cc_test_wrapper
该文件仅 load("//:def.bzl", "copts") 并直接使用原生 cc_test(:3/:17/:32),而 rtp_llm/cpp/** 下现有 19 个测试包一律 cc_test = cc_test_wrapper(含同 PR 修改的 cpp/model_rpc/test/BUILD:1,6)。差异会导致该包的链接方式与 test 包装脚本与其余测试目标不一致,后续若在 wrapper 内统一注入环境或诊断能力,本包会被遗漏。
建议: 按仓库约定改用 cc_test_wrapper;若确有必须使用原生 cc_test 的理由(如保持依赖轻量、无需 torch 静态链接),请在 BUILD 顶部注释说明该例外。
| @@ -0,0 +1,29 @@ | |||
| diff --git a/exporters/otlp/BUILD b/exporters/otlp/BUILD | |||
There was a problem hiding this comment.
[P3] OTel 裁剪 patch 缺少动机与撤销条件说明,并使上游 metrics exporter 目标不可构建
该 patch 从上游 exporters/otlp:otlp_recordable 中删除 otlp_metric_utils.cc/.h、otlp_preferred_temporality.h 与 metrics_service_proto_cc(三处 hunk 均只做删除),而同 BUILD 的 otlp_http_metric_exporter/otlp_grpc_metric_exporter 依赖这些源文件,patch 之后无法构建。patch 文件本身无任何头部说明,WORKSPACE:11 与空的 patches/opentelemetry_cpp/BUILD 也没有注释;相比之下同一 PR 的 .bazelrc:354-360 对 ROCm STL 固定写了详尽的 WHY。下次升级 OTel 时,维护者无法判断该裁剪是为规避 vendored protobuf 的 metrics proto 生成,还是已可撤销。
建议: 在 patch 顶部补 4-6 行说明:为何裁剪(vendored protobuf 与 metrics proto 生成冲突的具体现象)、影响面(metrics exporter 目标不可用、本仓不使用)、撤销条件(升级到哪个上游版本或改用哪种 proto 生成方式后可删除),并在 WORKSPACE 的 patches 行加一句指向该说明的注释。
Checklist: [6.1] 回滚路径:风险行为存在运维回滚手段;[6.1] PR description 说明动机与设计;[I] 删除或重命名内部 file、registry entry、model name、metric enum、op binding、plugin symbol 时,必须全仓搜索消费者,并提供替代实现、迁移说明或 smoke 覆盖;只有暴露到 HTTP/RPC/config/persisted format 时才按外部兼容性处理
| # emit a second copy of their C++ code. compiler/plugin.pb.cc is linked into | ||
| # :protoc_lib, not :protobuf, so runtime consumers must still be allowed to | ||
| # generate it. | ||
| blacklisted_protos = [ |
There was a problem hiding this comment.
[P3] protobuf 黑名单过滤把 map key 复制成魔法字符串
blacklisted_protos = [proto[0] + "_proto" for proto in WELL_KNOWN_PROTO_MAP.items() if proto[0] != "compiler_plugin"](:890-894)把 WELL_KNOWN_PROTO_MAP 的一个 key 以字面量形式复制到过滤条件里。修复方向本身正确且注释(:883-889)解释清楚,但若上游 vendored 的 map key 改名,该过滤会静默失效(不再排除 compiler_plugin,或反之误排除),失败形态是 protoc 重复生成/缺失符号的链接错误,排查成本高于一次显式校验。
建议: 把被排除的 key 提为文件级常量(如 _PLUGIN_PROTO_KEY = "compiler_plugin")并在推导前断言其存在于 WELL_KNOWN_PROTO_MAP,使上游改名在 load 阶段即暴露。
Checklist: [6.1] DRY:重复非平凡逻辑被抽取或显式复用
|
|
||
| EXPECT_TRUE(TelemetryRuntime::shutdown(5000)); | ||
| auto spans = span_data->GetSpans(); | ||
| ASSERT_EQ(spans.size(), cases.size() + 1); |
There was a problem hiding this comment.
[P3] 端点校验用例依赖 span 导出顺序做下标索引
:507-511 依次创建 endpoint_i 子 span,:516 断言 spans.size() == cases.size() + 1,随后 :517-526 用 spans[i] 与 cases[i] 一一对应。这依赖 in-memory exporter 的导出顺序与创建顺序严格一致(父 span 排在最后),一旦 BSP 批次或 exporter 实现调整,用例会以「属性 key 不存在」的形式误报,且失败信息无法指向具体 case。同目录 grpc_propagation_test.cc 已采用按 span 名查找的写法。
建议: 改为按 endpoint_i 名称建立 map 后查找断言,与同目录既有写法统一,使断言与导出顺序解耦并让失败信息直接定位到具体 case。
Checklist: [6.1] 新逻辑有聚焦单测 + 相关集成/smoke 测试;[I] 同一功能用统一工具函数
765c6a0 to
de767be
Compare
Request-level stop words were tokenized only in their bare form. Byte-level BPE tokenizers merge a leading space into the first token, so a stop word emitted mid-sentence produces a different token sequence and the engine's token-level matcher (GenerateStream::matchStopWordsList) never fires: generation silently runs on to max_tokens. Tokenize each request stop word in both context forms (bare and space-prefixed) so the engine and the renderer stop at the same boundary. Model/env stop words keep the existing renderer tokenization path (special tokens have no space-context issue). Fall back to encode() without add_special_tokens for tokenizers that do not accept the kwarg, logging a warning since the fallback may pick up special tokens.
Vendor a trace-only opentelemetry-cpp dependency and pin its C++17 STL ABI mode consistently across CUDA and ROCm toolchains. Adjust the protobuf toolchain metadata required by the first native cc_proto_library consumer. Add the fail-open process runtime, OTLP exporter diagnostics, W3C gRPC propagation, exactly-once span guards, bounded trace attributes, and post-hoc phase span synthesis. Include focused runtime, propagation, and phase tests. Review follow-up: the propagation test now drives startRpcServerSpan() and asserts the exported SERVER span's remote parentage (trace id, parent span id, kServer, rpc attrs) with bounded RPC/shutdown deadlines and a foreign traceparent case; restore example/BUILD as its own package boundary while the OTel patch dir keeps a fresh BUILD; scope the attribute-schema comments to this PR (the Python schema aligns in a follow-up).
Consumers that need to know how far a request actually got had only wait_time_us and the first-token timestamp to work from, and neither field's contract is "state machine progress": - first_token_time could be published for a token that was later trimmed away, so the reported first-token instant did not correspond to any token in the final sequence - nothing distinguished a stream that never reached RUNNING from one that ran, so a reader could infer compute that never happened - getTimeInfo() read the fields without the stream mutex, so it could return a mix of values belonging to different states GenerateStream now publishes explicit milestones instead: - first_token_time is stamped only after the token actually commits to the sequence (num_new_tokens > 0, after setSeqLength) - running_started / generation_done flags with their own timestamps, taken on the real state transitions - getTimeInfo() takes the stream mutex and returns one coherent snapshot, and resetBeginTime keeps running_started_time consistent RpcServerRuntimeMeta now consumes that atomic snapshot: both the cancel and dequeue paths take a single getTimeInfo() sample and derive waiting/execution time from that one snapshot, instead of mixing a lock-free beginTimeUs() read with a separately locked getTimeInfo(). The now-unused beginTimeUs() accessor is removed so no consumer can reintroduce the cross-epoch skew. wait_time_us keeps its legacy publication contract untouched, so metrics and metadata consumers are unaffected. Review follow-up: first_token_rt_us keeps its frozen firstTokenLatencyUs() contract (committed once, never recomputed against a reset begin time, so it cannot go negative) rather than being clamped; GenerateStreamTest gains a frozen-across-reset regression, a real num_new_tokens == 0 max-token case, and a bounded-wait lifecycle publication test with deterministic destruction order.
The Python half of the end-to-end request trace. tracing.py is the process-level OTel runtime mirroring the C++ side: env-driven config, OTLP/HTTP export, bounded batch span processor, fail-open everywhere, disabled by default, host.ip from a real POD_IP (never faked from hostname-pid), system CA bundle auto-detection for HTTPS endpoints, and a disabled-branch log that distinguishes "env not passed" from "init not called". The global propagator is TraceContext-only. RequestTraceState holds the per-request span with a lock-guarded add_event() that drops events after finish, and exposes settled_ok so a child span that can only settle during its own teardown can tell plain cleanup from a genuine interruption. _DiagnosticExporter wraps the OTLP exporter with cumulative failure counters, a rate-limited warning per interval and a shutdown summary, so a dead wire path is visible instead of silently dropping spans; test injection paths stay unwrapped. attributes.py is the single source of truth for the whole attribute schema, the C++ keys included, grouped into resource / request / response layers with the consumer of every key documented, so a rename cannot silently produce a second unqueryable attribute name. resolve_region_env() resolves the region-mapped OTLP endpoint variables, and start_server.py calls it in the launcher before any child process spawns: the C++ backend reads OTEL_EXPORTER_OTLP_TRACES_* strictly from its inherited environment, so writing them only inside the frontend's init_telemetry() would leave the backend without an endpoint. It is idempotent (fills unset keys only) and fail-open, and carries the scope version over the same inheritance path. The tracing dependency is optional at runtime. When it is unavailable, tracing.py degrades to a no-op while inference remains unaffected. The tests use unittest to keep the test boundary dependency-light and report a missing runtime explicitly instead of silently passing.
Puts the C++ telemetry library to work on the Local / Prefill / Decode handlers and initializes the runtime from the pybind entry point. Each handler opens a gRPC SERVER span continuing the W3C context from inbound gRPC metadata, and the Prefill->Decode hop injects it again on the outbound call, so one chat completion is a single trace across both nodes. RAII guards settle every span exactly once. Phase span synthesis runs inside a PhaseSpanSynthesisScope so it also fires on early returns and exception unwinding rather than only on the happy path, and the decode side synthesizes the load_cache child span over its KV-arrival wait window. Per-hop token usage lands as the five-key gen_ai.usage.* group (semconv input/output, legacy prompt/completion aliases, total) on the prefill/decode/local spans, with a non-positive side suppressing the whole group. Two failure modes the naive instrumentation got wrong: CLIENT_GRPC_RET_IF_ERROR settles the RemoteGenerate CLIENT span before the retry loop advances, so the next attempt's finish(Retry) was always dropped by the exactly-once guard; worse, when the transport Finish() returned OK a business-level failure showed up as an OK attempt. closeGrpcStream() now accepts the business ErrorCode as an override for that window, and rtp_llm.retry_attempt - the zero-based count of retries already performed, so the initial attempt is not labelled a retry - keeps the whole chain visible on the platform. When KV cache loading failed the decode handler reported a generic INTERNAL error, so the trace looked like the decode node itself broke down while waiting for execution. The distinction matters: nothing on this node failed, an upstream dependency did. loadCacheFromPrefill now keeps the ErrorInfo on the context and maps the ErrorCode onto the matching gRPC status instead of flattening everything to INTERNAL, and a cache-load dependency failure labels the synthesized wait span DependencyFailure. rtp_llm.pd_sep on a span is computed from the request fields actually sent to the selected Prefill endpoint, mirroring PrefillRpcServer's own branch condition, rather than from the process role. Also sharpen cancellation semantics on the Decode streaming boundary the spans now report on: a failed GENERATE read is classified CANCELLED only when the server context confirms cancellation, keeping INTERNAL for protocol and cache failures, and Decode RPC stage failures are logged. Focused regression coverage included.
Add the frontend HTTP SERVER span and model RPC CLIENT span, propagate W3C context into the C++ request chain, and settle streaming, completion, cancellation, token usage, and access-log correlation consistently. Represent PD node selection as an INTERNAL master_route span. Record the selected route source and proactive queue-rejection inputs so routing latency and throttling decisions remain visible on both success and error paths. The finished application frame is not the gRPC EOF: if it escapes first, an upstream renderer can close the generator while the server is still settling the RPC, converting a naturally completed call into CANCELLED. grpc.aio receives the terminal status independently of the message iterator, so wait for that physical boundary before publishing the final frame, and record the settled status on the CLIENT span.
Add Dash inference and proxy gRPC boundary spans, W3C propagation, standalone proxy telemetry lifecycle, access-log correlation, and focused normal/error/ cancel coverage. Keep the proxy hops as RPC boundaries without gen_ai model semantics so the platform shows only the downstream real model invocation. Make span finalization exception-safe, cap external request IDs, declare OTel test runfiles, and assert terminal cancellation state rather than timing-racy cancel return values. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TTFT/TPOT previously mixed two unrelated notions on the root SERVER span: AuxInfo engine timings were published as if they were what the caller observed. HTTP and Dash also disagreed, and Dash summed its two phases into one engine TPOT that matched no physical stream. Split the two layers so each span reports only what it can observe: - Streaming HTTP/Dash entry SERVER spans carry the caller-visible timeline, measured at the boundary where the handler is about to yield: gen_ai.response.time_to_first_token and rtp_llm.frontend.time_per_output_token_ms. Role-only, empty, finish-only and internal control frames are not counted as visible tokens. - Each rtp_llm.generate_stream_call CLIENT span carries its own physical stream latency from AuxInfo: rtp_llm.engine.time_to_first_token_ms and rtp_llm.engine.time_per_output_token_ms. Dash phase1 and phase2 therefore report independently instead of being aggregated, and the cross-phase aggregation in access_record is removed. - Non-streaming HTTP writes neither frontend metric: the server cannot observe inter-token delivery when the whole body is sent at once. Request latency remains the SERVER span duration. Two conservative choices avoid publishing numbers we cannot observe: - Frontend TPOT requires two distinct delivery instants. A single frame carrying N>1 tokens exposes no inter-token boundary, so TPOT is omitted rather than reported as 0.0, which a dashboard would read as instant decoding. - For multi-return (n>1), which rides one physical stream, engine TTFT is written only when all sequences agree on it, and engine TPOT is limited to single-sequence streams instead of silently publishing sequence 0 as if it described the whole span. This matches the existing cross-sequence validation in _record_client_span_usage. gen_ai.latency.time_in_model_prefill/decode stay on the root span: they are phase latencies in nanoseconds, disjoint from the TTFT/TPOT keys.
de767be to
51869b4
Compare
LLLLKKKK
left a comment
There was a problem hiding this comment.
AI Code Review - PR #1341
Status: LGTM
Summary: P0/0 · P1/0 · P2/17 · P3/18
Reviewed: commit 51869b4da2a2 · 2026-08-28 18:49 UTC+8
lgtm ready to ci
Non-blocking Suggestions
P2
- Decode 阶段 span 的 error.type 会把 KV 加载完成之后的本地超时误标为 DependencyFailure @
rtp_llm/cpp/model_rpc/DecodeRpcServer.cc:111- 建议:不要用执行阶段推断故障归属,改为按错误码族判定:仅当
error_info.code()属于 cache-store / KV 加载类错误(LOAD_CACHE_TIMEOUT、CACHE_STORE_LOAD_*、P2P_CONNECTOR_*)时返回DependencyFailure,其余交给已有的grpcStatusCodeName(error_status)分支。若希望保留 stage 判定,则需把nextStage()提到CHECK_REQUEST_STOP之前。建议在DecodeRpcServerTest.cc补一个「load 成功 + 随后 GENERATE_TIMEOUT」的用例固化期望值。
- 建议:不要用执行阶段推断故障归属,改为按错误码族判定:仅当
- load 阶段的首个 Read 未改用新增的取消感知助手,同一取消事件的 trace 与指标双双失真 @
rtp_llm/cpp/model_rpc/DecodeRpcServer.cc:278- 建议:把该 Read 失败也改成
generateRequestReadFailureStatus(decode_context.isRequestCancelled())并复用同一条带 cancelled 标记的告警日志,使三处 Read 失败分类统一;同时把该分支纳入DecodeRpcServerTest现有的generateRequestReadFailureStatus用例组。
- 建议:把该 Read 失败也改成
- Decode 端 P→D 终态 gRPC status 改为 ErrorCode 映射,会触发 Prefill 侧 RESOURCE_EXHAUSTED 改写并缺少 handler 级覆盖 @
rtp_llm/cpp/model_rpc/DecodeRpcServer.cc:319- 建议:补 handler 级用例,断言
LOAD_CACHE_TIMEOUT下返回DEADLINE_EXCEEDED、CANCELLED下返回CANCELLED,并显式覆盖会被改写的RESOURCE_EXHAUSTED分支;给 PrefillRpcServer.cc:147 的改写加「仅当 PB 未给出业务错误码时才按传输码改写」的约束;并在 PR 描述中记录「P→D RemoteGenerate 终态码由固定 INTERNAL 变为按 ErrorCode 映射」及 decode 侧cancel_qps/error_code维度迁移,便于按 gRPC code 建看板/告警的一方同步。
- 建议:补 handler 级用例,断言
- 异步 batch PD 路径没有 SERVER span,而客户端已为该通道注入 traceparent @
rtp_llm/cpp/model_rpc/PrefillRpcServer.cc:752- 建议:若 batch PD 形态本期不接入,请在 PR 描述与
rtp_llm/telemetry/README.md中明确记录这一缺口(客户端有 span 与 traceparent 但服务端无对应子 span),避免上线后被判读为「trace 采集异常」;若属遗漏,则在PrefillBatchRpcServer请求入口按同样方式创建 SERVER span。注意该路径 context 由DeferredPrefillContext持有、跨EnqueueBatch/Fetch两次 RPC,span 起止需单独设计,不能照搬 handler 作用域写法。
- 建议:若 batch PD 形态本期不接入,请在 PR 描述与
- 流式 SERVER span 的结束与 traceparent 注入依赖生成器体内读取 contextvar,与同文件预捕获注释直接矛盾 @
rtp_llm/frontend/frontend_server.py:290- 建议:统一二者:由
chat_completion/_infer_impl在构造StreamingResponse之前把已捕获的 trace_state 作为参数传入stream_response;并补一条「从独立asyncio.Task迭代 stream_response」的用例断言 span 已 finish、traceparent 已注入。若确认 Starlette/anyio 必然复制上下文,则删除 :596-599 的预捕获与误导性注释,保持单一约定。
- 建议:统一二者:由
- 遥测结算 task 把自身清理触发的本地取消标成 RpcError,与 engine-finished 的成功语义自相矛盾 @
rtp_llm/cpp/model_rpc/model_rpc_client.py:224- 建议:区分终态来源:本地清理触发的 cancel 统一使用
RpcSettlementCancelled/RpcSettlementTimeout一类 error_type,或在引擎已 finished 时按成功收尾(仅记录rpc.response.status_code=CANCELLED属性),使同一业务结局在两条路径上得到一致的 span 状态;同步调整对应用例的期望值。
- 建议:区分终态来源:本地清理触发的 cancel 统一使用
- 每个被 trace 的请求派生游离 settlement task,把 gRPC 取消延后最长约 5 秒,超时硬编码且无回滚开关 @
rtp_llm/cpp/model_rpc/model_rpc_client.py:1069- 建议:把
RPC_SETTLE_TIMEOUT_SECONDS/RPC_CLEANUP_TIMEOUT_SECONDS暴露为可通过环境变量覆盖(与RTP_LLM_OTEL_*同一读取风格)并给更小默认值,形成线上回滚手段;为 settlement task 加进程级并发上限(超限直接以当前已知状态结束 span)与无 deadline 分支的兜底上界;补一条「trace 打开 +use_fetch_response+ 终帧」的用例,明确 FetchResponse 的取消契约是否允许被观测路径改变,并对cleanup_timed_out命中补指标或 warning。
- 建议:把
- telemetry_test_deps() 默认返回空列表,与无 skip 保护的依赖契约用例及 BUILD 注释三方矛盾 @
arch_config/arch_select.bzl:188- 建议:优先在公共
deps中补 OTel Python 依赖并让默认telemetry_test_deps()指向该 pip target,使传播链路在默认/开源构建中真实执行、依赖契约用例名副其实。若确定只在内部 lock 提供:(1) 把两处 BUILD 注释改成事实描述;(2) 把TestDependencyContract改为按环境变量或--define开启的严格模式(如RTP_LLM_OTEL_TEST_STRICT为真时OTEL_AVAILABLE=False应self.fail())并在携带 SDK 的 CI 配置打开,或在 CI 汇总显式统计 skip 数,避免「绿灯即通过」。
- 建议:优先在公共
- telemetry_test_deps() 成为 @arch_config 的新必需 Starlark 符号,需所有提供方同步 @
arch_config/arch_select.bzl:193- 建议:在 PR 描述或 commit message 中标注该符号需与所有 arch_config 提供方(含内部覆盖版本)同步添加,并确认对应改动已进入同一批次;函数注释里补一句「所有 arch_config 变体都必须提供此符号」,避免后续新增变体时漏实现。
- wheel 钉住跨 29 个小版本的 opentelemetry-sdk 与 exporter,且该组合无任何测试加载 @
rtp_llm/BUILD:701- 建议:把两个包对齐到同一 minor(若受 pinned protobuf 约束无法升 exporter,则把 sdk/api 一并降到同系列);并补一条针对已发布版本的依赖契约测试——真实构造
OTLPSpanExporter(endpoint=...)并对一条 span 执行export(),断言返回 SUCCESS 而非仅断言 import 成功。若必须保留版本差,请在注释中写明「已验证 sdk 1.44 的 SpanData 可被 exporter 1.15 编码」的依据与复验方式。
- 建议:把两个包对齐到同一 minor(若受 pinned protobuf 约束无法升 exporter,则把 sdk/api 一并降到同系列);并补一条针对已发布版本的依赖契约测试——真实构造
- DashSc 独立 wheel 打入 telemetry 代码并真实创建 span,但未声明 opentelemetry 运行时依赖 @
rtp_llm/BUILD:741- 建议:若 DashSc 独立部署形态需要 trace,请把
opentelemetry-sdk/opentelemetry-exporter-otlp-proto-http按同一组版本加入whl_reqs_dash_sc_grpc;若该形态有意不出 trace,请在该列表上加注释说明「独立 wheel 不携带 OTel 运行时,tracing 恒为 no-op」,避免后续排查误判为配置问题。
- 建议:若 DashSc 独立部署形态需要 trace,请把
- WORKSPACE 内联第三方 http_archive 绕过 @rtp_deps 分层,且 opentelemetry_cpp_deps() 的加载顺序是无注释的隐式强约束 @
WORKSPACE:5- 建议:把该
http_archive迁入deps/http.bzl(或git.bzl)以复用既有分层与内源覆盖点;如需保留内联,请在 :55 的load上方补注释明确「必须位于http_deps()/git_deps()之后,否则maybe()会让第三方 protobuf/curl/zlib pin 覆盖仓库自有版本」,并在 :5 的注释里指向 :55 的调用点,让顺序契约自解释。
- 建议:把该
- RtpLLMOp 重复实现 RoleType→字符串映射,新增角色会静默落到 unknown 并污染 service.name 拓扑 @
rtp_llm/cpp/pybind/multi_gpu_gpt/RtpLLMOp.cc:353- 建议:复用既有工具函数:把
roleTypeToString(role_type)结果统一转小写后传入TelemetryRuntime::init,或在RoleTypes.h旁新增roleTypeToLowerString()作为唯一映射点并删除本地switch;若必须保留独立 switch,建议去掉default改为覆盖全部枚举值,让新增角色在编译期暴露。
- 建议:复用既有工具函数:把
- rtp_llm.route.source 的低基数枚举契约未被生产代码约束,唯一引用它的断言恒真 @
rtp_llm/server/backend_rpc_server_visitor.py:453- 建议:把这 6 个取值改成
Enum/Literal(或模块级常量)并让RTP_LLM_ROUTE_SOURCE_VALUES由其派生,赋值处只引用常量;测试侧把恒真那行替换为对生产实际产出的校验(route_span.attributes[RTP_LLM_ROUTE_SOURCE] in RTP_LLM_ROUTE_SOURCE_VALUES),并用参数化补齐master/request/domain_fallback三条单段路径。
- 建议:把这 6 个取值改成
- returned_sequence_count 三元两支求值恒等,注释描述的 beam 区分在代码中不存在 @
rtp_llm/cpp/model_rpc/LocalRpcServer.cc:236- 建议:按注释意图修正条件(beam 场景应取 1),或直接删除恒等三元只保留
std::max(stream->numReturnSequences(), 1)并把注释改写为「beam 宽度不计入对外 usage,多返回序列按行数聚合」;同时用新增的createDecoderStream(..., num_return_sequences)形参补一条 n>1 的用量聚合用例固化期望值。
- 建议:按注释意图修正条件(beam 场景应取 1),或直接删除恒等三元只保留
- PhaseTiming 装配在 C++ 三处重复、CLIENT span 结算序列在 Python 五处重复,已出现细节漂移 @
rtp_llm/cpp/model_rpc/LocalRpcServer.cc:212- 建议:C++ 侧在
PhaseSpanSynthesizer.h增加makePhaseTiming(time_info, request_id, synthesis_end_us)辅助函数,三个 handler 只保留角色与error_type差异,使TimeInfo新增字段时不会漏改、usage 与 span 合成的先后顺序在一处确定;Python 侧抽出_finish_client_span(client_span, status, outputs, *, include_all_sequences, error=None, error_type="", with_latency=True),把五处收尾统一为一次调用并用参数表达差异,便于对收尾矩阵做参数化断言。
- 建议:C++ 侧在
- PD 分离判定在 Python 侧重复实现,rtp_llm.pd_sep 存在两个事实来源且无 parity 测试 @
rtp_llm/cpp/model_rpc/model_rpc_client.py:56- 建议:将该判定改为从
trans_input产出的input_pb.generate_config上求值(与 C++ 读取同一份数据),或增加一条 parity 测试:构造覆盖各 fallback 条件的输入,断言 Python 预测与 Prefill 侧实际分支一致。
- 建议:将该判定改为从
P3
- OTel STL ABI 模式仅在 rocm config 钉住,其余 config 依赖各 wrapper 恰好保留带括号的 -D @
.bazelrc:361- 建议:建议把
with_cxx_stdlib=2017提升为通用默认(build:/common:段),让所有 config 共享同一字面量 STL 模式而不依赖各 wrapper 的引用行为;并在 PR 描述中说明内部ppuconfig 是否已实测通过。
- 建议:建议把
- 同一 trace 内三个 SERVER span 各自完整申报 gen_ai.usage.*,聚合口径依据只写在另一文件的 docstring @
rtp_llm/cpp/model_rpc/PrefillRpcServer.cc:789- 建议:把「平台仅聚合 caller-side span,故各 SERVER span 可独立申报完整 usage」这一前提写入
setUsageTokenAttributes的注释与rtp_llm/telemetry/README.md,并注明验证方式与复验时机;若无法长期保证该前提,则改为只在一跳(建议 Decode,持有完整输出)申报完整 usage,Prefill 仅记录 per-hop 私有键。
- 建议:把「平台仅聚合 caller-side span,故各 SERVER span 可独立申报完整 usage」这一前提写入
- P→D CLIENT span 的阶段耗时属性在重试时跨 attempt 累加 @
rtp_llm/cpp/model_rpc/PrefillGenerateContext.cc:150- 建议:在
remoteAllocateResource创建新 CLIENT span 时快照当前三个 rt 值,closeGrpcStream写入时用增量;或把三个属性重命名为累计语义(如*_cumulative_rt_us)并在注释中写明跨 attempt 累加,使读者不会误读为本次 attempt 耗时。补一条两次 attempt 的用例断言第二个 span 的 allocate rt 不包含第一次的耗时。
- 建议:在
- except grpc.RpcError 分支缺少 settlement 归属判断,与分离任务竞争写同一 span @
rtp_llm/cpp/model_rpc/model_rpc_client.py:1086- 建议:将该分支与其他三处对齐,加上
and client_settlement_task is None守卫;若确实希望 RpcError 优先覆盖结算任务的判定,请在注释中显式声明该优先级并补一条断言最终error.type的用例,使语义可验证而非依赖调度顺序。
- 建议:将该分支与其他三处对齐,加上
- chat_completion 硬编码 span attribute key 字符串,未复用 attributes 常量表 @
rtp_llm/frontend/frontend_server.py:456- 建议:改为
trace_attrs.REQUEST_ID/trace_attrs.RTP_LLM_REQUEST_ID,让索引键只有单一定义点。
- 建议:改为
- getRpcConnection 中包裹已判空解引用的 try/catch 属不可达防御代码,且 trace 端点与 decode_addr 状态重复 @
rtp_llm/cpp/model_rpc/PrefillRpcServer.cc:272- 建议:删除该 try/catch,保留两行赋值(入口已清零,失败路径天然是空值语义);更进一步可去掉这两个字段,在
remoteAllocateResource的唯一调用点直接从prefill_context.decode_addr拆分 host/port 传给startChildClientSpan(该函数已对空地址与端口范围做校验并在无效时跳过 endpoint 属性)。
- 建议:删除该 try/catch,保留两行赋值(入口已清零,失败路径天然是空值语义);更进一步可去掉这两个字段,在
- 用字面量 getattr 读取动态挂载的 rtp_error_code @
rtp_llm/server/backend_rpc_server_visitor.py:486- 建议:在
FtRuntimeException.__init__中显式声明self.rtp_error_code: Optional[int] = None(并纳入__reduce__),随后改为直接属性访问;或提供resolve_error_code(exc) -> int工具函数,让 span 埋点与其他错误码消费方共用同一解析逻辑。
- 建议:在
- markPriorityPreemptionCanceled 在 stream 为空但 running 条目仍存在时不再清理该条目 @
rtp_llm/cpp/model_rpc/RpcServerRuntimeMeta.h:152- 建议:在
else分支补兜底:当running != end()且stream == nullptr(或running->second.stream == nullptr)时仍erase(running),把「不清理」严格限定在「running 条目属于替换流」这一情形;并在RpcServerRuntimeMetaTest.cc补一个「overlay 存在 + running 条目存在 + 传入 null stream」的用例。
- 建议:在
- waiting_time_ms 单位修正改变了 WorkerStatus 对外取值,缺少迁移说明 @
rtp_llm/cpp/model_rpc/RpcServerRuntimeMeta.h:89- 建议:在 PR 描述或 commit message 中显式记录「running 任务
waiting_time_ms由误填微秒修正为毫秒,取值缩小约 1000 倍」,并确认 FlexLB 侧无按旧量级标定的 hang 判定阈值;若存在,请同批调整或给出灰度顺序。
- 建议:在 PR 描述或 commit message 中显式记录「running 任务
- GRPC_RET_IF_ERROR 用魔法字符串 "0" 判断请求是否已获得 request_id @
rtp_llm/cpp/model_rpc/DecodeRpcServer.cc:42- 建议:改为显式状态判定,例如在
DecodeGenerateContext上增加bool requestIdentityResolved() const,或在prepareGenerateContext成功解析后置一个标志,宏内改判该标志;至少也应把"0"提为具名常量并与构造函数初始值绑定在同一处定义。
- 建议:改为显式状态判定,例如在
- enqueue 与 settlement 中存在重复条件分支、死赋值与冗余计算 @
rtp_llm/cpp/model_rpc/model_rpc_client.py:1039- 建议:合并同条件分支、删除死赋值、将
remaining直接写成RPC_SETTLE_TIMEOUT_SECONDS,让真正需要注释解释的时序约定更突出。
- 建议:合并同条件分支、删除死赋值、将
- trace 属性 parity 测试口径偏窄,无法拦截裸字面量属性键 @
rtp_llm/telemetry/test/trace_attributes_parity_test.py:44- 建议:补一条可执行约束(脚本检查或本测试增设规则):
rtp_llm/cpp/telemetry/**与rtp_llm/cpp/model_rpc/**中setAttribute(/SetAttribute(的第一个实参不得为字符串字面量、必须来自kAttr*,并在头文件写明「所有 span 属性必须以 kAttr 前缀命名」;Python 侧把白名单收窄为显式导出的 span 属性键集合,避免把取值常量混入 key 比对集合。
- 建议:补一条可执行约束(脚本检查或本测试增设规则):
- telemetry 测试包使用原生 cc_test,绕过仓库统一的 cc_test_wrapper @
rtp_llm/cpp/telemetry/test/BUILD:3- 建议:与其余测试包保持一致:
load("//:def.bzl", "cc_test_wrapper", "copts")并加上cc_test = cc_test_wrapper。若这三个目标确实需要原生cc_test语义,请在文件顶部注释写明理由。
- 建议:与其余测试包保持一致:
- telemetry_test 依赖 span 导出顺序做下标匹配,而非按名字查找 @
rtp_llm/cpp/telemetry/test/telemetry_test.cc:518- 建议:复用同批测试已有的按名查找模式:用
endpoint_i名字定位对应 span 后再断言server.address/server.port,使断言与创建顺序解耦。
- 建议:复用同批测试已有的按名查找模式:用
- dash_sc 遥测生命周期测试 mock 掉了自己声称覆盖的边界,role/rank 契约无断言 @
rtp_llm/dash_sc/test/app_test.py:192- 建议:把 patch 目标下移一层:patch
bg_app.init_telemetry而非_init_trace_telemetry,然后断言init_telemetry.assert_called_once_with("dash_sc", 0);或在 fail-open 用例中补上同样的入参断言,使用例名的承诺与断言一致。
- 建议:把 patch 目标下移一层:patch
- createDecoderStream 新增的 num_return_sequences 参数无任何调用方 @
rtp_llm/cpp/engine_base/stream/test/GenerateStreamTest.cc:76- 建议:在本 PR 内补一个使用该参数、断言多返回序列下 usage 聚合结果的用例(正好可覆盖上文
returned_sequence_count恒等三元);否则删除形参与 :79 赋值,保持测试辅助函数与实际用法一致。
- 建议:在本 PR 内补一个使用该参数、断言多返回序列下 usage 聚合结果的用例(正好可覆盖上文
- host_service_test 的 patch target 修复属无关变更,且该目标仍为 manual 无 CI 保护 @
rtp_llm/server/test/host_service_test.py:35- 建议:将该测试修复拆到独立提交/PR,并同步去掉
tags = ["manual"]使其真正进入 CI;如需保持最小 patch 面并恢复「被测消费者是 host_service」的表达,可把host_service.py的import requests提升到模块级,再 patchrtp_llm.server.host_service.requests.post。
- 建议:将该测试修复拆到独立提交/PR,并同步去掉
- patch 的“禁止接入 metrics exporter”约束仅存在于注释,无自动化守护 @
patches/opentelemetry_cpp/0001-trace-only-otlp-recordable.patch:7- 建议:在
patches/opentelemetry_cpp/BUILD(目前为空文件)或rtp_llm/cpp/telemetry/BUILD的 deps 注释中补一段说明,写明该目录承载的裁剪契约、误接入时会看到的具体报错形态与恢复步骤,使排查者能从错误信息直接定位到本 patch。
- 建议:在
Checklist Findings (20 fail / 54 total)
General Principles Checklist
- [6.1] Architecture — 依赖方向:无循环依赖/跨层惊喜 → issue
patch 的“禁止接入 metrics exporter”约束仅存在于注释,无自动化守护
patch 头部(:1-8)已明确写出裁剪动机与「Restore the removed inputs ... before adding any metrics exporter consumer」的恢复条件(该项相比上一轮 review 已改善)。但这一约束没有任何构建期或测试期守护:由于otlp_metric_utils.h(:24)与otlp_preferred_temporality.h(:26)被从hdrs移除而文件仍留在归档中,将来有人给//rtp_llm/cpp/telemetry或其他 target 加上 OTel metrics exporter,得到的会是指向第三方目录的缺失头文件编译错误,而不是「本仓库已裁剪 metrics 输入」这类可自解释的失败。 - [6.1] Architecture — 兼容性:外部 HTTP/RPC API、持久数据、配置、环境迁移安全 → issue
waiting_time_ms 单位修正改变了 WorkerStatus 对外取值,缺少迁移说明
enqueue由直接填入time_info.wait_time_us改为time_info.wait_time_us / 1000(:89)。这是正确修复——字段名为waiting_time_ms,captureStreamRuntimeSnapshot与computeExecutionTimeMs(:27)早已按毫秒口径。但该字段经LocalRpcServer.cc写入 proto 并由 FlexLB 与worker_status.py消费(后者注释标明用于 master 判定 server 是否 hang),running 任务上报值因此缩小约 1000 倍。已核对下游主要为观测面、未见按绝对值判定的调度阈值,风险有限,但仍是对外数值语义变更,而 PR 描述未提及。 - [6.1] Architecture — 分层边界:新概念在正确层级,不泄漏内部 → issue
WORKSPACE 内联第三方 http_archive 绕过 @rtp_deps 分层,且 opentelemetry_cpp_deps() 的加载顺序是无注释的隐式强约束
其余第三方依赖统一经@rtp_deps//:http.bzl/git.bzl(:43-49)声明,该仓库可被--override_repository替换、是内源覆盖点;io_opentelemetry_cpp却直接内联在 :5-21,脱离这一分层。更关键的是opentelemetry_cpp_deps()(:55-57)内部以maybe()声明 curl / protobuf / zlib / googletest / grpc 等核心依赖,只有在它排在http_deps()(:45)、git_deps()(:49)、xgrammar_deps()(:53) 之后调用时,仓库自有 pin 才会胜出。当前顺序正确但完全隐式:任何人把 :55-57 上移,都会让第三方 protobuf/curl 版本静默接管整个构建工具链——而这正是本 PR 需要修blacklisted_protos并打 metrics 裁剪 patch 的根因。:5 的声明与 :55 的调用相隔近 50 行且无互相引用。 - [6.1] Architecture — 可观测性:日志/指标/超时可操作、非噪声 → issue
P→D CLIENT span 的阶段耗时属性在重试时跨 attempt 累加
remoteAllocateResource(PrefillRpcServer.cc:349-375)每次重试新建 CLIENT span,而closeGrpcStream把stat_info.remote_allocate_resource_rt_us/poll_local_output_rt_us/poll_remote_output_rt_us写到该 per-attempt guard 上(:150-163)。已核对PrefillStatInfo::nextStage()(:17-60)对全部 rt 字段使用+=累加,restoreStage()(:13-15)只恢复stage索引,PrefillGenerateContext::reset()(:191-206)不清零stat_info,而EXECUTE_WITH_RETRY每次 attempt 都调用reset()。因此第 N 次 attempt 的 span 报告的是第 1..N 次的累计耗时,与该 span 自身的 duration 不自 - [6.1] Architecture — 回滚路径:风险行为存在运维回滚手段 → issue
patch 的“禁止接入 metrics exporter”约束仅存在于注释,无自动化守护
patch 头部(:1-8)已明确写出裁剪动机与「Restore the removed inputs ... before adding any metrics exporter consumer」的恢复条件(该项相比上一轮 review 已改善)。但这一约束没有任何构建期或测试期守护:由于otlp_metric_utils.h(:24)与otlp_preferred_temporality.h(:26)被从hdrs移除而文件仍留在归档中,将来有人给//rtp_llm/cpp/telemetry或其他 target 加上 OTel metrics exporter,得到的会是指向第三方目录的缺失头文件编译错误,而不是「本仓库已裁剪 metrics 输入」这类可自解释的失败。 - [6.1] Architecture — 状态不变量:创建/更新/失败/重试/回滚路径有效 → issue
GRPC_RET_IF_ERROR 用魔法字符串 "0" 判断请求是否已获得 request_id
宏内用decode_context.request_key == "0"(:42)区分「首个 ALLOCATE 报文尚未到达」。该等式成立只是因为DecodeGenerateContext以request_id=0构造(RemoteGenerate里紧挨着就写着TODO request id is 0 here)、GenerateContext把request_key初始化为std::to_string(request_id)(GenerateContext.h:23)。一旦初始request_id改成 -1 或其他占位值,这段日志会静默退化为打印裸request_key,且不会有任何编译或测试失败提示。 - [6.1] Architecture — 错误语义:fail-fast/retry/fallback/silent 行为显式 → issue
用字面量 getattr 读取动态挂载的 rtp_error_code
:486 使用int(getattr(e, "rtp_error_code", e.exception_type))。FtRuntimeException.__init__(rtp_llm/config/exceptions.py)并未声明rtp_error_code,该属性只在同文件 :476 由route_error.rtp_error_code = master_route_result.error_code动态挂载。于是取值语义完全依赖运行期是否走过那一行,静态检查与 IDE 都无法发现拼写错误或字段改名;__reduce__只回传(exception_type, message),跨进程传递时会静默丢掉该字段。 - [6.1] Quality — PR description 说明动机与设计 → issue
waiting_time_ms 单位修正改变了 WorkerStatus 对外取值,缺少迁移说明
enqueue由直接填入time_info.wait_time_us改为time_info.wait_time_us / 1000(:89)。这是正确修复——字段名为waiting_time_ms,captureStreamRuntimeSnapshot与computeExecutionTimeMs(:27)早已按毫秒口径。但该字段经LocalRpcServer.cc写入 proto 并由 FlexLB 与worker_status.py消费(后者注释标明用于 master 判定 server 是否 hang),running 任务上报值因此缩小约 1000 倍。已核对下游主要为观测面、未见按绝对值判定的调度阈值,风险有限,但仍是对外数值语义变更,而 PR 描述未提及。 - [6.1] Quality — 逻辑变更未混入无关格式化 → issue
host_service_test 的 patch target 修复属无关变更,且该目标仍为 manual 无 CI 保护
8 处装饰器由patch("rtp_llm.server.host_service.requests.post")改为patch("requests.post")。这是必要修复而非等价改写:host_service.py:339的import requests是函数内局部导入,模块上不存在requests属性,旧 target 在 patch 阶段即 AttributeError。但该修复与本 PR 的 telemetry 主题无关,PR 描述亦未提及;且rtp_llm/server/test/BUILD:47仍标注tags = ["manual"],修复后依旧不在 CI 中执行,同时把 patch 面从使用处扩大到全局requests模块属性。 - [6.1] Software Engineering — DRY:重复非平凡逻辑被抽取或显式复用 → issue
telemetry 测试包使用原生 cc_test,绕过仓库统一的 cc_test_wrapper
该文件只load("//:def.bzl", "copts")(:1),三个目标(:3、:17、:32)直接用原生cc_test。而rtp_llm/cpp/**下现有测试包(含同批修改的model_rpc/test/BUILD:1,6)统一写cc_test = cc_test_wrapper,由def.bzl展开为cc_binary+sh_test。新包因此脱离这层统一封装:后续对cc_test_wrapper/cc_test_wrapper.sh的任何调整(入口参数、tags、env 注入、链接方式)都不会作用到这三个目标。 - [6.1] Software Engineering — KISS/YAGNI:无投机性抽象 → issue
createDecoderStream 新增的 num_return_sequences 参数无任何调用方
本次把createDecoderStream(input_ids, new_token_ids)改成带默认值的三参版本(:76)并在 :79 写入generate_config->num_return_sequences,但该 builder 为 .cc 内部类,全文件仅 :133 以两个实参调用一次(走默认值 1);新增的 time-info 用例全部走createComplexContextStream。新增形参与 :79 赋值在本 PR 中即为死代码,而LocalRpcServer.cc:236的多返回序列用量聚合恰恰缺少 C++ 覆盖。 - [6.1] Software Engineering — SRP:模块/类职责单一 → issue
PhaseTiming 装配在 C++ 三处重复、CLIENT span 结算序列在 Python 五处重复,已出现细节漂移
LocalRpcServer.cc:212-249、PrefillRpcServer.cc:761-792、DecodeRpcServer.cc:1420-1448三处PhaseSpanSynthesisScope回调各自重复了同一套 9 行PhaseTiming字段搬运与request_ok计算,且已漂移:Local/Prefill 内联三元判定error_type,Decode 调用phaseErrorType;Local 在synthesizePhaseSpans之前写 usage,另两处在之后。Python 侧_record_client_rpc_status → _record_client_span_usage → _record_client_span_latency → finish(...)在model_rpc_client.py的 :1086-1094、:1127-1142、:1155-1164、:1194-1203、:1210-1217 共 5 处近似复制,差异(GeneratorExit - [6.1] Tests — 分布式/跨平台变更有对应覆盖 → issue
OTel STL ABI 模式仅在 rocm config 钉住,其余 config 依赖各 wrapper 恰好保留带括号的 -D
--@io_opentelemetry_cpp//api:with_cxx_stdlib=2017全仓仅出现在build:rocm(:361),而WORKSPACE:5无条件声明 OTel、telemetry/BUILD无select地依赖它、pybind/BUILD:115又把该 target 放进th_transformer_lib的基础 deps,故所有 config 都构建 OTel SDK。已逐条核对该缺陷机制(shlex.quote/pipes.quote吞掉带括号的-D)只存在于3rdparty/gpus/crosstool/.../crosstool_wrapper_driver_rocm.tpl与3rdparty/cuda_config/crosstool/.../crosstool_wrapper_driver_rocm.tpl,且都仅在args.x[0] == 'rocm'分支生效(后者 :220-223),因此「全平台静默 ABI 分裂」不成立,风险限于使用自定义 - [6.1] Tests — 新逻辑有聚焦单测 + 相关集成/smoke 测试 → issue
dash_sc 遥测生命周期测试 mock 掉了自己声称覆盖的边界,role/rank 契约无断言
test_start_initializes_dash_role_and_shutdowns(:192)用patch.object(bg_app, "_init_trace_telemetry")(:231)把被测边界整体替换,最终只断言init_trace.assert_called_once_with()(:237,无参),因此用例名中的「dash role」实际未被验证。角色字符串与 rank 硬编码在被 mock 掉的app.py:70init_telemetry("dash_sc", 0)中,它决定导出的service.name(rtp_llm_dash_sc),全仓无任何用例断言过这两个实参;test_init_failure_is_fail_open(:241-245)也只验证不抛异常,但已证明bg_app.init_telemetry可被直接 patch。 - [6.1] Tests — 边界 case 覆盖(空、单元素、最大值) → issue
markPriorityPreemptionCanceled 在 stream 为空但 running 条目仍存在时不再清理该条目
改动前只要running != running_streams_.end()就会erase(running);改动后 erase 只在running != end() && has_stream_snapshot && running->second.stream == stream分支发生(:152-155),else if (has_stream_snapshot)(:156-161)不 erase,stream == nullptr(has_stream_snapshot为 false)时也不 erase。若调用方按 :129 的注释「无本地流时传 null」传入 nullptr 而running_streams_[request_id]因故仍存在,该条目将永久滞留(running_streams_无 TTL 清理),虚增 WorkerStatus 的 running 负载并影响调度。逐条核对现有调用路径后确认当前不可达,属契约依赖而非现网缺陷。
RTP-LLM Checklist
- [I] 代码质量 — 同一功能用统一工具函数 → issue
telemetry_test 依赖 span 导出顺序做下标匹配,而非按名字查找
ClientSpanCanonicalizesAndValidatesEndpoint先ASSERT_EQ(spans.size(), cases.size() + 1)(:516),再用spans[i]与cases[i]一一对应(:517-527),隐式假设InMemorySpanData的导出顺序严格等于endpoint_ispan 的End()顺序(:507-511)。同 PR 的phase_span_synthesizer_test.cc提供了findSpan(spans, name)、grpc_propagation_test.cc提供了findSpanByName(...),本文件却退回下标匹配;一旦 BSP 批次拆分或导出顺序变化,失败会表现为「某个 endpoint 的 server.address 不对」这类误导性错误。
Python Static-First Checklist
- [P.A] 静态结构与类型纪律 — 字符串分发用 Enum/Literal → issue
rtp_llm.route.source 的低基数枚举契约未被生产代码约束,唯一引用它的断言恒真
attributes.py:80-95用RTP_LLM_ROUTE_SOURCE_VALUES显式声明该属性的 6 个合法取值(注释强调 low-cardinality enum),但生产代码在backend_rpc_server_visitor.py:453-461等处全部使用裸字符串字面量赋值route_source,从未与该集合比对。全仓检索确认该 frozenset 仅有一处引用:backend_rpc_server_visitor_test.py:392的assertIn("none", trace_attrs.RTP_LLM_ROUTE_SOURCE_VALUES)——字面量对静态集合的断言,无论生产代码怎么改都不会失败,反而给出「枚举已被守护」的错觉;测试中另两处(:382、:415)也只断言"none",master/request/domain_fallback三个单段取值路径完全无覆盖。 - [P.A] 静态结构与类型纪律 — 禁止 getattr/setattr literal 访问 → issue
用字面量 getattr 读取动态挂载的 rtp_error_code
:486 使用int(getattr(e, "rtp_error_code", e.exception_type))。FtRuntimeException.__init__(rtp_llm/config/exceptions.py)并未声明rtp_error_code,该属性只在同文件 :476 由route_error.rtp_error_code = master_route_result.error_code动态挂载。于是取值语义完全依赖运行期是否走过那一行,静态检查与 IDE 都无法发现拼写错误或字段改名;__reduce__只回传(exception_type, message),跨进程传递时会静默丢掉该字段。 - [P.G] 测试规范 — mock.patch target 是使用处而非定义处 → issue
host_service_test 的 patch target 修复属无关变更,且该目标仍为 manual 无 CI 保护
8 处装饰器由patch("rtp_llm.server.host_service.requests.post")改为patch("requests.post")。这是必要修复而非等价改写:host_service.py:339的import requests是函数内局部导入,模块上不存在requests属性,旧 target 在 patch 阶段即 AttributeError。但该修复与本 PR 的 telemetry 主题无关,PR 描述亦未提及;且rtp_llm/server/test/BUILD:47仍标注tags = ["manual"],修复后依旧不在 CI 中执行,同时把 patch 面从使用处扩大到全局requests模块属性。 - [P.G] 测试规范 — mock/fake/stub 不得替代本次声称覆盖的生产边界 → issue
dash_sc 遥测生命周期测试 mock 掉了自己声称覆盖的边界,role/rank 契约无断言
test_start_initializes_dash_role_and_shutdowns(:192)用patch.object(bg_app, "_init_trace_telemetry")(:231)把被测边界整体替换,最终只断言init_trace.assert_called_once_with()(:237,无参),因此用例名中的「dash role」实际未被验证。角色字符串与 rank 硬编码在被 mock 掉的app.py:70init_telemetry("dash_sc", 0)中,它决定导出的service.name(rtp_llm_dash_sc),全仓无任何用例断言过这两个实参;test_init_failure_is_fail_open(:241-245)也只验证不抛异常,但已证明bg_app.init_telemetry可被直接 patch。
Strengths
- span 生命周期设计有注释自证且经得起核对:
GenerateContext.h:61-64与RpcTraceHelper.h:289-294都写明「guard 必须声明在 status 之后」的析构序约束,finish()用原子交换保证 End() 恰好一次,因此重试链上对上一 attempt 补写error.type="Retry"与closeGrpcStream的幂等提前返回(PrefillGenerateContext.cc:117-123)叠加也不会二次 End。 RpcServerRuntimeMeta的三项顺带修复质量高:enqueue(:89)把time_info.wait_time_us / 1000与captureStreamRuntimeSnapshot、computeExecutionTimeMs的毫秒口径统一,修掉了waiting_time_ms字段被填入微秒的单位缺陷;running->second.stream == stream身份校验(:152/:183/:268)阻止重试产生的替换流被旧路径误消费;dequeue(:177-178)补 null 提前返回,消除空指针解引用路径。- 把 stream 指标采集抽成
captureStreamRuntimeSnapshot并移出read_write_lock_,与既有getEngineScheduleInfo的read_write_lock_ → stream mutex_顺序统一,从根上消除锁序反转;commitDequeueSnapshot再做一次身份校验,TOCTOU 安全。 GRPC_RET_IF_ERROR改为do { ... } while (false)(DecodeRpcServer.cc:36-53)消除悬空 else 隐患,code/msg只求值一次,并补上带 peer 与 status code 的可定位 WARNING;phaseErrorType/generateRequestReadFailureStatus抽成无副作用静态函数并获得针对性单测。- 跨进程传播不靠约定:
TelemetryRuntime.cc显式SetGlobalPropagator(HttpTraceContext)(默认 no-op 会静默丢上下文),GrpcTraceCarrier.h两个 carrier 均noexcept+ fail-open,grpc_propagation_test.cc走真实 in-process gRPC 传输验证父子关系并覆盖「无 metadata 起新 root」「外部直写 traceparent」。 - 合成子 span 严格拒绝非法区间(
begin <= 0、end <= begin一律跳过而非钳位),避免把错乱时间戳渲染成看似合理的瀑布图;setUsageTokenAttributes(RpcTraceHelper.h:278)对任一侧非正值整体跳过五键组,并有UsageTokenAttributesSkipNonPositiveValues显式固化该策略。 stream_response把GeneratorExit与asyncio.CancelledError合并捕获(frontend_server.py:306),顺带修掉了旧代码在except BaseException中yield从而触发async generator ignored GeneratorExit的真实缺陷;_handle_grpc_error把ExceptionType(error_code)包进try/except ValueError降级为UNKNOWN_ERROR,消除非法 error_code 抛异常覆盖真实 RPC 错误的既有隐患。- 「渲染器主动完成」有真实握手:
custom_renderer.py在break前mark_renderer_completed(),再由finally: await output_generator.aclose()关闭后端流,使_request_completed_normally能把停用词截断与真实断连区分开,而不是按异常类型猜测。 3rdparty/protobuf/BUILD:883-894的blacklisted_protos修正经逐条核对精确正确:列出的 WKT 已链入:protobuf,compiler/plugin.pb.cc只在:protoc_lib,故排除compiler_plugin必要且充分,注释完整解释了 bazel 6 的 ProtoInfo 约束成因。- 依赖声明不吃传递闭包:
model_rpc/BUILD(cc 与 py)、server/BUILD、pybind/BUILD:115均按直接 include/import 显式补齐,//rtp_llm:telemetry自身无 rtp_llm 内部依赖不引入环,且//rtp_llm/cpp/telemetry只被 pybind 与 model_rpc 依赖、二者同属libth_transformer.so,不会出现 TracerProvider 单例被多个.so各自静态化。 TelemetryRuntime::shutdown()在RtpLLMOp::stop()中受is_server_shutdown_单次守卫、包在gil_scoped_release内且 deadline 有界,不阻塞进程退出;trace 关闭路径有test_trace_disabled_does_not_wait_for_rpc_termination等对照用例直接验证数据面不因观测而改变。
| // cache timeouts onto DEADLINE_EXCEEDED, indistinguishable from a request | ||
| // deadline elsewhere. Client cancellation also surfaces in this stage and | ||
| // keeps its own classification. | ||
| if (stage == DecodeStatInfo::loadCacheFromPrefill && error_info.hasError() |
There was a problem hiding this comment.
[P2] Decode 阶段 span 的 error.type 会把 KV 加载完成之后的本地超时误标为 DependencyFailure
phaseErrorType(:111-114)用 stage == DecodeStatInfo::loadCacheFromPrefill 判定「上游依赖失败」。但 EXECUTE_STAGE_FUNC(GenerateContext.h:105-109)展开顺序是先 CHECK_REQUEST_STOP 再 nextStage(),因此 EXECUTE_STAGE_FUNC(localGenerate, ...)(:1484,紧接 :1483 的 loadCacheFromPrefill)的超时检查执行时 stage 仍是 loadCacheFromPrefill。KV 加载成功后若请求超时,CHECK_REQUEST_TIMEOUT 写入 GENERATE_TIMEOUT 并直接 return,此时 error_info.hasError() 为真且 code != CANCELLED,phaseErrorType 返回 DependencyFailure,把本节点的超时归因给 Prefill。KV 加载上限为 5 秒(...
建议: 不要用执行阶段推断故障归属,改为按错误码族判定:仅当 error_info.code() 属于 cache-store / KV 加载类错误(LOAD_CACHE_TIMEOUT、CACHE_STORE_LOAD_*、P2P_CONNECTOR_*)时返回 DependencyFailure,其余交给已有的 grpcStatusCodeName(error_status) 分支。若希望保留 stage 判定,则需把 nextStage() 提到 CHECK_REQUEST_STOP 之前。建议在 DecodeRpcServerTest.cc 补一个「load 成功 + 随后 GENERATE_TIMEOUT」的用例固化期望值。
| @@ -222,26 +279,46 @@ void DecodeRpcServer::loadCacheFromPrefill(DecodeGenerateContext& decode_context | |||
| decode_context.time_info.updateLoadBeginTime(); | |||
There was a problem hiding this comment.
📍 实际位置 rtp_llm/cpp/model_rpc/DecodeRpcServer.cc:278(不在 diff 展示范围内,就近挂载)
[P2] load 阶段的首个 Read 未改用新增的取消感知助手,同一取消事件的 trace 与指标双双失真
本 PR 为「流被对端取消时 Read 返回 false」引入了 generateRequestReadFailureStatus(cancelled)(:85-90),并在 prepareGenerateContext(:200)与 localGenerate(:331-338)使用,两处都打印 cancelled 标记与状态码。但 loadCacheFromPrefill 的 grpc_stream->Read(&load_request)(:277-278)仍走旧宏,固定得到 INTERNAL "failed to get loadReqeust" 且不写 error_info。于是同一个客户端取消事件:allocate/generate 阶段被分类为 Cancelled 并计入 cancel_qps(GenerateContext::cancelled() 判定 error_code == CANCELLED),load 阶段却是 Internal,phaseErrorType 因 error_info 为空而落到 `g...
建议: 把该 Read 失败也改成 generateRequestReadFailureStatus(decode_context.isRequestCancelled()) 并复用同一条带 cancelled 标记的告警日志,使三处 Read 失败分类统一;同时把该分支纳入 DecodeRpcServerTest 现有的 generateRequestReadFailureStatus 用例组。
| reportEarlyFinishTask(decode_context, | ||
| static_cast<int64_t>(error_info.code()), | ||
| "decode load cache from prefill failed: " + error_info.ToString()); | ||
| decode_context.error_status = grpc::Status(transErrorCodeToGrpc(error_info.code()), error_info.ToString()); |
There was a problem hiding this comment.
[P2] Decode 端 P→D 终态 gRPC status 改为 ErrorCode 映射,会触发 Prefill 侧 RESOURCE_EXHAUSTED 改写并缺少 handler 级覆盖
原先 loadCacheFromPrefill 失败恒返回 INTERNAL,现改为 grpc::Status(transErrorCodeToGrpc(error_info.code()), ...);读流失败(:200、:333)也从固定 INTERNAL 改为按 isRequestCancelled() 返回 CANCELLED。RpcErrorCode.h:13-18 把 MALLOC_FAILED / PRIORITY_PREEMPTED 映射为 RESOURCE_EXHAUSTED,而 Prefill 的 CLIENT_GRPC_RET_IF_ERROR(PrefillRpcServer.cc:147-149)在该传输码下无条件把 new_error_code 覆写为 DECODE_MALLOC_FAILED——该分支此前因终态恒为 INTERNAL 而不可达。PRIORITY_PREEMPTED 场景尤其失真(:15-17 注释说明真实 8429 走 ErrorDetailsPB trailing me...
建议: 补 handler 级用例,断言 LOAD_CACHE_TIMEOUT 下返回 DEADLINE_EXCEEDED、CANCELLED 下返回 CANCELLED,并显式覆盖会被改写的 RESOURCE_EXHAUSTED 分支;给 PrefillRpcServer.cc:147 的改写加「仅当 PB 未给出业务错误码时才按传输码改写」的约束;并在 PR 描述中记录「P→D RemoteGenerate 终态码由固定 INTERNAL 变为按 ErrorCode 映射」及 decode 侧 cancel_qps / error_code 维度迁移,便于按 gRPC code 建看板/告警的一方同步。
| // check above, so Local/Prefill each own exactly one SERVER span. RAII | ||
| // guard covers EXECUTE_STAGE_FUNC early returns and exceptions. | ||
| if (telemetry::TelemetryRuntime::isActive()) { | ||
| auto span = telemetry::startRpcServerSpan( |
There was a problem hiding this comment.
[P2] 异步 batch PD 路径没有 SERVER span,而客户端已为该通道注入 traceparent
全仓检索 startRpcServerSpan / trace_span_guard / PhaseSpanSynthesisScope 只命中 5 个文件(DecodeRpcServer.cc、PrefillRpcServer.cc、LocalRpcServer.cc、GenerateContext.h、DecodeRpcServerTest.cc),PrefillBatchRpcServer.cc 零命中。其 EnqueueBatch / EnqueueGroup / FetchResponse 虽复用同一批 stage 函数,但从不创建 SERVER span,trace_span_guard 恒为 nullptr;由于 P→D CLIENT span 的创建与 injectSpanToClientContext 都被 if (prefill_context.trace_span_guard && ...->valid())(:360)门控,该路径连 CLIENT span 也不产生。而 `model_rpc_client.py:10...
建议: 若 batch PD 形态本期不接入,请在 PR 描述与 rtp_llm/telemetry/README.md 中明确记录这一缺口(客户端有 span 与 traceparent 但服务端无对应子 span),避免上线后被判读为「trace 采集异常」;若属遗漏,则在 PrefillBatchRpcServer 请求入口按同样方式创建 SERVER span。注意该路径 context 由 DeferredPrefillContext 持有、跨 EnqueueBatch/Fetch 两次 RPC,span 起止需单独设计,不能照搬 handler 作用域写法。
| # HTTP SERVER span owner for streaming requests: the four exits below | ||
| # (success / cancel / error / finally) all funnel into the idempotent | ||
| # finish() (manual instrumentation, no ASGI middleware). | ||
| trace_state = CURRENT_TRACE_STATE.get() |
There was a problem hiding this comment.
[P2] 流式 SERVER span 的结束与 traceparent 注入依赖生成器体内读取 contextvar,与同文件预捕获注释直接矛盾
stream_response 在生成器体内首次 __anext__ 时才 CURRENT_TRACE_STATE.get()(:290);而同文件 _call_generate_with_report(:596-599)明确以「StreamingResponse may iterate it from another task whose contextvars snapshot no longer holds CURRENT_TRACE_STATE」为由刻意在 handler 上下文预捕获。两处对同一运行时事实给出相反假设。chat_completion:489-491 已注明「streaming spans are finished by stream_response's four exits」,即流式路径无其他收尾点:若 :596-599 的假设成立,trace_state 为 None,:303/:318/:336/:355 四个出口全部退化为 no-op、SERVER span 永不 end();model_rpc_client.py...
建议: 统一二者:由 chat_completion / _infer_impl 在构造 StreamingResponse 之前把已捕获的 trace_state 作为参数传入 stream_response;并补一条「从独立 asyncio.Task 迭代 stream_response」的用例断言 span 已 finish、traceparent 已注入。若确认 Starlette/anyio 必然复制上下文,则删除 :596-599 的预捕获与误导性注释,保持单一约定。
| auto spans = span_data->GetSpans(); | ||
| ASSERT_EQ(spans.size(), cases.size() + 1); | ||
| for (size_t i = 0; i < cases.size(); ++i) { | ||
| const auto& attributes = spans[i]->GetAttributes(); |
There was a problem hiding this comment.
[P3] telemetry_test 依赖 span 导出顺序做下标匹配,而非按名字查找
ClientSpanCanonicalizesAndValidatesEndpoint 先 ASSERT_EQ(spans.size(), cases.size() + 1)(:516),再用 spans[i] 与 cases[i] 一一对应(:517-527),隐式假设 InMemorySpanData 的导出顺序严格等于 endpoint_i span 的 End() 顺序(:507-511)。同 PR 的 phase_span_synthesizer_test.cc 提供了 findSpan(spans, name)、grpc_propagation_test.cc 提供了 findSpanByName(...),本文件却退回下标匹配;一旦 BSP 批次拆分或导出顺序变化,失败会表现为「某个 endpoint 的 server.address 不对」这类误导性错误。
建议: 复用同批测试已有的按名查找模式:用 endpoint_i 名字定位对应 span 后再断言 server.address / server.port,使断言与创建顺序解耦。
Checklist: [I] 同一功能用统一工具函数
|
|
||
|
|
||
| class TraceTelemetryLifecycleTest(TestCase): | ||
| def test_start_initializes_dash_role_and_shutdowns(self) -> None: |
There was a problem hiding this comment.
[P3] dash_sc 遥测生命周期测试 mock 掉了自己声称覆盖的边界,role/rank 契约无断言
test_start_initializes_dash_role_and_shutdowns(:192)用 patch.object(bg_app, "_init_trace_telemetry")(:231)把被测边界整体替换,最终只断言 init_trace.assert_called_once_with()(:237,无参),因此用例名中的「dash role」实际未被验证。角色字符串与 rank 硬编码在被 mock 掉的 app.py:70 init_telemetry("dash_sc", 0) 中,它决定导出的 service.name(rtp_llm_dash_sc),全仓无任何用例断言过这两个实参;test_init_failure_is_fail_open(:241-245)也只验证不抛异常,但已证明 bg_app.init_telemetry 可被直接 patch。
建议: 把 patch 目标下移一层:patch bg_app.init_telemetry 而非 _init_trace_telemetry,然后断言 init_telemetry.assert_called_once_with("dash_sc", 0);或在 fail-open 用例中补上同样的入参断言,使用例名的承诺与断言一致。
Checklist: [6.1] 新逻辑有聚焦单测 + 相关集成/smoke 测试;[P.G] mock/fake/stub 不得替代本次声称覆盖的生产边界
|
|
||
| GenerateStreamPtr createDecoderStream(std::vector<int> input_ids, std::vector<int> new_token_ids) { | ||
| GenerateStreamPtr | ||
| createDecoderStream(std::vector<int> input_ids, std::vector<int> new_token_ids, int num_return_sequences = 1) { |
There was a problem hiding this comment.
[P3] createDecoderStream 新增的 num_return_sequences 参数无任何调用方
本次把 createDecoderStream(input_ids, new_token_ids) 改成带默认值的三参版本(:76)并在 :79 写入 generate_config->num_return_sequences,但该 builder 为 .cc 内部类,全文件仅 :133 以两个实参调用一次(走默认值 1);新增的 time-info 用例全部走 createComplexContextStream。新增形参与 :79 赋值在本 PR 中即为死代码,而 LocalRpcServer.cc:236 的多返回序列用量聚合恰恰缺少 C++ 覆盖。
建议: 在本 PR 内补一个使用该参数、断言多返回序列下 usage 聚合结果的用例(正好可覆盖上文 returned_sequence_count 恒等三元);否则删除形参与 :79 赋值,保持测试辅助函数与实际用法一致。
Checklist: [6.1] KISS/YAGNI:无投机性抽象
|
|
||
| @patch("rtp_llm.server.host_service.kmonitor.report") | ||
| @patch("rtp_llm.server.host_service.requests.post") | ||
| @patch("requests.post") |
There was a problem hiding this comment.
[P3] host_service_test 的 patch target 修复属无关变更,且该目标仍为 manual 无 CI 保护
8 处装饰器由 patch("rtp_llm.server.host_service.requests.post") 改为 patch("requests.post")。这是必要修复而非等价改写:host_service.py:339 的 import requests 是函数内局部导入,模块上不存在 requests 属性,旧 target 在 patch 阶段即 AttributeError。但该修复与本 PR 的 telemetry 主题无关,PR 描述亦未提及;且 rtp_llm/server/test/BUILD:47 仍标注 tags = ["manual"],修复后依旧不在 CI 中执行,同时把 patch 面从使用处扩大到全局 requests 模块属性。
建议: 将该测试修复拆到独立提交/PR,并同步去掉 tags = ["manual"] 使其真正进入 CI;如需保持最小 patch 面并恢复「被测消费者是 host_service」的表达,可把 host_service.py 的 import requests 提升到模块级,再 patch rtp_llm.server.host_service.requests.post。
Checklist: [6.1] 逻辑变更未混入无关格式化;[P.G] mock.patch target 是使用处而非定义处
| # the shared recordable target while retaining trace and log inputs. | ||
| # | ||
| # This intentionally makes the vendored target unsuitable for OTel metrics. | ||
| # Restore the removed inputs when the protobuf toolchain supports them, or |
There was a problem hiding this comment.
[P3] patch 的“禁止接入 metrics exporter”约束仅存在于注释,无自动化守护
patch 头部(:1-8)已明确写出裁剪动机与「Restore the removed inputs ... before adding any metrics exporter consumer」的恢复条件(该项相比上一轮 review 已改善)。但这一约束没有任何构建期或测试期守护:由于 otlp_metric_utils.h(:24)与 otlp_preferred_temporality.h(:26)被从 hdrs 移除而文件仍留在归档中,将来有人给 //rtp_llm/cpp/telemetry 或其他 target 加上 OTel metrics exporter,得到的会是指向第三方目录的缺失头文件编译错误,而不是「本仓库已裁剪 metrics 输入」这类可自解释的失败。
建议: 在 patches/opentelemetry_cpp/BUILD(目前为空文件)或 rtp_llm/cpp/telemetry/BUILD 的 deps 注释中补一段说明,写明该目录承载的裁剪契约、误接入时会看到的具体报错形态与恢复步骤,使排查者能从错误信息直接定位到本 patch。
Checklist: [6.1] 依赖方向:无循环依赖/跨层惊喜;[6.1] 回滚路径:风险行为存在运维回滚手段
* fix(openai): tokenize request stop words in context Request-level stop words were tokenized only in their bare form. Byte-level BPE tokenizers merge a leading space into the first token, so a stop word emitted mid-sentence produces a different token sequence and the engine's token-level matcher (GenerateStream::matchStopWordsList) never fires: generation silently runs on to max_tokens. Tokenize each request stop word in both context forms (bare and space-prefixed) so the engine and the renderer stop at the same boundary. Model/env stop words keep the existing renderer tokenization path (special tokens have no space-context issue). Fall back to encode() without add_special_tokens for tokenizers that do not accept the kwarg, logging a warning since the fallback may pick up special tokens. * fix(test): repair frontend and host service test isolation * feat(trace): add the C++ OpenTelemetry runtime Vendor a trace-only opentelemetry-cpp dependency and pin its C++17 STL ABI mode consistently across CUDA and ROCm toolchains. Adjust the protobuf toolchain metadata required by the first native cc_proto_library consumer. Add the fail-open process runtime, OTLP exporter diagnostics, W3C gRPC propagation, exactly-once span guards, bounded trace attributes, and post-hoc phase span synthesis. Include focused runtime, propagation, and phase tests. Review follow-up: the propagation test now drives startRpcServerSpan() and asserts the exported SERVER span's remote parentage (trace id, parent span id, kServer, rpc attrs) with bounded RPC/shutdown deadlines and a foreign traceparent case; restore example/BUILD as its own package boundary while the OTel patch dir keeps a fresh BUILD; scope the attribute-schema comments to this PR (the Python schema aligns in a follow-up). * feat(trace): publish and consume coherent stream progress snapshots Consumers that need to know how far a request actually got had only wait_time_us and the first-token timestamp to work from, and neither field's contract is "state machine progress": - first_token_time could be published for a token that was later trimmed away, so the reported first-token instant did not correspond to any token in the final sequence - nothing distinguished a stream that never reached RUNNING from one that ran, so a reader could infer compute that never happened - getTimeInfo() read the fields without the stream mutex, so it could return a mix of values belonging to different states GenerateStream now publishes explicit milestones instead: - first_token_time is stamped only after the token actually commits to the sequence (num_new_tokens > 0, after setSeqLength) - running_started / generation_done flags with their own timestamps, taken on the real state transitions - getTimeInfo() takes the stream mutex and returns one coherent snapshot, and resetBeginTime keeps running_started_time consistent RpcServerRuntimeMeta now consumes that atomic snapshot: both the cancel and dequeue paths take a single getTimeInfo() sample and derive waiting/execution time from that one snapshot, instead of mixing a lock-free beginTimeUs() read with a separately locked getTimeInfo(). The now-unused beginTimeUs() accessor is removed so no consumer can reintroduce the cross-epoch skew. wait_time_us keeps its legacy publication contract untouched, so metrics and metadata consumers are unaffected. Review follow-up: first_token_rt_us keeps its frozen firstTokenLatencyUs() contract (committed once, never recomputed against a reset begin time, so it cannot go negative) rather than being clamped; GenerateStreamTest gains a frozen-across-reset regression, a real num_new_tokens == 0 max-token case, and a bounded-wait lifecycle publication test with deterministic destruction order. * feat(trace): add the Python telemetry runtime and attribute schema The Python half of the end-to-end request trace. tracing.py is the process-level OTel runtime mirroring the C++ side: env-driven config, OTLP/HTTP export, bounded batch span processor, fail-open everywhere, disabled by default, host.ip from a real POD_IP (never faked from hostname-pid), system CA bundle auto-detection for HTTPS endpoints, and a disabled-branch log that distinguishes "env not passed" from "init not called". The global propagator is TraceContext-only. RequestTraceState holds the per-request span with a lock-guarded add_event() that drops events after finish, and exposes settled_ok so a child span that can only settle during its own teardown can tell plain cleanup from a genuine interruption. _DiagnosticExporter wraps the OTLP exporter with cumulative failure counters, a rate-limited warning per interval and a shutdown summary, so a dead wire path is visible instead of silently dropping spans; test injection paths stay unwrapped. attributes.py is the single source of truth for the whole attribute schema, the C++ keys included, grouped into resource / request / response layers with the consumer of every key documented, so a rename cannot silently produce a second unqueryable attribute name. resolve_region_env() resolves the region-mapped OTLP endpoint variables, and start_server.py calls it in the launcher before any child process spawns: the C++ backend reads OTEL_EXPORTER_OTLP_TRACES_* strictly from its inherited environment, so writing them only inside the frontend's init_telemetry() would leave the backend without an endpoint. It is idempotent (fills unset keys only) and fail-open, and carries the scope version over the same inheritance path. The tracing dependency is optional at runtime. When it is unavailable, tracing.py degrades to a no-op while inference remains unaffected. The tests use unittest to keep the test boundary dependency-light and report a missing runtime explicitly instead of silently passing. * feat(trace): instrument the gRPC servers with request and phase spans Puts the C++ telemetry library to work on the Local / Prefill / Decode handlers and initializes the runtime from the pybind entry point. Each handler opens a gRPC SERVER span continuing the W3C context from inbound gRPC metadata, and the Prefill->Decode hop injects it again on the outbound call, so one chat completion is a single trace across both nodes. RAII guards settle every span exactly once. Phase span synthesis runs inside a PhaseSpanSynthesisScope so it also fires on early returns and exception unwinding rather than only on the happy path, and the decode side synthesizes the load_cache child span over its KV-arrival wait window. Per-hop token usage lands as the five-key gen_ai.usage.* group (semconv input/output, legacy prompt/completion aliases, total) on the prefill/decode/local spans, with a non-positive side suppressing the whole group. Two failure modes the naive instrumentation got wrong: CLIENT_GRPC_RET_IF_ERROR settles the RemoteGenerate CLIENT span before the retry loop advances, so the next attempt's finish(Retry) was always dropped by the exactly-once guard; worse, when the transport Finish() returned OK a business-level failure showed up as an OK attempt. closeGrpcStream() now accepts the business ErrorCode as an override for that window, and rtp_llm.retry_attempt - the zero-based count of retries already performed, so the initial attempt is not labelled a retry - keeps the whole chain visible on the platform. When KV cache loading failed the decode handler reported a generic INTERNAL error, so the trace looked like the decode node itself broke down while waiting for execution. The distinction matters: nothing on this node failed, an upstream dependency did. loadCacheFromPrefill now keeps the ErrorInfo on the context and maps the ErrorCode onto the matching gRPC status instead of flattening everything to INTERNAL, and a cache-load dependency failure labels the synthesized wait span DependencyFailure. rtp_llm.pd_sep on a span is computed from the request fields actually sent to the selected Prefill endpoint, mirroring PrefillRpcServer's own branch condition, rather than from the process role. Also sharpen cancellation semantics on the Decode streaming boundary the spans now report on: a failed GENERATE read is classified CANCELLED only when the server context confirms cancellation, keeping INTERNAL for protocol and cache failures, and Decode RPC stage failures are logged. Focused regression coverage included. * feat(trace): trace HTTP requests and PD routing end to end Add the frontend HTTP SERVER span and model RPC CLIENT span, propagate W3C context into the C++ request chain, and settle streaming, completion, cancellation, token usage, and access-log correlation consistently. Represent PD node selection as an INTERNAL master_route span. Record the selected route source and proactive queue-rejection inputs so routing latency and throttling decisions remain visible on both success and error paths. The finished application frame is not the gRPC EOF: if it escapes first, an upstream renderer can close the generator while the server is still settling the RPC, converting a naturally completed call into CANCELLED. grpc.aio receives the terminal status independently of the message iterator, so wait for that physical boundary before publishing the final frame, and record the settled status on the CLIENT span. * feat(trace): connect DashSc gRPC entry spans Add Dash inference and proxy gRPC boundary spans, W3C propagation, standalone proxy telemetry lifecycle, access-log correlation, and focused normal/error/ cancel coverage. Keep the proxy hops as RPC boundaries without gen_ai model semantics so the platform shows only the downstream real model invocation. Make span finalization exception-safe, cap external request IDs, declare OTel test runfiles, and assert terminal cancellation state rather than timing-racy cancel return values. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(trace): separate frontend and engine token latency TTFT/TPOT previously mixed two unrelated notions on the root SERVER span: AuxInfo engine timings were published as if they were what the caller observed. HTTP and Dash also disagreed, and Dash summed its two phases into one engine TPOT that matched no physical stream. Split the two layers so each span reports only what it can observe: - Streaming HTTP/Dash entry SERVER spans carry the caller-visible timeline, measured at the boundary where the handler is about to yield: gen_ai.response.time_to_first_token and rtp_llm.frontend.time_per_output_token_ms. Role-only, empty, finish-only and internal control frames are not counted as visible tokens. - Each rtp_llm.generate_stream_call CLIENT span carries its own physical stream latency from AuxInfo: rtp_llm.engine.time_to_first_token_ms and rtp_llm.engine.time_per_output_token_ms. Dash phase1 and phase2 therefore report independently instead of being aggregated, and the cross-phase aggregation in access_record is removed. - Non-streaming HTTP writes neither frontend metric: the server cannot observe inter-token delivery when the whole body is sent at once. Request latency remains the SERVER span duration. Two conservative choices avoid publishing numbers we cannot observe: - Frontend TPOT requires two distinct delivery instants. A single frame carrying N>1 tokens exposes no inter-token boundary, so TPOT is omitted rather than reported as 0.0, which a dashboard would read as instant decoding. - For multi-return (n>1), which rides one physical stream, engine TTFT is written only when all sequences agree on it, and engine TPOT is limited to single-sequence streams instead of silently publishing sequence 0 as if it described the whole span. This matches the existing cross-sequence validation in _record_client_span_usage. gen_ai.latency.time_in_model_prefill/decode stay on the root span: they are phase latencies in nanoseconds, disjoint from the TTFT/TPOT keys. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
This change adds end-to-end OpenTelemetry tracing for RTP-LLM inference requests across HTTP, DashSc, frontend, backend, and Prefill/Decode serving paths.
Tracing is disabled by default and designed to fail open: telemetry initialization, propagation, attribute collection, or export failures do not interrupt inference.
What This Adds
OpenTelemetry runtimes
End-to-end context propagation
traceparentcontext from HTTP requests and DashSc gRPC streams.Request and RPC lifecycle tracing
finishedframe arrives before RPC termination.Engine phase visibility
Consistent attributes and status
Compatibility
Related Correctness Fixes
Validation
Coverage includes: