feat: add trace settings management and UI for enabling/disabling trace logging#4822
feat: add trace settings management and UI for enabling/disabling trace logging#4822
Conversation
There was a problem hiding this comment.
Hey - 我发现了两个问题,并给出了一些整体建议:
- 新的 TracePage 头部文案和开关标签目前是硬编码字符串;建议使用现有的
useModuleI18n('features/trace')实例,这样这些文案就能和控制台的其他部分一样被统一本地化。 - 当前端在 API 调用失败或缺少
trace_enable时,会默认将traceEnabled设为true,但后端的默认配置却将trace_enable设为False;如果能对齐这两个默认值(或显式暴露后端的默认值),就可以避免令人困惑的状态不一致问题。 - 在
log.py中,你对/api/trace/settings注册了两次(分别为 GET 和 POST 规则);可以考虑通过一个带有methods=["GET", "POST"]的单一add_url_rule来简化,并在内部根据request.method分支逻辑,从而减少重复代码。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new TracePage header text and switch labels are hard-coded strings; consider using the existing `useModuleI18n('features/trace')` instance so these messages are localized consistently with the rest of the dashboard.
- The frontend defaults `traceEnabled` to `true` when the API call fails or `trace_enable` is missing, but the backend default config sets `trace_enable` to `False`; aligning these defaults (or surfacing the backend default explicitly) would avoid confusing state mismatches.
- In `log.py`, you register `/api/trace/settings` twice (separate GET and POST rules); you could simplify by using a single `add_url_rule` with `methods=["GET", "POST"]` and branching on `request.method` to reduce duplication.
## Individual Comments
### Comment 1
<location> `astrbot/dashboard/routes/log.py:136-138` </location>
<code_context>
+ if data is None:
+ return Response().error("请求数据为空").__dict__
+
+ trace_enable = data.get("trace_enable")
+ if trace_enable is not None:
+ self.config["trace_enable"] = bool(trace_enable)
+ self.config.save_config()
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `bool(trace_enable)` will treat non-empty strings like "false" as True, which can invert the intended setting.
If the frontend ever sends values like the strings "false" or "0", `bool(trace_enable)` will still be `True`, so the saved config may not match the user’s choice. Consider either using the value directly when `trace_enable` is already a bool, or normalizing known string values (e.g., case-insensitive checks for "true"/"false") before storing.
</issue_to_address>
### Comment 2
<location> `astrbot/core/astr_agent_run_util.py:78-79` </location>
<code_context>
# 用来标记流式响应需要分节
yield MessageChain(chain=[], type="break")
+ json_comp = resp.data["chain"].chain[0]
+ if isinstance(json_comp, Json):
+ tool_info = json_comp.data
+ else:
</code_context>
<issue_to_address>
**issue (bug_risk):** Unconditionally indexing `chain[0]` may raise if the chain is empty or not a list-like.
This used to run only when `show_tool_use` was True; now it runs for every `tool_call_result`. If `resp.data["chain"].chain` is empty or non-indexable, this will raise before sending or tracing the response. Please add a safety check (e.g., length/type check) or default `tool_info = None` when the chain is empty.
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈持续改进代码审查质量。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- The new TracePage header text and switch labels are hard-coded strings; consider using the existing
useModuleI18n('features/trace')instance so these messages are localized consistently with the rest of the dashboard. - The frontend defaults
traceEnabledtotruewhen the API call fails ortrace_enableis missing, but the backend default config setstrace_enabletoFalse; aligning these defaults (or surfacing the backend default explicitly) would avoid confusing state mismatches. - In
log.py, you register/api/trace/settingstwice (separate GET and POST rules); you could simplify by using a singleadd_url_rulewithmethods=["GET", "POST"]and branching onrequest.methodto reduce duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new TracePage header text and switch labels are hard-coded strings; consider using the existing `useModuleI18n('features/trace')` instance so these messages are localized consistently with the rest of the dashboard.
- The frontend defaults `traceEnabled` to `true` when the API call fails or `trace_enable` is missing, but the backend default config sets `trace_enable` to `False`; aligning these defaults (or surfacing the backend default explicitly) would avoid confusing state mismatches.
- In `log.py`, you register `/api/trace/settings` twice (separate GET and POST rules); you could simplify by using a single `add_url_rule` with `methods=["GET", "POST"]` and branching on `request.method` to reduce duplication.
## Individual Comments
### Comment 1
<location> `astrbot/dashboard/routes/log.py:136-138` </location>
<code_context>
+ if data is None:
+ return Response().error("请求数据为空").__dict__
+
+ trace_enable = data.get("trace_enable")
+ if trace_enable is not None:
+ self.config["trace_enable"] = bool(trace_enable)
+ self.config.save_config()
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `bool(trace_enable)` will treat non-empty strings like "false" as True, which can invert the intended setting.
If the frontend ever sends values like the strings "false" or "0", `bool(trace_enable)` will still be `True`, so the saved config may not match the user’s choice. Consider either using the value directly when `trace_enable` is already a bool, or normalizing known string values (e.g., case-insensitive checks for "true"/"false") before storing.
</issue_to_address>
### Comment 2
<location> `astrbot/core/astr_agent_run_util.py:78-79` </location>
<code_context>
# 用来标记流式响应需要分节
yield MessageChain(chain=[], type="break")
+ json_comp = resp.data["chain"].chain[0]
+ if isinstance(json_comp, Json):
+ tool_info = json_comp.data
+ else:
</code_context>
<issue_to_address>
**issue (bug_risk):** Unconditionally indexing `chain[0]` may raise if the chain is empty or not a list-like.
This used to run only when `show_tool_use` was True; now it runs for every `tool_call_result`. If `resp.data["chain"].chain` is empty or non-indexable, this will raise before sending or tracing the response. Please add a safety check (e.g., length/type check) or default `tool_info = None` when the chain is empty.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| trace_enable = data.get("trace_enable") | ||
| if trace_enable is not None: | ||
| self.config["trace_enable"] = bool(trace_enable) |
There was a problem hiding this comment.
issue (bug_risk): 使用 bool(trace_enable) 时,像 "false" 这样的非空字符串会被当作 True,从而反转预期的设置。
如果前端发送的是字符串 "false" 或 "0" 之类的值,bool(trace_enable) 依然会是 True,导致保存下来的配置和用户的选择不一致。建议在 trace_enable 已经是布尔值时直接使用它,或者对常见字符串取值进行规范化处理(例如不区分大小写地判断 "true"/"false")后再存储。
Original comment in English
issue (bug_risk): Using bool(trace_enable) will treat non-empty strings like "false" as True, which can invert the intended setting.
If the frontend ever sends values like the strings "false" or "0", bool(trace_enable) will still be True, so the saved config may not match the user’s choice. Consider either using the value directly when trace_enable is already a bool, or normalizing known string values (e.g., case-insensitive checks for "true"/"false") before storing.
astrbot/core/astr_agent_run_util.py
Outdated
| json_comp = resp.data["chain"].chain[0] | ||
| if isinstance(json_comp, Json): |
There was a problem hiding this comment.
issue (bug_risk): 不加判断地访问 chain[0],在 chain 为空或不是类列表对象时会抛异常。
之前这段逻辑只在 show_tool_use 为 True 时运行;现在会对每一个 tool_call_result 都执行。如果 resp.data["chain"].chain 为空或不可索引,就会在发送或追踪响应之前抛异常。请增加安全检查(例如长度/类型检查),或者在链为空时默认设置 tool_info = None。
Original comment in English
issue (bug_risk): Unconditionally indexing chain[0] may raise if the chain is empty or not a list-like.
This used to run only when show_tool_use was True; now it runs for every tool_call_result. If resp.data["chain"].chain is empty or non-indexable, this will raise before sending or tracing the response. Please add a safety check (e.g., length/type check) or default tool_info = None when the chain is empty.
…ts and status messages
|
已为该 PR 生成文档更新 PR(待人工审核): AI 改动摘要: 根据上游 PR 的改动,我已更新了相关文档。主要改动点如下:
这些更改确保了用户和开发者都能了解到新引入的 Trace 功能及其使用方法。 |
|
已为该 PR 生成文档更新 PR(待人工审核): AI 改动摘要:
|
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
requirements.txt和pyproject.toml文件相应位置。/ I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations inrequirements.txtandpyproject.toml.Summary by Sourcery
添加可配置的追踪记录功能及其界面控制,并细化需要被追踪的事件类型。
New Features:
/api/trace/settings接口启用或禁用追踪记录。trace_enable,用于控制运行时是否发送 trace span。Enhancements:
Original summary in English
Summary by Sourcery
Add configurable trace recording with UI controls and refine which events are traced.
New Features:
Enhancements: