Skip to content

feat(trace): end-to-end OpenTelemetry request tracing - #1341

Merged
netaddi merged 9 commits into
alibaba:mainfrom
SAzwj:feature/rtp-llm-otel-trace
Aug 31, 2026
Merged

netaddi merged 9 commits into
alibaba:mainfrom
SAzwj:feature/rtp-llm-otel-trace

Conversation

@SAzwj

@SAzwj SAzwj commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

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

  • Adds C++ and Python tracing runtimes with OTLP export support.
  • Supports environment-driven endpoint, header, certificate, resource, and sampling configuration.
  • Provides bounded batching, exporter failure diagnostics, and orderly shutdown.
  • Exports backend spans only from the owning tensor-parallel rank to avoid duplication.

End-to-end context propagation

  • Accepts W3C traceparent context from HTTP requests and DashSc gRPC streams.
  • Uses request-body context first at the DashSc boundary, with gRPC metadata as fallback.
  • Propagates trace context across frontend-to-backend and Prefill-to-Decode RPC calls.
  • Keeps propagation data process-safe by passing trace carriers rather than span objects.

Request and RPC lifecycle tracing

  • Creates request-level SERVER spans for HTTP and DashSc entry points.
  • Adds CLIENT and SERVER spans around physical model RPC calls.
  • Records the final physical gRPC status even when the application-level finished frame arrives before RPC termination.
  • Preserves cancellation and generator cleanup semantics for streaming responses.
  • Uses idempotent span guards so concurrent or repeated cleanup cannot finish a span twice.

Engine phase visibility

  • Publishes coherent engine progress snapshots under the stream lock.
  • Records explicit waiting, running, first-token, and generation-complete milestones.
  • Synthesizes scheduler and engine phase spans from those snapshots without moving span objects across worker threads.
  • Distinguishes frontend token latency from engine token latency.
  • Adds PD routing, resource allocation, cache loading, and remote generation visibility.

Consistent attributes and status

  • Defines a shared, bounded attribute schema for request identity, model information, token usage, latency, routing, RPC status, and errors.
  • Avoids prompts, generated content, and other unbounded business payloads.
  • Preserves exact application error codes while mapping physical gRPC outcomes to their corresponding semantic status.
  • Adds trace identifiers to access logs for request-to-trace correlation.

Compatibility

  • Tracing remains opt-in and has no effect when disabled.
  • Instrumentation and exporter failures are fail-open.
  • Existing inference responses, retry policy, FlexLB reporting, and KMonitor metrics remain unchanged.
  • Python tracing degrades to a no-op when OpenTelemetry packages are unavailable.
  • Region and exporter configuration is resolved before child processes start so frontend and backend processes inherit one coherent configuration.

Related Correctness Fixes

  • Tokenizes request-level stop words in both bare and leading-space contexts so byte-level BPE tokenizers stop at the intended boundary.
  • Restores frontend and host-service test isolation after dependency imports were moved to lazy-loading paths.

Validation

Coverage includes:

  • C++ runtime, propagation, exporter, span-guard, and phase-synthesis tests.
  • Python telemetry configuration, propagation, lifecycle, and attribute-schema tests.
  • Model RPC success, failure, cancellation, retry, and terminal-status tests.
  • DashSc multi-frame stream and carrier-selection tests.
  • Frontend streaming, token accounting, and fail-open instrumentation tests.
  • Trace-disabled behavior and exporter failure paths.

@SAzwj
SAzwj requested review from LLLLKKKK and netaddi as code owners August 27, 2026 09:32

@LLLLKKKK LLLLKKKK left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_tasknormal_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

Non-blocking Suggestions

