jiangML commented on PR #38911: URL: https://github.com/apache/shardingsphere/pull/38911#issuecomment-4795660690
### Summary **Review Result: Mergeable** **Reason:** The latest PR code consistently splits `routed_result_total` into `routed_storage_unit_total` and `routed_table_total`, adds the `database` label to `routed_sql_total`, and aligns unit tests, E2E cases, and user docs with the intended logical-table route result semantics. ### Evidence - `SQLRouteCountAdvice` now records `routed_sql_total` with `database` and SQL type labels. - `RouteResultCountAdvice` now records storage unit routes and logical table routes as separate metrics. - Unit tests cover the new label sets for `SQLRouteCountAdvice` and `RouteResultCountAdvice`. - Prometheus E2E case files cover JDBC and Proxy expectations for `routed_storage_unit_total` and `routed_table_total`. - Agent and Proxy observability docs list the new metric names and descriptions. ### Review Details **Reviewed Scope:** PR #38911 latest head `4a1a64c66b436e9c40f51a30eea42960db441f36`; base `6b46d9854c4d58b4adaf10ce21faf78f1d33faed`; merge-base `6b46d9854c4d58b4adaf10ce21faf78f1d33faed`. Reviewed all 13 files from GitHub `/pulls/38911/files`; local triple-dot file list matched GitHub scope. **Not Reviewed Scope:** Full Prometheus E2E runtime was not executed locally. **Verification:** - `git fetch apache master refs/pull/38911/head:pr-38911-review`: exit 0 - `./mvnw -pl agent/plugins/metrics/core -DskipTests spotless:check checkstyle:check -Pcheck -T1C`: exit 0 - `./mvnw -pl agent/plugins/metrics/core -DskipITs -Dspotless.skip=true -Dtest=SQLRouteCountAdviceTest,RouteResultCountAdviceTest test`: exit 0 **Release Note / User Docs:** User docs verified. Release note intentionally not treated as a blocker for this review. -- 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]
