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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
