fix SofaRpcMetricsCollector npe #1460
Conversation
sofastack-cla
bot
added
bug
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
metrics/metrics-prometheus/src/main/java/com/alipay/sofa/rpc/metrics/prometheus/SofaRpcMetricsCollector.java (1)
Line range hint
203-212: Consider enhancing type safetyWhile the current implementation handles null, Integer, and Long cases, consider adding support for other number types and better error handling.
Here's a more robust implementation:
private static long getLongAvoidNull(Object object) { if (object == null) { return 0L; } - if (object instanceof Integer) { - return Long.parseLong(object.toString()); + if (object instanceof Number) { + return ((Number) object).longValue(); } - return (Long) object; + try { + return Long.parseLong(object.toString()); + } catch (NumberFormatException e) { + return 0L; + } }This change:
- Handles all Number types (Integer, Long, Double, etc.)
- Safely parses string representations
- Falls back to 0L for unparseable values
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
metrics/metrics-prometheus/src/main/java/com/alipay/sofa/rpc/metrics/prometheus/SofaRpcMetricsCollector.java(1 hunks)
🔇 Additional comments (2)
metrics/metrics-prometheus/src/main/java/com/alipay/sofa/rpc/metrics/prometheus/SofaRpcMetricsCollector.java (2)
203-205: LGTM! The NPE fix looks good.
The change from Long to long with 0L default effectively prevents NPE in metrics collection, which aligns with the PR objective. This is a safe change since zero is a reasonable default value for missing metrics data.
Line range hint 203-212: Verify metrics collection behavior
Let's verify that all callers properly handle the primitive long return type and that the change doesn't affect metrics collection.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
metrics/metrics-prometheus/src/main/java/com/alipay/sofa/rpc/metrics/prometheus/SofaRpcMetricsCollector.java (2)
Line range hint
203-211: LGTM! The NPE fix looks good with a minor improvement suggestion.The change from
Longtolongeffectively prevents NPEs by returning0Lfor null inputs, which is appropriate for metrics collection. However, the type handling could be more robust.Consider making the number handling more robust:
private static long getLongAvoidNull(Object object) { if (object == null) { return 0L; } - if (object instanceof Integer) { + if (object instanceof Number) { - return Long.parseLong(object.toString()); + return ((Number) object).longValue(); } - return (Long) object; + try { + return Long.parseLong(object.toString()); + } catch (NumberFormatException e) { + return 0L; + } }This change:
- Handles all Number types (Integer, Long, Double, etc.)
- Safely parses string representations
- Provides a fallback for unparseable values
Inconsistent null handling in
getLongAvoidNull
SofaRpcMetricsCollector.javareturns0Lwhen nullSofaRpcMetrics.javareturnsnullwhen null
Review and standardize the null handling behavior to ensure consistent metrics collection.🔗 Analysis chain
Line range hint
203-211: Verify metrics collection behavior with the new changesThe change affects how metrics are collected when null values are encountered. Let's verify the impact on metrics collection and reporting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential issues with metrics collection # Look for other metric collectors that might expect null values rg -l "implements.*Collector" --type java # Check for other methods that might be affected by getLongAvoidNull rg "getLongAvoidNull" --type java -B 2 -A 2 # Look for test cases involving metrics collection rg -l "Test.*Metrics|Metrics.*Test" --type javaLength of output: 6953
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
metrics/metrics-prometheus/src/main/java/com/alipay/sofa/rpc/metrics/prometheus/SofaRpcMetricsCollector.java(1 hunks)
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no suggestions.
Motivation:
修复SofaRpcMetricsCollector中的npe问题
Modification:
getLongAvoidNull方法取不到时返回0L，不返回null
Result:
Fixes #1459.
Summary by CodeRabbit