Copilot commented on code in PR #16371:
URL: https://github.com/apache/dubbo/pull/16371#discussion_r3540902738
##########
dubbo-metrics/dubbo-tracing/src/main/java/org/apache/dubbo/tracing/handler/DubboClientTracingObservationHandler.java:
##########
@@ -30,7 +31,27 @@ public DubboClientTracingObservationHandler(Tracer tracer) {
}
@Override
- public void onScopeOpened(T context) {}
+ public void onScopeOpened(T context) {
+ io.micrometer.tracing.TraceContext traceContext =
+ tracer.currentTraceContext().context();
+ if (traceContext == null) {
+ return;
+ }
+ try {
+ MDC.put("traceId", traceContext.traceId());
+ MDC.put("spanId", traceContext.spanId());
+ } catch (Throwable ignored) {
+ }
Review Comment:
Catching `Throwable` here will also swallow serious JVM `Error`s (e.g.,
`OutOfMemoryError`, `StackOverflowError`) and can make failures much harder to
diagnose. If the goal is only to guard optional SLF4J/MDC linkage and
unexpected MDC runtime failures, prefer a narrower catch such as `LinkageError
| RuntimeException`.
##########
dubbo-metrics/dubbo-tracing/src/main/java/org/apache/dubbo/tracing/handler/DubboClientTracingObservationHandler.java:
##########
@@ -30,7 +31,27 @@ public DubboClientTracingObservationHandler(Tracer tracer) {
}
@Override
- public void onScopeOpened(T context) {}
+ public void onScopeOpened(T context) {
+ io.micrometer.tracing.TraceContext traceContext =
+ tracer.currentTraceContext().context();
+ if (traceContext == null) {
+ return;
+ }
+ try {
+ MDC.put("traceId", traceContext.traceId());
+ MDC.put("spanId", traceContext.spanId());
+ } catch (Throwable ignored) {
+ }
+ }
+
+ @Override
+ public void onScopeClosed(T context) {
+ try {
+ MDC.remove("traceId");
+ MDC.remove("spanId");
+ } catch (Throwable ignored) {
+ }
Review Comment:
`onScopeClosed` unconditionally removes `traceId` / `spanId` from MDC. On
the consumer side this can wipe an existing MDC trace context on the caller
thread (e.g., a web request trace), and it can also break nested observations
(closing an inner scope would clear the outer scope’s IDs). Consider restoring
the previous MDC values instead of always removing them (e.g., capture previous
values in `onScopeOpened` and restore them in `onScopeClosed`, or use
`MDC.putCloseable(...)` and keep the closeables in scope state).
##########
dubbo-metrics/dubbo-tracing/src/test/java/org/apache/dubbo/tracing/handler/DubboServerTracingObservationHandlerTest.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.dubbo.tracing.handler;
+
+import org.apache.dubbo.rpc.RpcContext;
+import org.apache.dubbo.tracing.context.DubboServerContext;
+
+import io.micrometer.tracing.CurrentTraceContext;
+import io.micrometer.tracing.TraceContext;
+import io.micrometer.tracing.Tracer;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+import org.slf4j.MDC;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.mockito.Mockito.when;
+
+class DubboServerTracingObservationHandlerTest {
+
+ private Tracer mockTracer;
+ private CurrentTraceContext mockCurrentTraceContext;
+ private TraceContext mockTraceContext;
+ private DubboServerContext mockContext;
+ private DubboServerTracingObservationHandler<DubboServerContext> handler;
+
+ @BeforeEach
+ void setUp() {
+ // Clear MDC before each test to ensure a clean state
+ MDC.clear();
+
+ mockTracer = Mockito.mock(Tracer.class);
+ mockCurrentTraceContext = Mockito.mock(CurrentTraceContext.class);
+ mockTraceContext = Mockito.mock(TraceContext.class);
+ mockContext = Mockito.mock(DubboServerContext.class);
+
+
when(mockTracer.currentTraceContext()).thenReturn(mockCurrentTraceContext);
+ when(mockCurrentTraceContext.context()).thenReturn(mockTraceContext);
+ when(mockTraceContext.traceId()).thenReturn("mock-trace-id-123");
+ when(mockTraceContext.spanId()).thenReturn("mock-span-id-456");
+
+ handler = new DubboServerTracingObservationHandler<>(mockTracer);
+ }
+
+ @AfterEach
+ void tearDown() {
+ MDC.clear();
+ RpcContext.removeServerContext();
+ }
+
+ @Test
+ void proveWorking_WhenScopeOpens_MdcShouldHaveIds() {
+ // 1. Verify MDC is currently empty
+ assertNull(MDC.get("traceId"));
+ assertNull(MDC.get("spanId"));
+
+ handler.onScopeOpened(mockContext);
+
+ assertEquals("mock-trace-id-123", MDC.get("traceId"), "Trace ID should
be mapped to MDC!");
+ assertEquals("mock-span-id-456", MDC.get("spanId"), "Span ID should be
mapped to MDC!");
+ }
+
+ @Test
+ void proveFailurePrevention_WhenScopeCloses_MdcShouldBeCleaned() {
+ handler.onScopeOpened(mockContext);
+ assertEquals("mock-trace-id-123", MDC.get("traceId"));
+
+ handler.onScopeClosed(mockContext);
+
+ // PROOF THE LEAK IS PREVENTED: MDC must be completely empty now.
+ assertNull(MDC.get("traceId"), "Trace ID leaked! It was not removed
when scope closed.");
+ assertNull(MDC.get("spanId"), "Span ID leaked! It was not removed when
scope closed.");
+ }
Review Comment:
This test currently asserts MDC is empty after `onScopeClosed()`. That’s
only correct when there was no trace context already present on the thread; for
nested scopes or when something upstream already populated MDC,
`onScopeClosed()` should restore the previous values instead of clearing them.
Consider extending the test to cover the “pre-existing MDC is preserved”
scenario.
##########
dubbo-metrics/dubbo-tracing/src/main/java/org/apache/dubbo/tracing/handler/DubboServerTracingObservationHandler.java:
##########
@@ -41,6 +42,20 @@ public void onScopeOpened(T context) {
return;
}
RpcContext.getServerContext().setAttachment(DEFAULT_TRACE_ID_KEY,
traceContext.traceId());
+ try {
+ MDC.put(DEFAULT_TRACE_ID_KEY, traceContext.traceId());
+ MDC.put("spanId", traceContext.spanId());
+ } catch (Throwable ignored) {
+ }
Review Comment:
Catching `Throwable` here will also swallow serious JVM `Error`s and can
hide failures. If the intent is to guard against missing SLF4J/MDC at runtime
and unexpected MDC failures, consider narrowing this to `LinkageError |
RuntimeException`.
##########
dubbo-metrics/dubbo-tracing/src/main/java/org/apache/dubbo/tracing/handler/DubboServerTracingObservationHandler.java:
##########
@@ -41,6 +42,20 @@ public void onScopeOpened(T context) {
return;
}
RpcContext.getServerContext().setAttachment(DEFAULT_TRACE_ID_KEY,
traceContext.traceId());
+ try {
+ MDC.put(DEFAULT_TRACE_ID_KEY, traceContext.traceId());
+ MDC.put("spanId", traceContext.spanId());
+ } catch (Throwable ignored) {
+ }
+ }
+
+ @Override
+ public void onScopeClosed(T context) {
+ try {
+ MDC.remove(DEFAULT_TRACE_ID_KEY);
+ MDC.remove("spanId");
+ } catch (Throwable ignored) {
+ }
Review Comment:
`onScopeClosed` unconditionally removes `traceId` / `spanId` from MDC. This
can clear any pre-existing MDC trace context on the thread and can break nested
observations (closing an inner scope would clear the outer scope’s IDs).
Consider restoring the previous MDC values instead of always removing them
(e.g., capture previous values in `onScopeOpened` and restore them in
`onScopeClosed`, or use `MDC.putCloseable(...)` and keep the closeables in
scope state).
##########
dubbo-metrics/dubbo-tracing/src/test/java/org/apache/dubbo/tracing/handler/DubboClientTracingObservationHandlerTest.java:
##########
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.dubbo.tracing.handler;
+
+import org.apache.dubbo.tracing.context.DubboClientContext;
+
+import io.micrometer.tracing.CurrentTraceContext;
+import io.micrometer.tracing.TraceContext;
+import io.micrometer.tracing.Tracer;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+import org.slf4j.MDC;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.mockito.Mockito.when;
+
+class DubboClientTracingObservationHandlerTest {
+
+ private Tracer mockTracer;
+ private CurrentTraceContext mockCurrentTraceContext;
+ private TraceContext mockTraceContext;
+ private DubboClientContext mockContext;
+ private DubboClientTracingObservationHandler<DubboClientContext> handler;
+
+ @BeforeEach
+ void setUp() {
+ // Clear MDC before each test to ensure a clean state
+ MDC.clear();
+
+ mockTracer = Mockito.mock(Tracer.class);
+ mockCurrentTraceContext = Mockito.mock(CurrentTraceContext.class);
+ mockTraceContext = Mockito.mock(TraceContext.class);
+ mockContext = Mockito.mock(DubboClientContext.class);
+
+
when(mockTracer.currentTraceContext()).thenReturn(mockCurrentTraceContext);
+ when(mockCurrentTraceContext.context()).thenReturn(mockTraceContext);
+ when(mockTraceContext.traceId()).thenReturn("mock-trace-id-123");
+ when(mockTraceContext.spanId()).thenReturn("mock-span-id-456");
+
+ handler = new DubboClientTracingObservationHandler<>(mockTracer);
+ }
+
+ @AfterEach
+ void tearDown() {
+ MDC.clear();
+ }
+
+ @Test
+ void proveWorking_WhenScopeOpens_MdcShouldHaveIds() {
+ // 1. Verify MDC is currently empty
+ assertNull(MDC.get("traceId"));
+ assertNull(MDC.get("spanId"));
+
+ handler.onScopeOpened(mockContext);
+
+ assertEquals("mock-trace-id-123", MDC.get("traceId"), "Trace ID should
be mapped to MDC!");
+ assertEquals("mock-span-id-456", MDC.get("spanId"), "Span ID should be
mapped to MDC!");
+ }
+
+ @Test
+ void proveFailurePrevention_WhenScopeCloses_MdcShouldBeCleaned() {
+ handler.onScopeOpened(mockContext);
+ assertEquals("mock-trace-id-123", MDC.get("traceId"));
+
+ handler.onScopeClosed(mockContext);
+
+ // PROOF THE LEAK IS PREVENTED: MDC must be completely empty now.
+ assertNull(MDC.get("traceId"), "Trace ID leaked! It was not removed
when scope closed.");
+ assertNull(MDC.get("spanId"), "Span ID leaked! It was not removed when
scope closed.");
+ }
Review Comment:
This test currently asserts MDC is empty after `onScopeClosed()`. That’s
only correct when there was no trace context already present on the thread; on
the consumer side (and in nested scopes) the correct behavior is usually to
restore the previous MDC values rather than clearing them. Adding a test that
starts with existing `traceId`/`spanId`, then asserts they are restored after
`onScopeClosed()`, would catch regressions where Dubbo clears the caller’s MDC.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
