ACCUMULO-2390 InvocationTargetEx in TraceProxy Handle InvocationTargetException specifically in TraceProxy, instead of letting it get propogated up the call stack. In some cases this is very bad as it turned into an UndeclaredThrowableException and made debugging more difficult than necessary.
Added unit test to verify behaviour. Project: http://git-wip-us.apache.org/repos/asf/accumulo/repo Commit: http://git-wip-us.apache.org/repos/asf/accumulo/commit/28294266 Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/28294266 Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/28294266 Branch: refs/heads/1.6.0-SNAPSHOT Commit: 2829426618b6e7d1487a4c88dd7b09186b9898d5 Parents: 7059c76 Author: Mike Drob <md...@cloudera.com> Authored: Fri Feb 21 13:42:01 2014 -0500 Committer: Mike Drob <md...@cloudera.com> Committed: Mon Feb 24 08:39:03 2014 -0500 ---------------------------------------------------------------------- .../cloudtrace/instrument/TraceProxy.java | 37 ++++++++++++------ .../cloudtrace/instrument/TracerTest.java | 41 ++++++++++++++++++-- 2 files changed, 63 insertions(+), 15 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/accumulo/blob/28294266/src/trace/src/main/java/org/apache/accumulo/cloudtrace/instrument/TraceProxy.java ---------------------------------------------------------------------- diff --git a/src/trace/src/main/java/org/apache/accumulo/cloudtrace/instrument/TraceProxy.java b/src/trace/src/main/java/org/apache/accumulo/cloudtrace/instrument/TraceProxy.java index 67c4463..6b71361 100644 --- a/src/trace/src/main/java/org/apache/accumulo/cloudtrace/instrument/TraceProxy.java +++ b/src/trace/src/main/java/org/apache/accumulo/cloudtrace/instrument/TraceProxy.java @@ -17,43 +17,56 @@ package org.apache.accumulo.cloudtrace.instrument; import java.lang.reflect.InvocationHandler; +import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.lang.reflect.Proxy; +import org.apache.log4j.Logger; + public class TraceProxy { - // private static final Logger log = Logger.getLogger(TraceProxy.class); - + private static final Logger log = Logger.getLogger(TraceProxy.class); + static final Sampler ALWAYS = new Sampler() { @Override public boolean next() { return true; } }; - + public static <T> T trace(T instance) { return trace(instance, ALWAYS); } - + @SuppressWarnings("unchecked") public static <T> T trace(final T instance, final Sampler sampler) { InvocationHandler handler = new InvocationHandler() { @Override public Object invoke(Object obj, Method method, Object[] args) throws Throwable { - if (!sampler.next()) { - return method.invoke(instance, args); + Span span = null; + if (sampler.next()) { + span = Trace.on(method.getName()); } - Span span = Trace.on(method.getName()); try { return method.invoke(instance, args); - } catch (Throwable ex) { - ex.printStackTrace(); - throw ex; + // Can throw RuntimeException, Error, or any checked exceptions of the method. + } catch (InvocationTargetException ite) { + Throwable cause = ite.getCause(); + if (cause == null) { + // This should never happen, but account for it anyway + log.error("Invocation exception during trace with null cause: ", ite); + throw new RuntimeException(ite); + } + throw cause; + } catch (IllegalAccessException e) { + throw new RuntimeException(e); } finally { - span.stop(); + if (span != null) { + span.stop(); + } } } }; return (T) Proxy.newProxyInstance(instance.getClass().getClassLoader(), instance.getClass().getInterfaces(), handler); } - + } http://git-wip-us.apache.org/repos/asf/accumulo/blob/28294266/src/trace/src/test/java/org/apache/accumulo/cloudtrace/instrument/TracerTest.java ---------------------------------------------------------------------- diff --git a/src/trace/src/test/java/org/apache/accumulo/cloudtrace/instrument/TracerTest.java b/src/trace/src/test/java/org/apache/accumulo/cloudtrace/instrument/TracerTest.java index 30b62f9..20fc768 100644 --- a/src/trace/src/test/java/org/apache/accumulo/cloudtrace/instrument/TracerTest.java +++ b/src/trace/src/test/java/org/apache/accumulo/cloudtrace/instrument/TracerTest.java @@ -21,16 +21,15 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import java.io.IOException; import java.net.ServerSocket; import java.net.Socket; import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.Callable; -import org.apache.accumulo.cloudtrace.instrument.Span; -import org.apache.accumulo.cloudtrace.instrument.Trace; -import org.apache.accumulo.cloudtrace.instrument.Tracer; import org.apache.accumulo.cloudtrace.instrument.receivers.SpanReceiver; import org.apache.accumulo.cloudtrace.instrument.thrift.TraceWrap; import org.apache.accumulo.cloudtrace.thrift.TInfo; @@ -42,6 +41,7 @@ import org.apache.thrift.server.TThreadPoolServer; import org.apache.thrift.transport.TServerSocket; import org.apache.thrift.transport.TSocket; import org.apache.thrift.transport.TTransport; +import org.junit.Before; import org.junit.Test; @@ -179,4 +179,39 @@ public class TracerTest { tserver.stop(); t.join(100); } + + Callable<Object> callable; + + @Before + public void setup() { + callable = new Callable<Object>() { + @Override + public Object call() throws IOException { + throw new IOException(); + } + }; + } + + /** + * Verify that exceptions propagate up through the trace wrapping with sampling enabled, instead of seeing the reflexive exceptions. + */ + @Test(expected = IOException.class) + public void testTracedException() throws Exception { + TraceProxy.trace(callable).call(); + } + + /** + * Verify that exceptions propagate up through the trace wrapping with sampling disabled, instead of seeing the reflexive exceptions. + */ + @Test(expected = IOException.class) + public void testUntracedException() throws Exception { + Sampler never = new Sampler() { + @Override + public boolean next() { + return false; + } + }; + + TraceProxy.trace(callable, never).call(); + } }
