dodjdnh commented on PR #1587:
URL: https://github.com/apache/dubbo-admin/pull/1587#issuecomment-6380426999
> ## 结论：可以合入，修掉一处数据竞争是本 PR 的额外收益
> 37 个文件，`changed_files=37` 与 Diff 一致；Go 侧改动集中在可观测性配置与 dashboard URL 生成，UI
侧新增日志页签与 Trace ID 搜索。
>
> ### 做得好的地方
> * `concatURLWithQueryVars` 现在复制 `*url.URL` 再改 `RawQuery`。原实现直接修改配置里的
`baseURL`，在并发请求下是对共享配置对象的写竞争；这个修复虽然没有写在 PR 标题里，但价值很高。
> * `GetGrafanaDashboard` 在 `grafanaDashboards == nil` 之后补了
`return`。原代码只写了错误响应却继续往下走，会拿 nil 指针继续拼 URL。
> * `GetTraceStartTime` 的安全边界比较完整：Trace ID 限定 16/32 位十六进制、Jaeger 响应限制 16
MiB、客户端 5s 超时、校验返回的 Trace ID 与请求一致、`earliest/1000`
正确完成微秒到毫秒的换算。`fmt.Sprintf("%032s", ...)` 的补零对比我单独验证过，确实按零填充（`[%032s]` →
`000…abc123`），16 位与 32 位 ID 的等价判断成立。
>
> ### 已核对的转义设计
> `GetApplicationLogDashboard` 对应用名做 `regexp.QuoteMeta` 后再 `strconv.Quote`
并去掉外层引号，结果是 `shop.user` → `shop\.user`（经过 JSON 解码后是 `shop\\.user`）。这个双重转义不是
bug：仪表盘里 `${application:raw}` 位于 LogQL
双引号字符串内，字符串层先消费一个反斜杠，正则层再消费第二个，最终匹配字面量点号。文档里对这条契约写得很清楚，测试也断言了 `shop\\.user`。
>
> ### P3 · Trace 搜索只有单元测试
> `GetTraceStartTime` / `GetTraceLogDashboard` 的测试走
`httptest`，覆盖了状态码、超大响应、Trace ID 不匹配等分支，这部分很好；但没有对着真实 Jaeger 的 16 位 ID
返回做一次验证。文档里提到的「zero-padded 32 位与 16 位等价」属于 Jaeger 侧行为，建议在验收记录里补一条真实查询结果。
>
> ### P3 · `GetApplicationLogDashboard` 未校验空应用名
> `req.AppName` 为空时会生成 `var-application=""`，最终是空面板。既有 `GetAppDashboard`
也没有校验，属于一致行为，可以不改；如果要改，建议两个入口一起加。
>
> ### P3 · 时间范围校验不检查上界
> `GetTraceLogDashboard` 校验了 `to > from` 和跨度 ≤ 10 分钟，但没有拒绝明显在未来的
`to`。Grafana 会自行处理，影响很小，记录一下。
明白
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
