From ef3e247c7dd6935c7197fd2b7ea22fd03b98b810 Mon Sep 17 00:00:00 2001 From: Scott Chamberlain Date: Wed, 1 Apr 2020 17:31:37 -0700 Subject: [PATCH 1/3] Fixed implementation of TracingStatement when no segment is present. Solves issues brought up in #136 --- .../amazonaws/xray/sql/TracingStatement.java | 20 ++++++- .../xray/sql/TracingStatementTest.java | 58 ++++++++++++++++++- 2 files changed, 75 insertions(+), 3 deletions(-) diff --git a/aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/TracingStatement.java b/aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/TracingStatement.java index 09c1bffe..0467a6cc 100644 --- a/aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/TracingStatement.java +++ b/aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/TracingStatement.java @@ -97,8 +97,24 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl Subsegment subsegment = createSubsegment(); if (subsegment == null) { - // don't trace if failed to create subsegment - return method.invoke(delegate, args); + try { + // don't trace if failed to create subsegment + return method.invoke(delegate, args); + } catch (Throwable t) { + if (t instanceof InvocationTargetException) { + // the reflection may wrap the actual error with an InvocationTargetException. + // we want to use the root cause to make the instrumentation seamless + InvocationTargetException ite = (InvocationTargetException) t; + if (ite.getTargetException() != null) { + throw ite.getTargetException(); + } + if (ite.getCause() != null) { + throw ite.getCause(); + } + } + + throw t; + } } logger.debug("Invoking statement execution with X-Ray tracing."); diff --git a/aws-xray-recorder-sdk-sql/src/test/java/com/amazonaws/xray/sql/TracingStatementTest.java b/aws-xray-recorder-sdk-sql/src/test/java/com/amazonaws/xray/sql/TracingStatementTest.java index 55e52784..24da407b 100644 --- a/aws-xray-recorder-sdk-sql/src/test/java/com/amazonaws/xray/sql/TracingStatementTest.java +++ b/aws-xray-recorder-sdk-sql/src/test/java/com/amazonaws/xray/sql/TracingStatementTest.java @@ -10,6 +10,7 @@ import java.sql.DatabaseMetaData; import java.sql.PreparedStatement; import java.sql.ResultSet; +import java.sql.SQLException; import java.sql.Statement; import java.util.HashMap; import java.util.Map; @@ -25,6 +26,8 @@ import com.amazonaws.xray.AWSXRay; import com.amazonaws.xray.entities.Namespace; import com.amazonaws.xray.entities.Subsegment; +import com.amazonaws.xray.strategy.ContextMissingStrategy; +import com.amazonaws.xray.strategy.IgnoreErrorContextMissingStrategy; @RunWith(MockitoJUnitRunner.class) public class TracingStatementTest { @@ -169,7 +172,7 @@ public void testCalledStatementExecuteUpdate() throws Exception { } @Test - public void testCaptureException() throws Exception { + public void testCaptureRuntimeException() throws Exception { Throwable exception = new RuntimeException("foo"); when(delegate.execute(SQL)).thenThrow(exception); try { @@ -183,6 +186,59 @@ public void testCaptureException() throws Exception { } } + @Test + public void testCaptureSqlException() throws Exception { + Throwable exception = new SQLException("foo"); + when(delegate.execute(SQL)).thenThrow(exception); + try { + statement.execute(SQL); + fail("Expected exception is not thrown"); + } catch (Throwable th) { + assertEquals(exception, th); + } finally { + assertEquals(exception, AWSXRay.getCurrentSegment().getSubsegments().get(0).getCause().getExceptions().get(0).getThrowable()); + assertSubsegment(); + } + } + + + + @Test + public void testCaptureRuntimeExceptionWithoutSegment() throws Exception { + ContextMissingStrategy oldStrategy = AWSXRay.getGlobalRecorder().getContextMissingStrategy(); + AWSXRay.getGlobalRecorder().setContextMissingStrategy(new IgnoreErrorContextMissingStrategy()); + try { + Throwable exception = new RuntimeException("foo"); + when(delegate.execute(SQL)).thenThrow(exception); + try { + statement.execute(SQL); + fail("Expected exception is not thrown"); + } catch (Throwable th) { + assertEquals(exception, th); + } + } finally { + AWSXRay.getGlobalRecorder().setContextMissingStrategy(oldStrategy); + } + } + + @Test + public void testCaptureSqlExceptionWithoutSegment() throws Exception { + ContextMissingStrategy oldStrategy = AWSXRay.getGlobalRecorder().getContextMissingStrategy(); + AWSXRay.getGlobalRecorder().setContextMissingStrategy(new IgnoreErrorContextMissingStrategy()); + try { + Throwable exception = new SQLException("foo"); + when(delegate.execute(SQL)).thenThrow(exception); + try { + statement.execute(SQL); + fail("Expected exception is not thrown"); + } catch (Throwable th) { + assertEquals(exception, th); + } + } finally { + AWSXRay.getGlobalRecorder().setContextMissingStrategy(oldStrategy); + } + } + @Test public void testNonTracedMethod() throws Exception { statement.close(); From 159b117095c919f247393ad0b63592eadba4c10e Mon Sep 17 00:00:00 2001 From: Scott Chamberlain Date: Thu, 2 Apr 2020 10:30:06 -0700 Subject: [PATCH 2/3] Refactored to remove duplicate code. * Removed the extra calls to `method.invoke(delegate, args` so all calls happen within the try/catch block. * Added a message at the end of the logging statement if the tracing was enabled or not during the execution --- .../amazonaws/xray/sql/TracingStatement.java | 51 ++++++------------- 1 file changed, 16 insertions(+), 35 deletions(-) diff --git a/aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/TracingStatement.java b/aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/TracingStatement.java index 0467a6cc..6069c0cf 100644 --- a/aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/TracingStatement.java +++ b/aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/TracingStatement.java @@ -90,58 +90,39 @@ private static class TracingStatementHandler implements InvocationHandler { } public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { - if (!isExecution(method)) { - // don't trace non execution methods - return method.invoke(delegate, args); - } - - Subsegment subsegment = createSubsegment(); - if (subsegment == null) { - try { - // don't trace if failed to create subsegment - return method.invoke(delegate, args); - } catch (Throwable t) { - if (t instanceof InvocationTargetException) { - // the reflection may wrap the actual error with an InvocationTargetException. - // we want to use the root cause to make the instrumentation seamless - InvocationTargetException ite = (InvocationTargetException) t; - if (ite.getTargetException() != null) { - throw ite.getTargetException(); - } - if (ite.getCause() != null) { - throw ite.getCause(); - } - } + Subsegment subsegment = null; - throw t; - } + if (isExecution(method)) { + // only trace on execution methods + subsegment = createSubsegment(); } - logger.debug("Invoking statement execution with X-Ray tracing."); + logger.debug(String.format("Invoking statement execution with X-Ray tracing. Tracing active: %s", subsegment != null)); try { // execute the query "wrapped" in a XRay Subsegment return method.invoke(delegate, args); } catch (Throwable t) { + Throwable rootThrowable = t; if (t instanceof InvocationTargetException) { // the reflection may wrap the actual error with an InvocationTargetException. // we want to use the root cause to make the instrumentation seamless InvocationTargetException ite = (InvocationTargetException) t; if (ite.getTargetException() != null) { - subsegment.addException(ite.getTargetException()); - throw ite.getTargetException(); + rootThrowable = ite.getTargetException(); } - if (ite.getCause() != null) { - subsegment.addException(ite.getCause()); - throw ite.getCause(); + else if (ite.getCause() != null) { + rootThrowable = ite.getCause(); } - subsegment.addException(ite); - throw ite; } - subsegment.addException(t); - throw t; + if (subsegment != null) { + subsegment.addException(rootThrowable); + } + throw rootThrowable; } finally { - AWSXRay.endSubsegment(); + if (isExecution(method)) { + AWSXRay.endSubsegment(); + } } } From dbb8f5bb9e1c206cb8a2b0e66a0a720ced70fb37 Mon Sep 17 00:00:00 2001 From: Scott Chamberlain Date: Mon, 6 Apr 2020 12:26:59 -0700 Subject: [PATCH 3/3] Added fast fail check in the finally block. --- .../src/main/java/com/amazonaws/xray/sql/TracingStatement.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/TracingStatement.java b/aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/TracingStatement.java index 6069c0cf..fefdedae 100644 --- a/aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/TracingStatement.java +++ b/aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/TracingStatement.java @@ -120,7 +120,7 @@ else if (ite.getCause() != null) { } throw rootThrowable; } finally { - if (isExecution(method)) { + if (subsegment != null && isExecution(method)) { AWSXRay.endSubsegment(); } }