P2

  • opentelemetry 可用性契约自相矛盾,两个 py_test 在公共 lock 下确定性失败而非跳过 @ rtp_llm/cpp/model_rpc/test/model_rpc_client_test.py:735
    • 建议:明确取其一并使四处注释自洽:(1)若公共 lock 确实不带 opentelemetry,则给 ModelRpcClientGrpcMetadataTestTestDependencyContract 加上与其他三处一致的 skipUnless(tracing.OTEL_AVAILABLE, ...),并把 test_tracing.py 的 docstring 改为与 arch_select.bzl 一致;(2)若这两个 target 必须在所有 lock 上验证真实链路,则让公共 telemetry_test_deps() 真正声明 opentelemetry,并同步修正 arch_select.bzl:186rtp_llm/BUILD:453-455 的「保持绿色」表述。无论哪种,都请把 server.start()/insecure_channel() 之后的清理纳入 try/finallycontextlib.AsyncExitStack,保证任何提前失败都能停 server、关 channel 并复位 telemetry。
  • 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.hattributes.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_ERRORRESOURCE_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 属性。
  • arch_config 新增 telemetry_test_deps() 扩展点,需确认内部所有 override 变体已同步 @ arch_config/arch_select.bzl:185
    • 建议:在 PR description 中注明内部 arch_config 全部 override 变体已同步新增 telemetry_test_deps(),或提供对应改动链接;并确认内部实现返回值与开源侧空列表在语义上兼容(内部 lock 提供 opentelemetry 运行时)。函数注释可补一句返回值契约:必须返回 list。
  • 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 的确切约束来源与已实测通过的组合。
  • 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 不含第一次」的用例。
  • 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 只传 PhaseRoleerror_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 上的单一方法供两侧共享。
  • 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 = Trueroute_source = "master"。空列表场景会正确落到 "domain_fallback",并顺带为该异常保留可观测信号。
  • route span 未把取消归一化为 Cancelled,客户端断连计入路由错误率 @ rtp_llm/server/backend_rpc_server_visitor.py:488
    • 建议:在 except 分支补一条与其他埋点一致的分类:对 asyncio.CancelledError/GeneratorExitroute_error_type = "Cancelled",其余非 FtRuntimeException 走一个固定值(如 "RouteError"),使取值域保持有界,并在 _FakeRouteSpan 记录 error_type 后补断言。
  • 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") 断言「异常被吞掉的同时确实留下可排查日志」。
  • WORKSPACE 内联第三方 http_archive 绕过既有依赖分层与内源覆盖点 @ WORKSPACE:5
    • 建议:将该 http_archive 迁入 deps/http.bzlhttp_deps()(patches 标签用 clean_dep(...),与 deps/git.bzlzlib.BUILD 的写法一致),或按 xgrammar 先例新增 3rdparty/opentelemetry_cpp/repositories.bzlWORKSPACE 只保留 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,并把测试内的字面量同步换成常量引用,使重命名在导入期或测试中立即暴露。
  • 抢占终结路径未传 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
  • 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 一次性确立不变量后三处都直接解引用并去掉宏内判空。
  • 空 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 提供」这一意图只存在一处。
  • in_memory_span_exporter 被加入包级共享 test_deps,实际只有一个 cc_test 使用 @ rtp_llm/cpp/model_rpc/test/BUILD:51
    • 建议:与同包 query_converter_testdeps = 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:2rtp_llm/telemetry/test/BUILD:1rtp_llm/frontend/test/BUILD:1rtp_llm/dash_sc/test/BUILD:1WORKSPACE: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.0opentelemetry-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_cppWORKSPACE 中唯一的内联 http_archive(:5-19);其余第三方归档全部集中在 @rtp_depsdeps/http.bzl/deps/git.bzl,需要后续 *_deps() 宏的依赖走仓内包装文件(3rdparty/xgrammar/repositories.bzl,:49-51 有先例)。更实际的后果是:@rtp_depslocal_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 NoneExceptionType 是 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_testprefill_rpc_server_testdecode_rpc_server_testresponse_buffer_testprefill_batch_rpc_server_testrpc_server_runtime_meta_testquery_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-230PrefillRpcServer.cc:771-780DecodeRpcServer.cc:1424-1433 三处逐字段抄写同一份 PhaseTiming(9 行完全相同赋值),仅 PhaseRoleerror_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-18MALLOC_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:52findSpan)与 grpc_propagation_test.cc:41findSpanByName)都提供了按名查找工具,本文件却没有采用;一旦 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:52findSpan)与 grpc_propagation_test.cc:41findSpanByName)都提供了按名查找工具,本文件却没有采用;一旦 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 NoneExceptionType 是 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_shutdownspatch.object 整体替换 bg_app._init_trace_telemetry/_shutdown_trace_telemetry,断言只有两条无参 assert_called_once_with()(:237-238),因此写进用例名的「dash role」完全未被验证。真正的角色/rank 契约在 rtp_llm/dash_sc/app.py:70init_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:64trace_span_guard 刻意声明在 error_status(:55)之后,利用成员逆序析构保证 guard 析构时仍能读到最终 gRPC 状态,并在注释中写明这一容易被后续改动破坏的隐性约束;phase_span_scopepd_client_span_guard 的相对声明顺序同样使 CHECK_ERROR_STATUS 提前 return、异常展开、~PrefillGenerateContext() 中的 closeGrpcStream() 都落在正确时序。
  • 严格 fail-open 契约:startRpcServerSpan/startChildClientSpan/synthesizeChildSpan 全部 try/catch(...) 兜底,TelemetryRuntime::isActive() 前置短路,RequestSpanGuard::finish() 由原子标志保证 exactly-once 且析构 noexcepttracing.pyOTEL_AVAILABLE 为假时整体退化 no-op;PhaseSpanSynthesisScope 对回调抛异常与回调为 nullptr 两种情形都有针对性用例。
  • 属性值一律取自闭集:grpcStatusCodeName/grpcStatusDescription/ErrorCodeToString,显式避免把 error_message() 原始文本写进 span,兼顾低基数与敏感信息不外泄;attributes.py:103-126 建立 C++/Python 单一注册表并配 parity 测试,把两侧漂移从口头约定变成可失败的测试。
  • RpcServerRuntimeMetastream->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:1064DoneGuard + 双向握手 + 墙钟兜底使生产者失败不会挂死整个 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:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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_tasknormal_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

