[ISSUE #10683] Validate proxy metric collector address - #10684
Conversation
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Validates metricCollectorAddress in GrpcClientSettingsManager.mergeMetric before parsing, preventing runtime exceptions when the address is empty, malformed, or has a non-numeric port.
Findings
- [Info]
GrpcClientSettingsManager.java:143-158— The newparseMetricCollectorAddressmethod handles all edge cases: blank input, missing colon separator, empty host/port, and non-numeric port. Returnsnullfor invalid input, allowing the caller to gracefully disable the metric collector. - [Info]
GrpcClientSettingsManager.java:121-126— The caller correctly handlesnullby logging a warning and settingmetricBuilder.setOn(false), which prevents the client from attempting to connect to an invalid endpoint. - [Info] Uses
StringUtils.isBlank()from commons-lang3, consistent with existing dependencies in the project. - [Info]
GrpcClientSettingsManagerTest.java— Tests cover valid address and multiple invalid scenarios (blank, missing port, non-numeric port). Good coverage.
Suggestions
- [Info] Consider making
parseMetricCollectorAddressprivateinstead ofprotectedif external subclassing is not intended. Minor style point. - [Info] The
split(":")approach assumes IPv4 addresses. If IPv6 support is needed in the future, consider using a more robust parsing approach (e.g.,InetSocketAddress.createUnresolved()). Not blocking for this PR.
Overall: Solid defensive improvement. Prevents a potential runtime exception in the gRPC client settings path when the metric collector is enabled but the address is misconfigured.
Automated review by github-manager-bot
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #10684 +/- ##
=============================================
- Coverage 48.31% 48.26% -0.06%
+ Complexity 13511 13498 -13
=============================================
Files 1380 1380
Lines 101091 101100 +9
Branches 13101 13103 +2
=============================================
- Hits 48844 48795 -49
- Misses 46285 46320 +35
- Partials 5962 5985 +23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot (Re-review)
PR #10684: [ISSUE #10683] Validate proxy metric collector address
Re-review scope: New commit 621a345 — "Cover blank metric collector port"
Changes Since Last Review
- Added test case for blank metric collector port value — covers the edge case where the address has a colon but the port portion is empty or whitespace
Assessment
Tests — Good additional coverage for the blank-port edge case in parseMetricCollectorAddress.
Overall: LGTM — Test coverage is now comprehensive for all validation branches.
Automated re-review by github-manager-bot
Which Issue(s) This PR Fixes
Fixes #10683
Brief Description
GrpcClientSettingsManager.mergeMetricpreviously parsedmetricCollectorAddresswithsplit(":")and directly readsplit[1]. WhenmetricCollectorMode=onbut the configured address was empty, missing a port, or using a non-numeric port, the gRPC client settings path could throw a runtime exception while constructing the client metric endpoint.This PR validates the configured collector address before building the endpoint. Invalid addresses now disable the client metric collector for the settings response and log a warning, while valid
host:portvalues keep the existing behavior.How Did You Test This Change?
mvn -pl proxy -Dtest=GrpcClientSettingsManagerTest -DfailIfNoTests=false testResult:
BUILD SUCCESS,Tests run: 9, Failures: 0, Errors: 0, Skipped: 0.