RongtongJin commented on code in PR #10539:
URL: https://github.com/apache/rocketmq/pull/10539#discussion_r3496681474
##########
remoting/src/main/java/org/apache/rocketmq/remoting/metrics/RemotingMetricsManager.java:
##########
@@ -87,6 +95,37 @@ public List<Pair<InstrumentSelector, ViewBuilder>>
getMetricsView() {
return Lists.newArrayList(new Pair<>(selector, viewBuilder));
}
+ public Attributes getOrBuildAttributes(int requestCode, int responseCode,
+ boolean isLongPolling, String result) {
+ int resultIdx;
+ if (RESULT_SUCCESS.equals(result)) resultIdx = 0;
+ else if (RESULT_ONEWAY.equals(result)) resultIdx = 1;
+ else if (RESULT_WRITE_CHANNEL_FAILED.equals(result)) resultIdx = 2;
+ else if (RESULT_CANCELED.equals(result)) resultIdx = 3;
+ else resultIdx = -1;
+
+ if (resultIdx < 0) {
+ return buildAttributes(requestCode, responseCode, isLongPolling,
result);
+ }
+
+ long key = ((long) requestCode << 19)
Review Comment:
The caching logic looks reasonable, but the packed key is now part of the
correctness boundary for remoting metrics. Could you add focused tests to
verify that `requestCode`, `responseCode`, `isLongPolling`, and `result` all
produce distinct cached Attributes where expected, and that unknown result
values still use the non-cached path? This would reduce the risk of future
metric label mix-ups, especially since the current patch coverage is very low.
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingAbstract.java:
##########
@@ -235,19 +234,14 @@ public static void writeResponse(Channel channel,
RemotingCommand request, @Null
if (response == null) {
return;
}
- final AttributesBuilder attributesBuilder;
- if (remotingMetricsManager != null) {
- attributesBuilder = remotingMetricsManager.newAttributesBuilder();
- attributesBuilder.put(LABEL_IS_LONG_POLLING, request.isSuspended())
- .put(LABEL_REQUEST_CODE,
RemotingHelper.getRequestCodeDesc(request.getCode()))
- .put(LABEL_RESPONSE_CODE,
RemotingHelper.getResponseCodeDesc(response.getCode()));
- } else {
- attributesBuilder = null;
- }
+ final int requestCode = request.getCode();
+ final int responseCode = response.getCode();
+ final boolean isLongPolling = request.isSuspended();
if (request.isOnewayRPC()) {
- if (attributesBuilder != null) {
- attributesBuilder.put(LABEL_RESULT, RESULT_ONEWAY);
-
remotingMetricsManager.getRpcLatency().record(request.getProcessTimer().elapsed(TimeUnit.MILLISECONDS),
attributesBuilder.build());
+ if (remotingMetricsManager != null) {
+ Attributes attrs = remotingMetricsManager.getOrBuildAttributes(
+ requestCode, responseCode, isLongPolling, RESULT_ONEWAY);
+
remotingMetricsManager.getRpcLatency().record(request.getProcessTimer().elapsed(TimeUnit.MILLISECONDS),
attrs);
Review Comment:
The PR description mentions switching to `processTimerElapsedMs`, but this
diff still records latency using
`request.getProcessTimer().elapsed(TimeUnit.MILLISECONDS)` in these paths. If
the timing change is no longer part of this PR, please update the description
so reviewers can evaluate the actual scope.
--
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]