@LLLLKKKK LLLLKKKK left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Code Review - PR #1341 (non-blocking suggestions)

31 条 P2/P3 建议,不阻塞合并。阻塞判定与完整摘要见上一条 review。

await server.start()
channel = grpc.aio.insecure_channel(f"127.0.0.1:{port}")

from opentelemetry.sdk.trace.export.in_memory_span_exporter import (

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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,则给 ModelRpcClientGrpcMetadataTestTestDependencyContract 加上与其他三处一致的 skipUnless(tracing.OTEL_AVAILABLE, ...),并把 test_tracing.py 的 docstring 改为与 arch_select.bzl 一致;(2)若这两个 target 必须在所有 lock 上验证真实链路,则让公共 telemetry_test_deps() 真正声明 opentelemetry,并同步修正 arch_select.bzl:186rtp_llm/BUILD:453-455 的「保持绿色」表述。无论哪种,都请把 server.start()/insecure_channel() 之后的清理纳入 try/finallycontextlib.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) :

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.hattributes.py 注释作为单一事实来源,并补一条 beam 请求的 usage 断言用例。

auto new_task = makeTaskInfo(TaskIdentity{identity.request_id, batch_id},
stream->prefixLength(),
stream->inputLength(),
time_info.wait_time_us / 1000);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Decode 加载 KV 失败回传的 gRPC status 改为错误码映射,会触发 Prefill 侧精确 ErrorCode 被覆写

旧代码恒返回 grpc::StatusCode::INTERNAL,现为 grpc::Status(transErrorCodeToGrpc(error_info.code()), ...)RpcErrorCode.h:13-18MALLOC_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_ERRORRESOURCE_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():

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:2rtp_llm/telemetry/test/BUILD:1rtp_llm/frontend/test/BUILD:1rtp_llm/dash_sc/test/BUILD:1WORKSPACE: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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:52findSpan)与 grpc_propagation_test.cc:41findSpanByName)都提供了按名查找工具,本文件却没有采用;一旦 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)),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] 用字面量 getattr 读取动态挂载的 rtp_error_code

int(getattr(e, "rtp_error_code", e.exception_type)) 以字符串字面量做属性探测。根因是 rtp_error_code 并非 FtRuntimeException 的声明字段,而是 :476 动态挂载的。当前取值安全(:474 已保证 error_code is not NoneExceptionType 是 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 访问

Comment thread rtp_llm/cpp/model_rpc/QueryConverter.cc
query_info.cancel_after_response_count is not None
and response_count >= query_info.cancel_after_response_count
):
cancel_requested = True

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 的诊断信息而不是留一个被覆盖的局部变量。

Comment thread rtp_llm/cpp/engine_base/stream/test/GenerateStreamTest.cc Outdated
@SAzwj
SAzwj force-pushed the feature/rtp-llm-otel-trace branch from 3fc322e to b3de40f Compare August 27, 2026 12:11

