dodjdnh commented on PR #1587:
URL: https://github.com/apache/dubbo-admin/pull/1587#issuecomment-6251051799
> ## 审查意见
> 整体实现比较完整：新路由都挂在 `protected` 下；`GetTraceStartTime` 只接受 16/32 位十六进制 Trace ID
且 Jaeger 地址来自配置，没有 SSRF 风险；`concatURLWithQueryVars` 改为复制 `url.URL`
避免修改共享配置，`GetGrafanaDashboard` 补上缺失的 `return` 也修复了配置缺失时的空指针。我在 PR head 上跑了 `go
test ./pkg/console/service/... ./pkg/config/...`，均通过。以下是几点需要关注的问题：
>
> ### 1. Trace ID 大小写未规范化，ES 面板会查不到日志
> * `pkg/console/service/trace.go`（`GetTraceStartTime` 返回的
`TraceStartTimeResp.TraceID`）与
`pkg/console/service/observability.go`（`GetTraceLogDashboard` 的
`var-traceId`）都原样透传用户输入的大小写；前端又用后端返回的 `traceId` 构造请求。
> * Loki 面板使用 `(?i)` 做不区分大小写匹配，而 ES 面板使用
`trace_id.keyword:"${traceId:raw}"`，`.keyword` 是区分大小写的精确匹配。用户粘贴大写形式的 Trace
ID（`traceIDPattern` 与前端 `isValidTraceId` 都允许）时，Jaeger 能查到、Loki 能查到，但 ES
日志页会静默返回空结果，且没有任何错误提示。
> * 建议：在 `GetTraceStartTime` 与 `GetTraceLogDashboard` 中统一 `strings.ToLower`
后再返回 / 拼接 URL，并在 `trace_test.go`、`observability_test.go` 中补一个大写输入的用例。
>
> ### 2. handler 层改动缺少测试
> * `pkg/console/handler/observability.go`：新增的 `LogDashboard` 分支（应用 / 实例走
`GetApplicationLogDashboard` /
`GetInstanceLogDashboard`）、`GetTraceStartTime`、`GetTraceLogDashboard`，以及配置缺失时新增的
`return`（此前会继续往下执行并解引用 nil 的 `grafanaDashboards`）都没有 handler 级测试。PR
描述中“Relevant Go tests in `service`, `handler`, and `config/console`”与实际不符——本 PR
没有改动任何
`pkg/console/handler/*_test.go`，该包现有测试（`auth_test.go`、`rule_version_test.go`）也与这些改动无关。
> * 建议至少补：`logDashboards` 未配置时返回 `NotFoundError` 且不
panic；`/trace/log-dashboard` 在 `logDashboards.trace` 未配置时的行为。
明白
--
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]
