Skip to content

feat(server): span X-Request-ID by server log#269

Merged
hittyt merged 2 commits intoalibaba:mainfrom
Pangjiping:feat/span/request-id
Mar 3, 2026
Merged

feat(server): span X-Request-ID by server log#269
hittyt merged 2 commits intoalibaba:mainfrom
Pangjiping:feat/span/request-id

Conversation

@Pangjiping
Copy link
Collaborator

Summary

Testing

  • Not run (explain why)
  • Unit tests
  • Integration tests
  • e2e / manual verification

Breaking Changes

  • None
  • Yes (describe impact and migration path)

Checklist

  • Linked Issue or clearly described motivation FEATURE: span X-Request-ID #267
  • Added/updated docs (if needed)
  • Added/updated tests (if needed)
  • Security impact considered
  • Backward compatibility considered

@jwx0925
Copy link
Collaborator

jwx0925 commented Feb 28, 2026

审查结果(按严重级别排序):

  1. [P1] RequestIdMiddleware 执行顺序与注释不一致,导致 request-id 不能保证覆盖 auth/cors 层日志。
  • 位置:server/src/main.py:95
  • 说明:FastAPI/Starlette 中间件是“后添加先执行”。当前把 RequestIdMiddleware 在 CORS/Auth 之前添加,并不会让它最外层执行。
  • 影响:日志关联覆盖不完整,部分请求链路可能没有 request-id。
  1. [P2] 移除路由函数里的 x_request_id: Header(...) 会让 FastAPI 生成的 OpenAPI 丢失 X-Request-ID 参数。
  • 位置:server/src/api/lifecycle.py 多个接口签名
  • 说明:PR 删除了这些 Header 参数,但 specs/sandbox-lifecycle.yml 仍定义了 X-Request-ID。
  • 影响:生成文档/SDK 可能失去该 header 的显式参数支持,产生契约漂移。

建议:

  • 将 RequestIdMiddleware 调整为最后添加(以便最先执行),或在更早边界注入 request-id。
  • 若要去掉接口签名中的 header,请同步通过全局 OpenAPI 参数等方式保留 X-Request-ID 的契约声明。

@Pangjiping
Copy link
Collaborator Author

Pangjiping commented Feb 28, 2026

2. 移除路由函数里的 x_request_id: Header(...) 会让 FastAPI 生成的 OpenAPI 丢失 X-Request-ID 参数。

日志关联覆盖不完整,部分请求链路可能没有 request-id - 实际上只有fastAPI的access没有日志

移除路由函数里的 x_request_id: Header(...) 会让 FastAPI 生成的 OpenAPI 丢失 X-Request-ID 参数 - fixed this

Copy link
Collaborator

@hittyt hittyt left a comment

Choose a reason for hiding this comment

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

Implemented X-Request-ID middleware and logging correlation. The core logic for injecting the ID into contextvars is sound, but the middleware registration order in main.py needs to be corrected to ensure it is actually the outermost layer as intended.

@hittyt hittyt self-requested a review March 3, 2026 02:53
Copy link
Collaborator

@hittyt hittyt left a comment

Choose a reason for hiding this comment

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

LGTM

@hittyt hittyt merged commit 7425d71 into alibaba:main Mar 3, 2026
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/server feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants