Skip to content

feat: expand time range for query#395

Merged
lzf575 merged 1 commit into
mainfrom
expand_time_range
Apr 24, 2026
Merged

feat: expand time range for query#395
lzf575 merged 1 commit into
mainfrom
expand_time_range

Conversation

@taloric
Copy link
Copy Markdown
Contributor

@taloric taloric commented Feb 28, 2026

No description provided.

@taloric taloric force-pushed the expand_time_range branch from a269dd3 to 0779d4a Compare April 23, 2026 08:15
@taloric taloric marked this pull request as ready for review April 23, 2026 08:15
@taloric
Copy link
Copy Markdown
Contributor Author

taloric commented Apr 23, 2026

@claude 用中文 review 这个PR

@claude
Copy link
Copy Markdown

claude Bot commented Apr 23, 2026

Claude finished @taloric's task in 1m 4s —— View job


a. 事件类型:PR 上的 issue comment 触发,用户要求“用中文 review 这个 PR”。 b. 任务类型:代码评审,不做实现。 c. 关键信息:PR #395,分支 `expand_time_range`,相对 `origin/main` 有 4 个改动文件,核心涉及 `l7_flow_tracing.py`、`tracing_completion.py`、`config.py` 和 `app.yaml`。 d. 主要任务:阅读 diff 与相关文件上下文;检查功能正确性、向后兼容性、配置说明、潜在边界问题;输出中文 review。 e. 计划:先建立清单并同步进度;读取 PR diff 与相关文件;结合仓库 CLAUDE.md 中的 tracing/config 约束完成 review;若无法运行测试则在评论中说明。本次为 review,一般不需要修改代码。 f. 当前未开始测试/运行命令,先完成静态审查。

任务清单

  • 建立评审任务清单并同步评论
  • 阅读 PR 变更与相关文件上下文
  • 整理中文 review 结论并更新评论

@taloric taloric force-pushed the expand_time_range branch 2 times, most recently from 7cc12a5 to 1bc9731 Compare April 24, 2026 08:55
@taloric
Copy link
Copy Markdown
Contributor Author

taloric commented Apr 24, 2026

@claude review 这个 PR

@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Claude finished @taloric's task in 3m 6s —— View job


a. 事件类型:PR 上的 issue comment 触发,要求对当前 PR 做中文 review。 b. 任务类型:代码评审,不做实现。 c. 关键信息:PR #395,分支 `expand_time_range`,相对 `origin/main` 修改了 `app.yaml`、`config.py`、`l7_flow_tracing.py`、`tracing_completion.py`。 d. 主要检查点:时间范围扩展逻辑是否正确;配置项是否完整;是否引入边界条件或返回值不兼容问题。 e. 已执行:阅读 `git diff origin/main...HEAD` 和相关文件上下文,重点检查 `query_and_trace_flowmetas` / `trace_l7_flow` / `TracingCompletion.query`。 f. 未执行:未跑测试,以下结论基于静态代码审查。

任务清单

  • 建立评审任务清单并同步评论
  • 阅读 PR 变更与相关文件上下文
  • 整理中文 review 结论并更新评论

Review 结论

整体思路我认可:把 trace_id 迭代查询的时间窗和后续关联查询时间窗解耦,并通过配置项控制扩展范围,方向是合理的,也符合“默认关闭、保持兼容”的约束。

不过我这里发现了一个需要先修的正确性问题:

  1. query_and_trace_flowmetas() 的返回值个数不一致,会导致调用方解包失败
    在这次改动里,query_and_trace_flowmetas() 的返回类型已经从 2 个值改成了 3 个值:

    • app/app/application/l7_flow_tracing.py:287
    • 调用方解包位置:app/app/application/l7_flow_tracing.py:688

    但是函数里的早退分支仍然保留了旧返回值:

    • app/app/application/l7_flow_tracing.py:320-322

    这里现在还是:

    return set(), app_spans_from_api

    而调用方已经按下面这种方式解包:

    l7_flow_ids, app_spans_from_external, final_time_filter = await self.query_and_trace_flowmetas(...)

    这样一旦命中“初始 flowmeta 查不到”的分支,就会直接抛 ValueError: not enough values to unpack,不是按预期返回空结果。

    这在 tracing completion 场景里尤其容易碰到,因为这里本来就在兼容“trace_id 不存在但仍需继续处理 app spans”的情况。

    Fix this →

其他说明

  • 配置项 iteration_expand_time_range 已同步到 app/app/config.py:34-35app/app.yaml:53-58,这点是好的。
  • 我这次没有再看到其它明确会导致错误的点;修掉上面的返回值问题后,这个 PR 我倾向于继续推进。
  • 本次未运行测试,结论基于静态审查。

@taloric taloric force-pushed the expand_time_range branch from 1bc9731 to ce8b854 Compare April 24, 2026 09:20
@taloric
Copy link
Copy Markdown
Contributor Author

taloric commented Apr 24, 2026

@claude review 这个 PR

@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Claude finished @taloric's task in 2m 34s —— View job


a. 事件类型:PR 上的 issue comment 触发,用户要求“review 这个 PR”。 b. 任务类型:代码评审,不做实现。 c. 关键信息:PR #395,分支 `expand_time_range`,相对 `origin/main` 修改 4 个文件,核心是为多 trace_id/异步场景扩展查询时间窗,并把最终查询时间范围从 `query_and_trace_flowmetas()` 透传到 `trace_l7_flow()`。 d. 主要任务:阅读 PR diff 与相关文件上下文,检查正确性、边界条件、配置兼容性与文档同步情况。 e. 计划:已查看 `git diff origin/main...HEAD`、`app/app/application/l7_flow_tracing.py`、`app/app/application/tracing_completion.py`、`app/app/config.py`、`app/app.yaml`,基于仓库 CLAUDE.md 进行静态审查。 f. 本次未运行测试,以下结论基于静态代码审查。

任务清单

  • 建立评审任务清单并同步评论
  • 阅读 PR 变更与相关文件上下文
  • 整理中文 review 结论并更新评论

Review 结论

这次复查后,我这边没有再看到新的 blocking 问题,之前提到的返回值个数不一致问题已经修掉了:query_and_trace_flowmetas() 的早退分支现在也统一返回 3 个值,和 trace_l7_flow() 的解包保持一致,app/app/application/l7_flow_tracing.py:319-322,685-700

整体看,这个 PR 的改动方向是合理的:

  • 通过 iteration_expand_time_range 做 opt-in 配置,默认值为 0,符合仓库里“默认不改变已有追踪结果”的兼容性要求,app/app/config.py:17-35, app/app.yaml:53-58
  • 扩大的时间窗只用于 trace_id 迭代查询,而不是无差别放大所有关联搜索,能控制误报面,app/app/application/l7_flow_tracing.py:362-399
  • 后续 tcp_seq/syscall/x_request_id/dns 的关联查询会跟随已发现 flow 的实际时间跨度,这和 PR 目标是一致的,app/app/application/l7_flow_tracing.py:573-580

备注

  • trace_l7_flow() 现在把 final_time_filter 传给了 query_all_flows(),不过 query_all_flows() 内部当前仍会基于 _id 重新收缩 time_filterapp/app/application/l7_flow_tracing.py:798-829。这点目前我没看到明确正确性问题,因为最终查询还有 _id IN (...) 约束;但从代码表达上看,后续如果想继续强化“扩展后的全局时间窗”语义,可以再考虑是否需要统一这里的行为。
  • 本次未运行测试,结论基于静态审查。

@lzf575 lzf575 merged commit 9c05ea0 into main Apr 24, 2026
1 check passed
@taloric taloric deleted the expand_time_range branch April 24, 2026 09:49
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.

2 participants