Fix NPE in end span (#35299)

* Fix NPE in end span
Azure · Jun 5, 2023 · 7395f50 · 7395f50
1 parent b75df37
commit 7395f50
Show file tree

Hide file tree

Showing 7 changed files with 159 additions and 2 deletions.
diff --git a/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md b/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md
@@ -12,6 +12,9 @@
 ### Bugs Fixed
 
 - Fixed exception when attempting to populate trace context on received `EventData`. ([#33594](https://github.com/Azure/azure-sdk-for-java/issues/33594))
+- Fixed `NullPointerException` when ending span when `AmqpException` is thrown, but its `AmqpErrorCondition` is `null`.
+  ([#35299](https://github.com/Azure/azure-sdk-for-java/issues/35299))
+
 
 ### Other Changes
 

diff --git a/...in/java/com/azure/messaging/eventhubs/implementation/instrumentation/EventHubsTracer.java b/...in/java/com/azure/messaging/eventhubs/implementation/instrumentation/EventHubsTracer.java
@@ -83,7 +83,9 @@ public void endSpan(Throwable throwable, Context span, AutoCloseable scope) {
             String errorCondition = null;
             if (throwable instanceof AmqpException) {
                 AmqpException exception = (AmqpException) throwable;
-                errorCondition = exception.getErrorCondition().getErrorCondition();
+                if (exception.getErrorCondition() != null) {
+                    errorCondition = exception.getErrorCondition().getErrorCondition();
+                }
             }
 
             try {

diff --git a/...va/com/azure/messaging/eventhubs/implementation/instrumentation/EventHubsTracerTests.java b/...va/com/azure/messaging/eventhubs/implementation/instrumentation/EventHubsTracerTests.java
@@ -0,0 +1,73 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+package com.azure.messaging.eventhubs.implementation.instrumentation;
+
+import com.azure.core.amqp.exception.AmqpErrorCondition;
+import com.azure.core.amqp.exception.AmqpException;
+import com.azure.core.util.Context;
+import com.azure.core.util.tracing.Tracer;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Stream;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.ArgumentMatchers.isNull;
+import static org.mockito.ArgumentMatchers.same;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+public class EventHubsTracerTests {
+    @Test
+    public void testSpanEndNoError() {
+        Tracer inner = mock(Tracer.class);
+        when(inner.isEnabled()).thenReturn(true);
+
+        EventHubsTracer tracer = new EventHubsTracer(inner, "fqdn", "entityPath");
+        tracer.endSpan(null, Context.NONE, null);
+
+        verify(inner, times(1)).end(isNull(), isNull(), same(Context.NONE));
+    }
+
+    @Test
+    public void testSpanEndNoErrorAndScope() {
+        Tracer inner = mock(Tracer.class);
+        when(inner.isEnabled()).thenReturn(true);
+
+        EventHubsTracer tracer = new EventHubsTracer(inner, "fqdn", "entityPath");
+        AtomicBoolean closed = new AtomicBoolean();
+        tracer.endSpan(null, Context.NONE, () -> closed.set(true));
+
+        verify(inner, times(1)).end(isNull(), isNull(), same(Context.NONE));
+        assertTrue(closed.get());
+    }
+
+    @ParameterizedTest
+    @MethodSource("getAmqpException")
+    public void testSpanEndException(Exception amqpException, String expectedStatus) {
+        Tracer inner = mock(Tracer.class);
+        when(inner.isEnabled()).thenReturn(true);
+
+        EventHubsTracer tracer = new EventHubsTracer(inner, "fqdn", "entityPath");
+
+        tracer.endSpan(amqpException, Context.NONE, null);
+
+        verify(inner, times(1)).end(eq(expectedStatus), eq(amqpException), same(Context.NONE));
+    }
+
+    public static Stream<Arguments> getAmqpException() {
+        return Stream.of(
+            Arguments.of(new RuntimeException("foo"), null),
+            Arguments.of(new AmqpException(false, "foo", null, null), null),
+            Arguments.of(new AmqpException(false, AmqpErrorCondition.NOT_FOUND, "foo", null), AmqpErrorCondition.NOT_FOUND.getErrorCondition()),
+            Arguments.of(new AmqpException(false, AmqpErrorCondition.TIMEOUT_ERROR, "", null), AmqpErrorCondition.TIMEOUT_ERROR.getErrorCondition()),
+            Arguments.of(new AmqpException(false, AmqpErrorCondition.SERVER_BUSY_ERROR, null, new RuntimeException("foo"), null), AmqpErrorCondition.SERVER_BUSY_ERROR.getErrorCondition()));
+    }
+}
diff --git a/sdk/servicebus/azure-messaging-servicebus/CHANGELOG.md b/sdk/servicebus/azure-messaging-servicebus/CHANGELOG.md
@@ -8,6 +8,9 @@
 
 ### Bugs Fixed
 
+- Fixed `NullPointerException` when ending span when `AmqpException` is thrown, but its `AmqpErrorCondition` is `null`.
+  ([#35299](https://github.com/Azure/azure-sdk-for-java/issues/35299))
+
 ### Other Changes
 
 ## 7.14.0 (2023-05-15)

diff --git a/sdk/servicebus/azure-messaging-servicebus/pom.xml b/sdk/servicebus/azure-messaging-servicebus/pom.xml
@@ -43,6 +43,7 @@
       --add-opens com.azure.messaging.servicebus/com.azure.messaging.servicebus.administration.models=ALL-UNNAMED
 
       --add-exports com.azure.core/com.azure.core.implementation.util=ALL-UNNAMED
+      --add-exports com.azure.core/com.azure.core.implementation.instrumentation=ALL-UNNAMED
       --add-opens com.azure.core/com.azure.core.implementation.util=ALL-UNNAMED
       --add-reads com.azure.messaging.servicebus=com.azure.http.netty
       --add-reads com.azure.messaging.servicebus=com.azure.core.tracing.opentelemetry

diff --git a/.../java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracer.java b/.../java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracer.java
@@ -127,7 +127,9 @@ public void endSpan(Throwable throwable, Context span, AutoCloseable scope) {
             String errorCondition = null;
             if (throwable instanceof AmqpException) {
                 AmqpException exception = (AmqpException) throwable;
-                errorCondition = exception.getErrorCondition().getErrorCondition();
+                if (exception.getErrorCondition() != null) {
+                    errorCondition = exception.getErrorCondition().getErrorCondition();
+                }
             }
 
             try {

diff --git a/.../com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracerTests.java b/.../com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracerTests.java
@@ -0,0 +1,73 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+package com.azure.messaging.servicebus.implementation.instrumentation;
+
+import com.azure.core.amqp.exception.AmqpErrorCondition;
+import com.azure.core.amqp.exception.AmqpException;
+import com.azure.core.util.Context;
+import com.azure.core.util.tracing.Tracer;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Stream;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.ArgumentMatchers.isNull;
+import static org.mockito.ArgumentMatchers.same;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+public class ServiceBusTracerTests {
+    @Test
+    public void testSpanEndNoError() {
+        Tracer inner = mock(Tracer.class);
+        when(inner.isEnabled()).thenReturn(true);
+
+        ServiceBusTracer tracer = new ServiceBusTracer(inner, "fqdn", "entityPath");
+        tracer.endSpan(null, Context.NONE, null);
+
+        verify(inner, times(1)).end(isNull(), isNull(), same(Context.NONE));
+    }
+
+    @Test
+    public void testSpanEndNoErrorAndScope() {
+        Tracer inner = mock(Tracer.class);
+        when(inner.isEnabled()).thenReturn(true);
+
+        ServiceBusTracer tracer = new ServiceBusTracer(inner, "fqdn", "entityPath");
+        AtomicBoolean closed = new AtomicBoolean();
+        tracer.endSpan(null, Context.NONE, () -> closed.set(true));
+
+        verify(inner, times(1)).end(isNull(), isNull(), same(Context.NONE));
+        assertTrue(closed.get());
+    }
+
+    @ParameterizedTest
+    @MethodSource("getAmqpException")
+    public void testSpanEndException(Exception amqpException, String expectedStatus) {
+        Tracer inner = mock(Tracer.class);
+        when(inner.isEnabled()).thenReturn(true);
+
+        ServiceBusTracer tracer = new ServiceBusTracer(inner, "fqdn", "entityPath");
+
+        tracer.endSpan(amqpException, Context.NONE, null);
+
+        verify(inner, times(1)).end(eq(expectedStatus), eq(amqpException), same(Context.NONE));
+    }
+
+    public static Stream<Arguments> getAmqpException() {
+        return Stream.of(
+            Arguments.of(new RuntimeException("foo"), null),
+            Arguments.of(new AmqpException(false, "foo", null, null), null),
+            Arguments.of(new AmqpException(false, AmqpErrorCondition.NOT_FOUND, "foo", null), AmqpErrorCondition.NOT_FOUND.getErrorCondition()),
+            Arguments.of(new AmqpException(false, AmqpErrorCondition.TIMEOUT_ERROR, "", null), AmqpErrorCondition.TIMEOUT_ERROR.getErrorCondition()),
+            Arguments.of(new AmqpException(false, AmqpErrorCondition.SERVER_BUSY_ERROR, null, new RuntimeException("foo"), null), AmqpErrorCondition.SERVER_BUSY_ERROR.getErrorCondition()));
+    }
+}