@LLLLKKKK LLLLKKKK left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ImportErrorself.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-189rtp_llm/BUILD:453-455 的注释,使三处对「运行时来源」的描述自洽。
  • 遥测结算 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() 统一取消。
  • Decode 加载 KV 失败回传的 gRPC status 改为错误码映射,会覆写 Prefill 侧精确 ErrorCode @ rtp_llm/cpp/model_rpc/DecodeRpcServer.cc:313
    • 建议:在 PR 描述中把该 P→D 终态契约变更单列为独立行为变更,并列出受影响指标(cancel_qpserror_qpserror_code 分布);补一条用例固化「decode 返回 RESOURCE_EXHAUSTED / DEADLINE_EXCEEDED 时 Prefill 最终上报的 ErrorCode」。若目的仅是让 span 的 error.type 更精确,更小的改法是保留 INTERNAL、只把 error_info.code() 写进 span 属性——phaseErrorType 已能独立从 error_info 识别 DependencyFailure
  • 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 且长度不等的用例。
  • 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 的死赋值。
  • PD 分离判定在 Python 侧重复实现,rtp_llm.pd_sep 存在两个事实来源 @ rtp_llm/cpp/model_rpc/model_rpc_client.py:56
    • 建议:优先让服务端把实际分流结果作为可观测数据回传(Prefill SERVER span 已能写入 rtp_llm.pd_sep),客户端只读取不重算;若必须保留客户端重算,请补一个跨语言一致性测试,并在注释中登记 C++ 侧文件与行号锚点。
  • 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 记录已验证组合。
  • 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.bzlhttp_deps(),patch 标签改为 @rtp_llm//patches/opentelemetry_cpp:...;WORKSPACE 只保留 loadopentelemetry_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" 的用例。
  • 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
  • P→D CLIENT span 的阶段耗时属性在重试时跨 attempt 累加 @ rtp_llm/cpp/model_rpc/PrefillGenerateContext.cc:149
    • 建议:在写 span 属性前减去上一 attempt 的快照基线(在 remoteAllocateResource 重建 guard 时记录 stat_info 基线),或把这三个属性改名为明确的累计语义并在文档中写明;kmonitor 侧若依赖累计口径请保留独立字段。补一条覆盖 retry_times>0 时属性值的用例。
  • 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 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) 调用(或直接把本用例改为 patch init_telemetry,使名称与断言一致);test_init_failure_is_fail_openassertLogs 断言降级告警。
  • 新增 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 生成方式后可删除),并在 WORKSPACEpatches 行加一句指向该说明的注释。
  • protobuf 黑名单过滤把 map key 复制成魔法字符串 @ 3rdparty/protobuf/BUILD:890
    • 建议:把被排除的 key 提为文件级常量(如 _PLUGIN_PROTO_KEY = "compiler_plugin")并在推导前断言其存在于 WELL_KNOWN_PROTO_MAP,使上游改名在 load 阶段即暴露。
  • 端点校验用例依赖 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.bzl load 该符号:rtp_llm/cpp/model_rpc/test/BUILD:2rtp_llm/telemetry/test/BUILD:1rtp_llm/frontend/test/BUILD:1rtp_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.bzl load 该符号:rtp_llm/cpp/model_rpc/test/BUILD:2rtp_llm/telemetry/test/BUILD:1rtp_llm/frontend/test/BUILD:1rtp_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 行专门 load http_archive,全文仅此一处;其余为两个 local_repository + 各 *_deps() 调用),而全部三方仓集中在 @rtp_depsdeps/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/.hotlp_preferred_temporality.hmetrics_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 == nullptrPrefillGenerateContext.cc:304-323finalized_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_codeOptional[int],因此当前不存在 int() 抛异常覆盖原始路由异常的风险,但这一安全性完全依赖「未来没人把非 int 赋给该属性」。
  • [6.1] Quality — Mega-PR 已拆分为独立变更 → issue enqueue 等待时长单位由微秒改为毫秒,WorkerStatus 对外取值缩小约 1000 倍
    enqueue 原先把 wait_time_us 直接作为 makeTaskInfowaiting_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/.hotlp_preferred_temporality.hmetrics_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_testquery_converter_testbroadcast_manager_testprefill_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_testquery_converter_testbroadcast_manager_testprefill_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-29roleTypeToString() 已是同一枚举的既有唯一映射,并已被 ConfigModules.ccLocalRpcServer.cc 复用。该 role 直接决定 service.name = "rtp_llm_" + rolertp_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-249PrefillRpcServer.cc:761-792DecodeRpcServer.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 Nonerpc_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,风险潜伏;一旦有 .cu TU
  • [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 == nullptrPrefillGenerateContext.cc:304-323finalized_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/.hotlp_preferred_temporality.hmetrics_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-457backend_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_codeOptional[int],因此当前不存在 int() 抛异常覆盖原始路由异常的风险,但这一安全性完全依赖「未来没人把非 int 赋给该属性」。
  • [P.G] 测试规范 — mock/fake/stub 不得替代本次声称覆盖的生产边界 → issue dash_sc telemetry 生命周期用例把 role/rank 接线整体 mock,fail-open 用例零断言
    test_start_initializes_dash_role_and_shutdownspatch.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_usageinput_len 不一致时整体放弃写入,宁缺勿错。
  • RpcServerRuntimeMeta 锁序与身份校验改进扎实:enqueue/dequeue/markPriorityPreemptionCanceledcaptureStreamRuntimeSnapshot(内部访问 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。
  • 新增逻辑刻意做成可测纯函数:phaseErrorTypegenerateRequestReadFailureStatus_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 (

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:32telemetry_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 ImportErrorself.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-189rtp_llm/BUILD:453-455 的注释,使三处对「运行时来源」的描述自洽。

Comment thread rtp_llm/cpp/model_rpc/model_rpc_client.py Outdated
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());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Decode 加载 KV 失败回传的 gRPC status 改为错误码映射,会覆写 Prefill 侧精确 ErrorCode

该行由固定 grpc::StatusCode::INTERNAL 改为 grpc::Status(transErrorCodeToGrpc(error_info.code()), ...)。按 RpcErrorCode.h:12-31MALLOC_FAILED/DECODE_MALLOC_FAILED/PRIORITY_PREEMPTEDRESOURCE_EXHAUSTEDLOAD_CACHE_TIMEOUT 等 → DEADLINE_EXCEEDEDCANCELLEDCANCELLED。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_qpserror_qpserror_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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] enqueue 等待时长单位由微秒改为毫秒,WorkerStatus 对外取值缩小约 1000 倍

enqueue 原先把 wait_time_us 直接作为 makeTaskInfowaiting_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(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] OTel 裁剪 patch 缺少动机与撤销条件说明,并使上游 metrics exporter 目标不可构建

该 patch 从上游 exporters/otlp:otlp_recordable 中删除 otlp_metric_utils.cc/.hotlp_preferred_temporality.hmetrics_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 生成方式后可删除),并在 WORKSPACEpatches 行加一句指向该说明的注释。

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 时才按外部兼容性处理

