Re: [PR] [ISSUE #10537] Cache RPC Attributes and use processTimerElapsedMs in NettyRemotingAbstract [rocketmq]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #10539:
URL: https://github.com/apache/rocketmq/pull/10539#discussion_r3781214054


##########
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:
   PR description/issue mention replacing deprecated getProcessTimer().elapsed(...) with processTimerElapsedMs(), but this code still uses getProcessTimer().elapsed(...), and RemotingCommand currently only exposes getProcessTimer() (remoting/.../RemotingCommand.java:638). Either update the PR description or introduce and use the intended processTimerElapsedMs() API.



##########
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)
+            | ((long) (responseCode & 0xFFFF) << 3)
+            | (isLongPolling ? 4L : 0L)
+            | resultIdx;
+        return attributesCache.computeIfAbsent(key,
+            k -> buildAttributes(requestCode, responseCode, isLongPolling, result));

Review Comment:
   The cache key masks responseCode to 16 bits, which can cause collisions (and therefore incorrect cached Attributes) when responseCode is outside 0..65535 (e.g. negative codes like ResponseCode.RPC_TIME_OUT). Consider only using the cache when request/response codes fit in 16 bits and otherwise fall back to direct attribute building, while also masking requestCode to match the intended 35-bit packing.



-- 
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: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org