From b3b8d07bf3e640b03c3e4308f79c9a15e0610514 Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Mon, 5 Jun 2023 12:00:54 -0700 Subject: [PATCH 1/4] Fix NPE in end span --- .../azure-messaging-servicebus/pom.xml | 1 + .../instrumentation/ServiceBusTracer.java | 4 +- .../ServiceBusTracerTests.java | 88 +++++++++++++++++++ 3 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracerTests.java diff --git a/sdk/servicebus/azure-messaging-servicebus/pom.xml b/sdk/servicebus/azure-messaging-servicebus/pom.xml index 72bc413cc5251..1d2574115118a 100644 --- 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/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracer.java b/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracer.java index f7363b6859732..a31314a7d8cf9 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracer.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/main/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/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracerTests.java b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracerTests.java new file mode 100644 index 0000000000000..86c52f1ccb0a9 --- /dev/null +++ b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracerTests.java @@ -0,0 +1,88 @@ +// 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 com.azure.messaging.servicebus.ServiceBusErrorSource; +import com.azure.messaging.servicebus.implementation.DispositionStatus; +import org.apache.qpid.proton.amqp.transport.DeliveryState; +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()); + } + + @Test + public void testSpanEndException() { + Tracer inner = mock(Tracer.class); + when(inner.isEnabled()).thenReturn(true); + + ServiceBusTracer tracer = new ServiceBusTracer(inner, "fqdn", "entityPath"); + + Exception ex = new RuntimeException("foo"); + tracer.endSpan(ex, Context.NONE, null); + + verify(inner, times(1)).end(isNull(), eq(ex), same(Context.NONE)); + } + + @ParameterizedTest + @MethodSource("getAmqpException") + public void testSpanEndAmqpException(AmqpException 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 getAmqpException() { + return Stream.of( + 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())); + } +} From 4af7340c34755192295b49e0c74cf370276145b7 Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Mon, 5 Jun 2023 12:06:08 -0700 Subject: [PATCH 2/4] nits and changelog --- sdk/servicebus/azure-messaging-servicebus/CHANGELOG.md | 2 ++ .../implementation/instrumentation/ServiceBusTracerTests.java | 3 --- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/sdk/servicebus/azure-messaging-servicebus/CHANGELOG.md b/sdk/servicebus/azure-messaging-servicebus/CHANGELOG.md index 3ea21d7078dec..c77874f872244 100644 --- a/sdk/servicebus/azure-messaging-servicebus/CHANGELOG.md +++ b/sdk/servicebus/azure-messaging-servicebus/CHANGELOG.md @@ -7,6 +7,8 @@ ### Breaking Changes ### 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 diff --git a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracerTests.java b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracerTests.java index 86c52f1ccb0a9..039811d2a28a8 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracerTests.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracerTests.java @@ -7,9 +7,6 @@ import com.azure.core.amqp.exception.AmqpException; import com.azure.core.util.Context; import com.azure.core.util.tracing.Tracer; -import com.azure.messaging.servicebus.ServiceBusErrorSource; -import com.azure.messaging.servicebus.implementation.DispositionStatus; -import org.apache.qpid.proton.amqp.transport.DeliveryState; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; From 2c18af1ebb1f454910f1dfdb68af09bce76be104 Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Mon, 5 Jun 2023 12:07:52 -0700 Subject: [PATCH 3/4] up --- .../instrumentation/ServiceBusTracerTests.java | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracerTests.java b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracerTests.java index 039811d2a28a8..bdf827931cb3b 100644 --- a/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracerTests.java +++ b/sdk/servicebus/azure-messaging-servicebus/src/test/java/com/azure/messaging/servicebus/implementation/instrumentation/ServiceBusTracerTests.java @@ -49,22 +49,9 @@ public void testSpanEndNoErrorAndScope() { assertTrue(closed.get()); } - @Test - public void testSpanEndException() { - Tracer inner = mock(Tracer.class); - when(inner.isEnabled()).thenReturn(true); - - ServiceBusTracer tracer = new ServiceBusTracer(inner, "fqdn", "entityPath"); - - Exception ex = new RuntimeException("foo"); - tracer.endSpan(ex, Context.NONE, null); - - verify(inner, times(1)).end(isNull(), eq(ex), same(Context.NONE)); - } - @ParameterizedTest @MethodSource("getAmqpException") - public void testSpanEndAmqpException(AmqpException amqpException, String expectedStatus) { + public void testSpanEndException(Exception amqpException, String expectedStatus) { Tracer inner = mock(Tracer.class); when(inner.isEnabled()).thenReturn(true); @@ -77,6 +64,7 @@ public void testSpanEndAmqpException(AmqpException amqpException, String expecte public static Stream 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()), From cc5614b7117fff4713e22a68031733bc6a15ba7d Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Mon, 5 Jun 2023 14:10:52 -0700 Subject: [PATCH 4/4] eventhubs --- .../azure-messaging-eventhubs/CHANGELOG.md | 3 + .../instrumentation/EventHubsTracer.java | 4 +- .../instrumentation/EventHubsTracerTests.java | 73 +++++++++++++++++++ .../azure-messaging-servicebus/CHANGELOG.md | 1 + 4 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 sdk/eventhubs/azure-messaging-eventhubs/src/test/java/com/azure/messaging/eventhubs/implementation/instrumentation/EventHubsTracerTests.java diff --git a/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md b/sdk/eventhubs/azure-messaging-eventhubs/CHANGELOG.md index 7d749aec3ec8e..05e6a3776b841 100644 --- 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/sdk/eventhubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/implementation/instrumentation/EventHubsTracer.java b/sdk/eventhubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/implementation/instrumentation/EventHubsTracer.java index 37f052651c4f0..9d6fba525e39b 100644 --- a/sdk/eventhubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/implementation/instrumentation/EventHubsTracer.java +++ b/sdk/eventhubs/azure-messaging-eventhubs/src/main/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/sdk/eventhubs/azure-messaging-eventhubs/src/test/java/com/azure/messaging/eventhubs/implementation/instrumentation/EventHubsTracerTests.java b/sdk/eventhubs/azure-messaging-eventhubs/src/test/java/com/azure/messaging/eventhubs/implementation/instrumentation/EventHubsTracerTests.java new file mode 100644 index 0000000000000..c055307dd963b --- /dev/null +++ b/sdk/eventhubs/azure-messaging-eventhubs/src/test/java/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 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 index c77874f872244..48d6c33715bf8 100644 --- a/sdk/servicebus/azure-messaging-servicebus/CHANGELOG.md +++ b/sdk/servicebus/azure-messaging-servicebus/CHANGELOG.md @@ -7,6 +7,7 @@ ### Breaking Changes ### 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))