Comment thread 3rdparty/protobuf/BUILD
# 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 = [

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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] 同一功能用统一工具函数

Comment thread rtp_llm/cpp/model_rpc/QueryConverter.cc
@LLLLKKKK
LLLLKKKK dismissed their stale review August 27, 2026 13:11

LGTM:阻断已解除(rtpcli 自动清除旧红标)

@SAzwj
SAzwj force-pushed the feature/rtp-llm-otel-trace branch 4 times, most recently from 765c6a0 to de767be Compare August 28, 2026 08:53
SAzwj and others added 9 commits August 28, 2026 17:39
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.
@SAzwj
SAzwj force-pushed the feature/rtp-llm-otel-trace branch from de767be to 51869b4 Compare August 28, 2026 09:48

@LLLLKKKK LLLLKKKK left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_TIMEOUTCACHE_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 用例组。
  • Decode 端 P→D 终态 gRPC status 改为 ErrorCode 映射,会触发 Prefill 侧 RESOURCE_EXHAUSTED 改写并缺少 handler 级覆盖 @ rtp_llm/cpp/model_rpc/DecodeRpcServer.cc:319
    • 建议:补 handler 级用例,断言 LOAD_CACHE_TIMEOUT 下返回 DEADLINE_EXCEEDEDCANCELLED 下返回 CANCELLED,并显式覆盖会被改写的 RESOURCE_EXHAUSTED 分支;给 PrefillRpcServer.cc:147 的改写加「仅当 PB 未给出业务错误码时才按传输码改写」的约束;并在 PR 描述中记录「P→D RemoteGenerate 终态码由固定 INTERNAL 变为按 ErrorCode 映射」及 decode 侧 cancel_qps / error_code 维度迁移,便于按 gRPC code 建看板/告警的一方同步。
  • 异步 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 作用域写法。
  • 流式 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 状态;同步调整对应用例的期望值。
  • 每个被 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=Falseself.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 编码」的依据与复验方式。
  • 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」,避免后续排查误判为配置问题。
  • 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 三条单段路径。
  • 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 的用量聚合用例固化期望值。
  • 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),把五处收尾统一为一次调用并用参数表达差异,便于对收尾矩阵做参数化断言。
  • 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 描述中说明内部 ppu config 是否已实测通过。
  • 同一 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 私有键。
  • 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 属性)。
  • 用字面量 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 判定阈值;若存在,请同批调整或给出灰度顺序。
  • 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 用例中补上同样的入参断言,使用例名的承诺与断言一致。
  • createDecoderStream 新增的 num_return_sequences 参数无任何调用方 @ rtp_llm/cpp/engine_base/stream/test/GenerateStreamTest.cc:76
    • 建议:在本 PR 内补一个使用该参数、断言多返回序列下 usage 聚合结果的用例(正好可覆盖上文 returned_sequence_count 恒等三元);否则删除形参与 :79 赋值,保持测试辅助函数与实际用法一致。
  • 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.pyimport requests 提升到模块级,再 patch rtp_llm.server.host_service.requests.post
  • 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_mscaptureStreamRuntimeSnapshotcomputeExecutionTimeMs(: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,而 closeGrpcStreamstat_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 报文尚未到达」。该等式成立只是因为 DecodeGenerateContextrequest_id=0 构造(RemoteGenerate 里紧挨着就写着 TODO request id is 0 here)、GenerateContextrequest_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_mscaptureStreamRuntimeSnapshotcomputeExecutionTimeMs(: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:339import 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-249PrefillRpcServer.cc:761-792DecodeRpcServer.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/BUILDselect 地依赖它、pybind/BUILD:115 又把该 target 放进 th_transformer_lib 的基础 deps,故所有 config 都构建 OTel SDK。已逐条核对该缺陷机制(shlex.quote / pipes.quote 吞掉带括号的 -D只存在于 3rdparty/gpus/crosstool/.../crosstool_wrapper_driver_rocm.tpl3rdparty/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:70 init_telemetry("dash_sc", 0) 中,它决定导出的 service.namertp_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 == nullptrhas_stream_snapshot 为 false)时也不 erase。若调用方按 :129 的注释「无本地流时传 null」传入 nullptr 而 running_streams_[request_id] 因故仍存在,该条目将永久滞留(running_streams_ 无 TTL 清理),虚增 WorkerStatus 的 running 负载并影响调度。逐条核对现有调用路径后确认当前不可达,属契约依赖而非现网缺陷。

RTP-LLM Checklist

  • [I] 代码质量 — 同一功能用统一工具函数 → issue telemetry_test 依赖 span 导出顺序做下标匹配,而非按名字查找
    ClientSpanCanonicalizesAndValidatesEndpointASSERT_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 不对」这类误导性错误。

Python Static-First Checklist

  • [P.A] 静态结构与类型纪律 — 字符串分发用 Enum/Literal → issue rtp_llm.route.source 的低基数枚举契约未被生产代码约束,唯一引用它的断言恒真
    attributes.py:80-95RTP_LLM_ROUTE_SOURCE_VALUES 显式声明该属性的 6 个合法取值(注释强调 low-cardinality enum),但生产代码在 backend_rpc_server_visitor.py:453-461 等处全部使用裸字符串字面量赋值 route_source,从未与该集合比对。全仓检索确认该 frozenset 仅有一处引用:backend_rpc_server_visitor_test.py:392assertIn("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:339import 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:70 init_telemetry("dash_sc", 0) 中,它决定导出的 service.namertp_llm_dash_sc),全仓无任何用例断言过这两个实参;test_init_failure_is_fail_open(:241-245)也只验证不抛异常,但已证明 bg_app.init_telemetry 可被直接 patch。

Strengths

  • span 生命周期设计有注释自证且经得起核对:GenerateContext.h:61-64RpcTraceHelper.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 / 1000captureStreamRuntimeSnapshotcomputeExecutionTimeMs 的毫秒口径统一,修掉了 waiting_time_ms 字段被填入微秒的单位缺陷;running->second.stream == stream 身份校验(:152/:183/:268)阻止重试产生的替换流被旧路径误消费;dequeue(:177-178)补 null 提前返回,消除空指针解引用路径。
  • 把 stream 指标采集抽成 captureStreamRuntimeSnapshot 并移出 read_write_lock_,与既有 getEngineScheduleInforead_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 <= 0end <= begin 一律跳过而非钳位),避免把错乱时间戳渲染成看似合理的瀑布图;setUsageTokenAttributes(RpcTraceHelper.h:278)对任一侧非正值整体跳过五键组,并有 UsageTokenAttributesSkipNonPositiveValues 显式固化该策略。
  • stream_responseGeneratorExitasyncio.CancelledError 合并捕获(frontend_server.py:306),顺带修掉了旧代码在 except BaseExceptionyield 从而触发 async generator ignored GeneratorExit 的真实缺陷;_handle_grpc_errorExceptionType(error_code) 包进 try/except ValueError 降级为 UNKNOWN_ERROR,消除非法 error_code 抛异常覆盖真实 RPC 错误的既有隐患。
  • 「渲染器主动完成」有真实握手:custom_renderer.pybreakmark_renderer_completed(),再由 finally: await output_generator.aclose() 关闭后端流,使 _request_completed_normally 能把停用词截断与真实断连区分开,而不是按异常类型猜测。
  • 3rdparty/protobuf/BUILD:883-894blacklisted_protos 修正经逐条核对精确正确:列出的 WKT 已链入 :protobufcompiler/plugin.pb.cc 只在 :protoc_lib,故排除 compiler_plugin 必要且充分,注释完整解释了 bazel 6 的 ProtoInfo 约束成因。
  • 依赖声明不吃传递闭包:model_rpc/BUILD(cc 与 py)、server/BUILDpybind/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()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Decode 阶段 span 的 error.type 会把 KV 加载完成之后的本地超时误标为 DependencyFailure

phaseErrorType(:111-114)用 stage == DecodeStatInfo::loadCacheFromPrefill 判定「上游依赖失败」。但 EXECUTE_STAGE_FUNC(GenerateContext.h:105-109)展开顺序是先 CHECK_REQUEST_STOPnextStage(),因此 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_TIMEOUTCACHE_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();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📍 实际位置 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 标记与状态码。但 loadCacheFromPrefillgrpc_stream->Read(&load_request)(:277-278)仍走旧宏,固定得到 INTERNAL "failed to get loadReqeust" 且不写 error_info。于是同一个客户端取消事件:allocate/generate 阶段被分类为 Cancelled 并计入 cancel_qpsGenerateContext::cancelled() 判定 error_code == CANCELLED),load 阶段却是 Internal,phaseErrorTypeerror_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());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Decode 端 P→D 终态 gRPC status 改为 ErrorCode 映射,会触发 Prefill 侧 RESOURCE_EXHAUSTED 改写并缺少 handler 级覆盖

原先 loadCacheFromPrefill 失败恒返回 INTERNAL,现改为 grpc::Status(transErrorCodeToGrpc(error_info.code()), ...);读流失败(:200、:333)也从固定 INTERNAL 改为按 isRequestCancelled() 返回 CANCELLEDRpcErrorCode.h:13-18MALLOC_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_EXCEEDEDCANCELLED 下返回 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(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] telemetry_test 依赖 span 导出顺序做下标匹配,而非按名字查找

ClientSpanCanonicalizesAndValidatesEndpointASSERT_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:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.namertp_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) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] host_service_test 的 patch target 修复属无关变更,且该目标仍为 manual 无 CI 保护

8 处装饰器由 patch("rtp_llm.server.host_service.requests.post") 改为 patch("requests.post")。这是必要修复而非等价改写:host_service.py:339import 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.pyimport 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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] 回滚路径:风险行为存在运维回滚手段

@netaddi
netaddi merged commit b7a5437 into alibaba:main Aug 31, 2026
2 of 10 checks passed
@SAzwj
SAzwj deleted the feature/rtp-llm-otel-trace branch August 31, 2026 05:15
ningweikang pushed a commit to ningweikang/rtp-llm that referenced this pull request Sep 2, 2026
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants