From 2b0e12d27189e758a328814cfeca9f27e14536a8 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 9 Sep 2019 16:28:52 -0700 Subject: [PATCH] Fix #393: Loosen InvocationEventHanndler context bound to object --- .../tritium/event/InvocationEventHandler.java | 6 +- .../event/AbstractInvocationEventHandler.java | 7 +- .../CompositeInvocationEventHandler.java | 78 +++++------- .../CompositeInvocationEventHandlerTest.java | 114 +++++++++--------- .../proxy/ByteBuddyInstrumentation.java | 8 +- .../proxy/ByteBuddyInstrumentationAdvice.java | 9 +- .../tritium/proxy/Instrumentation.java | 20 +-- .../tritium/proxy/InstrumentationProxy.java | 9 +- .../tritium/proxy/InvocationEventProxy.java | 42 ++----- .../proxy/InvocationEventProxyTest.java | 43 +++---- ...ricsServiceInvocationEventHandlerTest.java | 3 +- 11 files changed, 150 insertions(+), 189 deletions(-) diff --git a/tritium-api/src/main/java/com/palantir/tritium/event/InvocationEventHandler.java b/tritium-api/src/main/java/com/palantir/tritium/event/InvocationEventHandler.java index 2f80f14a5..6b7ad3acf 100644 --- a/tritium-api/src/main/java/com/palantir/tritium/event/InvocationEventHandler.java +++ b/tritium-api/src/main/java/com/palantir/tritium/event/InvocationEventHandler.java @@ -26,7 +26,7 @@ * @see java.lang.reflect.InvocationHandler * @param invocation context */ -public interface InvocationEventHandler { +public interface InvocationEventHandler { /** * Returns true if this event handler instance is enabled, otherwise false. @@ -57,7 +57,7 @@ public interface InvocationEventHandler { * @param context the current invocation context. * @param result the return value from invocation, or null if {@link Void}. */ - void onSuccess(@Nullable InvocationContext context, @Nullable Object result); + void onSuccess(@Nullable C context, @Nullable Object result); /** * Invoked when an invocation fails. @@ -69,6 +69,6 @@ public interface InvocationEventHandler { * @param context the current invocation context. * @param cause the throwable which caused the failure. */ - void onFailure(@Nullable InvocationContext context, @Nonnull Throwable cause); + void onFailure(@Nullable C context, @Nonnull Throwable cause); } diff --git a/tritium-core/src/main/java/com/palantir/tritium/event/AbstractInvocationEventHandler.java b/tritium-core/src/main/java/com/palantir/tritium/event/AbstractInvocationEventHandler.java index 5ef60fd6d..cc7823687 100644 --- a/tritium-core/src/main/java/com/palantir/tritium/event/AbstractInvocationEventHandler.java +++ b/tritium-core/src/main/java/com/palantir/tritium/event/AbstractInvocationEventHandler.java @@ -28,8 +28,7 @@ * * @param invocation context */ -public abstract class AbstractInvocationEventHandler - implements InvocationEventHandler { +public abstract class AbstractInvocationEventHandler implements InvocationEventHandler { private static final Logger logger = LoggerFactory.getLogger(AbstractInvocationEventHandler.class); @@ -78,7 +77,7 @@ public final boolean isEnabled() { * * @param context invocation context */ - protected final void debugIfNullContext(@Nullable InvocationContext context) { + protected final void debugIfNullContext(@Nullable C context) { if (context == null) { logger.debug( "{} encountered null metric context, likely due to exception in preInvocation", @@ -98,7 +97,7 @@ private static SafeArg safeClassName(@Nullable Object obj) { * @return false if "instrument.fully.qualified.class.Name" is set to "false", otherwise true */ protected static com.palantir.tritium.api.functions.BooleanSupplier getSystemPropertySupplier( - Class> clazz) { + Class> clazz) { checkNotNull(clazz, "clazz"); return InstrumentationProperties.getSystemPropertySupplier(clazz.getName()); } diff --git a/tritium-core/src/main/java/com/palantir/tritium/event/CompositeInvocationEventHandler.java b/tritium-core/src/main/java/com/palantir/tritium/event/CompositeInvocationEventHandler.java index 27175a88e..b05e1c4a7 100644 --- a/tritium-core/src/main/java/com/palantir/tritium/event/CompositeInvocationEventHandler.java +++ b/tritium-core/src/main/java/com/palantir/tritium/event/CompositeInvocationEventHandler.java @@ -28,22 +28,20 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -public final class CompositeInvocationEventHandler extends AbstractInvocationEventHandler { +public final class CompositeInvocationEventHandler extends AbstractInvocationEventHandler { private static final Logger logger = LoggerFactory.getLogger(CompositeInvocationEventHandler.class); - private final InvocationEventHandler[] handlers; + private final InvocationEventHandler[] handlers; - @SuppressWarnings("unchecked") - private CompositeInvocationEventHandler(List> handlers) { + private CompositeInvocationEventHandler(List> handlers) { this.handlers = checkNotNull(handlers, "handlers").toArray(new InvocationEventHandler[0]); - for (InvocationEventHandler handler : handlers) { + for (InvocationEventHandler handler : handlers) { checkNotNull(handler, "Null handlers are not allowed"); } } - public static InvocationEventHandler of( - List> handlers) { + public static InvocationEventHandler of(List> handlers) { if (handlers.isEmpty()) { return NoOpInvocationEventHandler.INSTANCE; } else if (handlers.size() == 1) { @@ -54,8 +52,8 @@ public static InvocationEventHandler of( } @Nullable - private InvocationEventHandler tryGetEnabledHandler(int index) { - InvocationEventHandler handler = handlers[index]; + private InvocationEventHandler tryGetEnabledHandler(int index) { + InvocationEventHandler handler = handlers[index]; if (handler.isEnabled()) { return handler; } @@ -63,47 +61,49 @@ private InvocationEventHandler tryGetEnabledHandler(int index } @Override - public InvocationContext preInvocation(@Nonnull Object instance, @Nonnull Method method, @Nonnull Object[] args) { - InvocationContext[] contexts = new InvocationContext[handlers.length]; + public Object[] preInvocation(@Nonnull Object instance, @Nonnull Method method, @Nonnull Object[] args) { + Object[] contexts = new Object[handlers.length]; for (int i = 0; i < handlers.length; i++) { contexts[i] = handlePreInvocation(tryGetEnabledHandler(i), instance, method, args); } - return new CompositeInvocationContext(instance, method, args, contexts); + return contexts; } @Override - public void onSuccess(@Nullable InvocationContext context, @Nullable Object result) { + public void onSuccess(@Nullable Object[] context, @Nullable Object result) { debugIfNullContext(context); if (context != null) { - success(((CompositeInvocationContext) context).getContexts(), result); + success(context, result); } } - private void success(@Nonnull InvocationContext[] contexts, @Nullable Object result) { + @SuppressWarnings("unchecked") + private void success(@Nonnull Object[] contexts, @Nullable Object result) { for (int i = contexts.length - 1; i > -1; i--) { - handleSuccess(handlers[i], contexts[i], result); + handleSuccess((InvocationEventHandler) handlers[i], contexts[i], result); } } @Override - public void onFailure(@Nullable InvocationContext context, @Nonnull Throwable cause) { + public void onFailure(@Nullable Object[] context, @Nonnull Throwable cause) { debugIfNullContext(context); if (context != null) { - failure(((CompositeInvocationContext) context).getContexts(), cause); + failure(context, cause); } } - private void failure(InvocationContext[] contexts, @Nonnull Throwable cause) { + @SuppressWarnings("unchecked") + private void failure(Object[] contexts, @Nonnull Throwable cause) { for (int i = contexts.length - 1; i > -1; i--) { - handleFailure(handlers[i], contexts[i], cause); + handleFailure((InvocationEventHandler) handlers[i], contexts[i], cause); } } @Nullable - private static InvocationContext handlePreInvocation( - @Nullable InvocationEventHandler handler, + private static Object handlePreInvocation( + @Nullable InvocationEventHandler handler, Object instance, Method method, Object[] args) { @@ -123,7 +123,7 @@ public String toString() { } private static void preInvocationFailed( - @Nullable InvocationEventHandler handler, + @Nullable InvocationEventHandler handler, @Nullable Object instance, Method method, @Nullable Exception exception) { @@ -136,9 +136,9 @@ private static void preInvocationFailed( exception); } - private static void handleSuccess( - InvocationEventHandler handler, - @Nullable InvocationContext context, + private static void handleSuccess( + InvocationEventHandler handler, + @Nullable T context, @Nullable Object result) { if (context != null) { try { @@ -149,9 +149,9 @@ private static void handleSuccess( } } - private static void handleFailure( - InvocationEventHandler handler, - @Nullable InvocationContext context, + private static void handleFailure( + InvocationEventHandler handler, + @Nullable T context, Throwable cause) { if (context != null) { try { @@ -164,7 +164,7 @@ private static void handleFailure( private static void eventFailed( String event, - @Nullable InvocationContext context, + @Nullable Object context, @Nullable Object result, RuntimeException exception) { logger.warn( @@ -174,22 +174,4 @@ private static void eventFailed( UnsafeArg.of("result", result), exception); } - - static class CompositeInvocationContext extends DefaultInvocationContext { - - private final InvocationContext[] contexts; - - CompositeInvocationContext( - Object instance, - Method method, - @Nullable Object[] args, - InvocationContext[] contexts) { - super(System.nanoTime(), instance, method, args); - this.contexts = checkNotNull(contexts); - } - - InvocationContext[] getContexts() { - return contexts; - } - } } diff --git a/tritium-core/src/test/java/com/palantir/tritium/event/CompositeInvocationEventHandlerTest.java b/tritium-core/src/test/java/com/palantir/tritium/event/CompositeInvocationEventHandlerTest.java index 7fc3e5132..6794d18f2 100644 --- a/tritium-core/src/test/java/com/palantir/tritium/event/CompositeInvocationEventHandlerTest.java +++ b/tritium-core/src/test/java/com/palantir/tritium/event/CompositeInvocationEventHandlerTest.java @@ -38,30 +38,32 @@ final class CompositeInvocationEventHandlerTest { private static final Object[] EMPTY_ARGS = {}; @Test - @SuppressWarnings("checkstyle:illegalthrows") + @SuppressWarnings({"checkstyle:illegalthrows", "unchecked"}) void testSimpleFlow() throws Throwable { - InvocationEventHandler compositeHandler = CompositeInvocationEventHandler.of( - Arrays.asList(NoOpInvocationEventHandler.INSTANCE, - new SimpleInvocationEventHandler())); + InvocationEventHandler compositeHandler = + (InvocationEventHandler) CompositeInvocationEventHandler.of( + Arrays.asList(NoOpInvocationEventHandler.INSTANCE, + new SimpleInvocationEventHandler())); assertThat(compositeHandler).isInstanceOf(CompositeInvocationEventHandler.class); - InvocationContext context = compositeHandler.preInvocation(this, getToStringMethod(), EMPTY_ARGS); + Object context = compositeHandler.preInvocation(this, getToStringMethod(), EMPTY_ARGS); compositeHandler.onSuccess(context, "test"); } @Test @SuppressWarnings("unchecked") void testDisabledWhileRunning_success() throws NoSuchMethodException { - InvocationEventHandler handler = mock(InvocationEventHandler.class); + InvocationEventHandler handler = mock(InvocationEventHandler.class); when(handler.preInvocation(any(), any(), any())).thenReturn(mock(InvocationContext.class)); when(handler.isEnabled()).thenReturn(true, false); - InvocationEventHandler compositeHandler = CompositeInvocationEventHandler.of( - Arrays.asList(NoOpInvocationEventHandler.INSTANCE, handler)); + InvocationEventHandler compositeHandler = + (InvocationEventHandler) CompositeInvocationEventHandler.of( + Arrays.asList(NoOpInvocationEventHandler.INSTANCE, handler)); assertThat(compositeHandler).isInstanceOf(CompositeInvocationEventHandler.class); - InvocationContext context = compositeHandler.preInvocation(this, getToStringMethod(), EMPTY_ARGS); + Object context = compositeHandler.preInvocation(this, getToStringMethod(), EMPTY_ARGS); compositeHandler.onSuccess(context, "test"); verify(handler).isEnabled(); @@ -73,15 +75,16 @@ void testDisabledWhileRunning_success() throws NoSuchMethodException { @Test @SuppressWarnings("unchecked") void testDisabledWhileRunning_failure() throws NoSuchMethodException { - InvocationEventHandler handler = mock(InvocationEventHandler.class); + InvocationEventHandler handler = mock(InvocationEventHandler.class); when(handler.preInvocation(any(), any(), any())).thenReturn(mock(InvocationContext.class)); when(handler.isEnabled()).thenReturn(true, false); - InvocationEventHandler compositeHandler = CompositeInvocationEventHandler.of( - Arrays.asList(NoOpInvocationEventHandler.INSTANCE, handler)); + InvocationEventHandler compositeHandler = + (InvocationEventHandler) CompositeInvocationEventHandler.of( + Arrays.asList(NoOpInvocationEventHandler.INSTANCE, handler)); assertThat(compositeHandler).isInstanceOf(CompositeInvocationEventHandler.class); - InvocationContext context = compositeHandler.preInvocation(this, getToStringMethod(), EMPTY_ARGS); + Object context = compositeHandler.preInvocation(this, getToStringMethod(), EMPTY_ARGS); compositeHandler.onFailure(context, new RuntimeException()); verify(handler).isEnabled(); @@ -92,52 +95,53 @@ void testDisabledWhileRunning_failure() throws NoSuchMethodException { @Test void testSuccessHandlerFailureShouldNotThrow() throws Exception { - InvocationEventHandler compositeHandler = CompositeInvocationEventHandler.of( - Arrays.asList(NoOpInvocationEventHandler.INSTANCE, new ThrowingInvocationEventHandler(true) { - @Override - public InvocationContext preInvocation( - @Nonnull Object instance, - @Nonnull Method method, - @Nonnull Object[] args) { - return DefaultInvocationContext.of(instance, method, args); - } - })); + @SuppressWarnings("unchecked") InvocationEventHandler compositeHandler = + (InvocationEventHandler) CompositeInvocationEventHandler.of( + Arrays.asList(NoOpInvocationEventHandler.INSTANCE, new ThrowingInvocationEventHandler(true) { + @Override + public InvocationContext preInvocation( + @Nonnull Object instance, + @Nonnull Method method, + @Nonnull Object[] args) { + return DefaultInvocationContext.of(instance, method, args); + } + })); assertThat(compositeHandler).isInstanceOf(CompositeInvocationEventHandler.class); - InvocationContext context = compositeHandler.preInvocation(this, getToStringMethod(), EMPTY_ARGS); + Object context = compositeHandler.preInvocation(this, getToStringMethod(), EMPTY_ARGS); compositeHandler.onSuccess(context, "test"); } @Test void testFailureHandlerFailureShouldNotThrow() throws Exception { - InvocationEventHandler compositeHandler = CompositeInvocationEventHandler.of( - Arrays.asList(NoOpInvocationEventHandler.INSTANCE, new ThrowingInvocationEventHandler(true) { - @Override - public InvocationContext preInvocation( - @Nonnull Object instance, - @Nonnull Method method, - @Nonnull Object[] args) { - return DefaultInvocationContext.of(instance, method, args); - } - })); + @SuppressWarnings("unchecked") InvocationEventHandler compositeHandler = + (InvocationEventHandler) CompositeInvocationEventHandler.of( + Arrays.asList(NoOpInvocationEventHandler.INSTANCE, new ThrowingInvocationEventHandler(true) { + @Override + public InvocationContext preInvocation( + @Nonnull Object instance, + @Nonnull Method method, + @Nonnull Object[] args) { + return DefaultInvocationContext.of(instance, method, args); + } + })); assertThat(compositeHandler).isInstanceOf(CompositeInvocationEventHandler.class); - InvocationContext context = compositeHandler.preInvocation(this, getToStringMethod(), EMPTY_ARGS); + Object context = compositeHandler.preInvocation(this, getToStringMethod(), EMPTY_ARGS); compositeHandler.onFailure(context, new RuntimeException("simple failure")); } @Test void testEmpty() throws Exception { - InvocationEventHandler compositeHandler = CompositeInvocationEventHandler.of( - Collections.emptyList()); + @SuppressWarnings("unchecked") InvocationEventHandler compositeHandler = + (InvocationEventHandler) CompositeInvocationEventHandler.of(Collections.emptyList()); assertThat(compositeHandler).isInstanceOf(NoOpInvocationEventHandler.class); assertThat(compositeHandler).isSameAs(NoOpInvocationEventHandler.INSTANCE); - InvocationContext context = compositeHandler.preInvocation(this, getToStringMethod(), EMPTY_ARGS); + Object context = compositeHandler.preInvocation(this, getToStringMethod(), EMPTY_ARGS); assertThat(context).isNotNull(); - assertThat(context.getMethod().getName()).isEqualTo("toString"); compositeHandler.onSuccess(context, "Hello World"); compositeHandler.onFailure(context, new RuntimeException()); } @@ -160,44 +164,42 @@ void testNullHandler() { @Test void testPreInvocationThrowingHandler() throws Exception { - @SuppressWarnings("unchecked") List> handlers = + List> handlers = Arrays.asList(NoOpInvocationEventHandler.INSTANCE, createThrowingHandler(true)); - InvocationEventHandler compositeHandler = CompositeInvocationEventHandler.of(handlers); + InvocationEventHandler compositeHandler = CompositeInvocationEventHandler.of(handlers); assertThat(compositeHandler).isInstanceOf(CompositeInvocationEventHandler.class); - InvocationContext context = compositeHandler.preInvocation(this, getToStringMethod(), EMPTY_ARGS); - assertThat(context).isInstanceOf(InvocationContext.class); + compositeHandler.preInvocation(this, getToStringMethod(), EMPTY_ARGS); } @Test void testThrowingOnSuccess() throws Exception { - @SuppressWarnings("unchecked") List> handlers = + List> handlers = Arrays.asList(NoOpInvocationEventHandler.INSTANCE, createThrowingHandler(true)); - InvocationEventHandler compositeHandler = CompositeInvocationEventHandler.of(handlers); + @SuppressWarnings("unchecked") InvocationEventHandler compositeHandler = + (InvocationEventHandler) CompositeInvocationEventHandler.of(handlers); assertThat(compositeHandler).isInstanceOf(CompositeInvocationEventHandler.class); - InvocationContext context = new CompositeInvocationEventHandler.CompositeInvocationContext(this, - getToStringMethod(), null, new InvocationContext[] {null, null}); - compositeHandler.onSuccess(context, "Hello World"); + compositeHandler.onSuccess(new Object[] {null, null}, "Hello World"); } @Test void testThrowingOnFailure() throws Exception { - InvocationEventHandler throwingHandler = createThrowingHandler(true); - @SuppressWarnings("unchecked") List> handlers = + InvocationEventHandler throwingHandler = createThrowingHandler(true); + List> handlers = Arrays.asList(NoOpInvocationEventHandler.INSTANCE, throwingHandler); - InvocationEventHandler compositeHandler = CompositeInvocationEventHandler.of(handlers); + @SuppressWarnings("unchecked") InvocationEventHandler compositeHandler = + (InvocationEventHandler) CompositeInvocationEventHandler.of(handlers); assertThat(compositeHandler).isInstanceOf(CompositeInvocationEventHandler.class); - InvocationContext context = new CompositeInvocationEventHandler.CompositeInvocationContext(this, - getToStringMethod(), null, new InvocationContext[] {null, null}); - compositeHandler.onFailure(context, new RuntimeException()); + compositeHandler.onFailure(new Object[] {null, null}, new RuntimeException()); } @Test void testToString() { - InvocationEventHandler handler = CompositeInvocationEventHandler.of( - Arrays.asList(NoOpInvocationEventHandler.INSTANCE, new SimpleInvocationEventHandler())); + @SuppressWarnings("unchecked") InvocationEventHandler handler = + (InvocationEventHandler) CompositeInvocationEventHandler.of( + Arrays.asList(NoOpInvocationEventHandler.INSTANCE, new SimpleInvocationEventHandler())); assertThat(handler).asString() .startsWith("CompositeInvocationEventHandler{handlers=[INSTANCE, ") .endsWith("]}"); diff --git a/tritium-lib/src/main/java/com/palantir/tritium/proxy/ByteBuddyInstrumentation.java b/tritium-lib/src/main/java/com/palantir/tritium/proxy/ByteBuddyInstrumentation.java index 042511e2a..ccc1c4974 100644 --- a/tritium-lib/src/main/java/com/palantir/tritium/proxy/ByteBuddyInstrumentation.java +++ b/tritium-lib/src/main/java/com/palantir/tritium/proxy/ByteBuddyInstrumentation.java @@ -28,8 +28,6 @@ import com.palantir.logsafe.exceptions.SafeIllegalStateException; import com.palantir.logsafe.exceptions.SafeRuntimeException; import com.palantir.tritium.api.event.InstrumentationFilter; -import com.palantir.tritium.event.CompositeInvocationEventHandler; -import com.palantir.tritium.event.InvocationContext; import com.palantir.tritium.event.InvocationEventHandler; import java.lang.reflect.Method; import java.lang.reflect.Modifier; @@ -69,12 +67,12 @@ private ByteBuddyInstrumentation() { static T instrument( Class interfaceClass, U delegate, - List> handlers, + InvocationEventHandler handler, InstrumentationFilter instrumentationFilter) { checkNotNull(interfaceClass, "interfaceClass"); checkNotNull(delegate, "delegate"); checkNotNull(instrumentationFilter, "instrumentationFilter"); - checkNotNull(handlers, "handlers"); + checkNotNull(handler, "handlers"); if (!isAccessible(interfaceClass)) { log.warn("Interface {} is not accessible. Delegate {} of type {} will not be instrumented", @@ -93,7 +91,7 @@ static T instrument( try { return newInstrumentationClass(classLoader, interfaceClass, additionalInterfaces) .getConstructor(interfaceClass, InvocationEventHandler.class, InstrumentationFilter.class) - .newInstance(delegate, CompositeInvocationEventHandler.of(handlers), instrumentationFilter); + .newInstance(delegate, handler, instrumentationFilter); } catch (ReflectiveOperationException e) { throw new SafeRuntimeException("Failed to instrumented delegate", e); } diff --git a/tritium-lib/src/main/java/com/palantir/tritium/proxy/ByteBuddyInstrumentationAdvice.java b/tritium-lib/src/main/java/com/palantir/tritium/proxy/ByteBuddyInstrumentationAdvice.java index 98738db4a..5edc600d9 100644 --- a/tritium-lib/src/main/java/com/palantir/tritium/proxy/ByteBuddyInstrumentationAdvice.java +++ b/tritium-lib/src/main/java/com/palantir/tritium/proxy/ByteBuddyInstrumentationAdvice.java @@ -20,7 +20,6 @@ import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.UnsafeArg; import com.palantir.tritium.api.event.InstrumentationFilter; -import com.palantir.tritium.event.InvocationContext; import com.palantir.tritium.event.InvocationEventHandler; import java.lang.annotation.ElementType; import java.lang.annotation.Retention; @@ -50,11 +49,11 @@ private ByteBuddyInstrumentationAdvice() {} @Nullable @Advice.OnMethodEnter - static InvocationContext enter( + static Object enter( @Advice.This Object proxy, @Advice.AllArguments Object[] arguments, @Advice.FieldValue("instrumentationFilter") InstrumentationFilter filter, - @Advice.FieldValue("invocationEventHandler") InvocationEventHandler eventHandler, + @Advice.FieldValue("invocationEventHandler") InvocationEventHandler eventHandler, @Advice.FieldValue("methods") Method[] methods, @MethodIndex int index) { Method method = methods[index]; @@ -77,8 +76,8 @@ static InvocationContext enter( static void exit( @Advice.Return(typing = Assigner.Typing.DYNAMIC) Object result, @Advice.Thrown Throwable thrown, - @Advice.FieldValue("invocationEventHandler") InvocationEventHandler eventHandler, - @Advice.Enter InvocationContext context) { + @Advice.FieldValue("invocationEventHandler") InvocationEventHandler eventHandler, + @Advice.Enter Object context) { if (context != null) { try { if (thrown == null) { diff --git a/tritium-lib/src/main/java/com/palantir/tritium/proxy/Instrumentation.java b/tritium-lib/src/main/java/com/palantir/tritium/proxy/Instrumentation.java index a94f04826..5981bf96a 100644 --- a/tritium-lib/src/main/java/com/palantir/tritium/proxy/Instrumentation.java +++ b/tritium-lib/src/main/java/com/palantir/tritium/proxy/Instrumentation.java @@ -22,9 +22,9 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.palantir.tritium.api.event.InstrumentationFilter; +import com.palantir.tritium.event.CompositeInvocationEventHandler; import com.palantir.tritium.event.InstrumentationFilters; import com.palantir.tritium.event.InstrumentationProperties; -import com.palantir.tritium.event.InvocationContext; import com.palantir.tritium.event.InvocationEventHandler; import com.palantir.tritium.event.log.LoggingInvocationEventHandler; import com.palantir.tritium.event.log.LoggingLevel; @@ -46,9 +46,10 @@ private Instrumentation() { throw new UnsupportedOperationException(); } + @SuppressWarnings("unchecked") static T wrap(Class interfaceClass, U delegate, - List> handlers, + List> handlers, InstrumentationFilter instrumentationFilter) { checkNotNull(interfaceClass, "interfaceClass"); checkNotNull(delegate, "delegate"); @@ -59,11 +60,13 @@ static T wrap(Class interfaceClass, return delegate; } + InvocationEventHandler handler = + (InvocationEventHandler) CompositeInvocationEventHandler.of(handlers); if (InstrumentationProperties.isSpecificEnabled("dynamic-proxy", false)) { return Proxies.newProxy(interfaceClass, delegate, - new InstrumentationProxy<>(instrumentationFilter, handlers, delegate)); + new InstrumentationProxy<>(instrumentationFilter, handler, delegate)); } else { - return ByteBuddyInstrumentation.instrument(interfaceClass, delegate, handlers, instrumentationFilter); + return ByteBuddyInstrumentation.instrument(interfaceClass, delegate, handler, instrumentationFilter); } } @@ -75,7 +78,7 @@ static T wrap(Class interfaceClass, @Deprecated static T wrap(Class interfaceClass, U delegate, - List> handlers) { + List> handlers) { return wrap(interfaceClass, delegate, handlers, InstrumentationFilters.INSTRUMENT_ALL); } @@ -111,8 +114,7 @@ public static final class Builder { private final Class interfaceClass; private final U delegate; - private final ImmutableList.Builder> handlers = ImmutableList - .builder(); + private final ImmutableList.Builder> handlers = ImmutableList.builder(); private InstrumentationFilter filter = InstrumentationFilters.INSTRUMENT_ALL; private Builder(Class interfaceClass, U delegate) { @@ -187,12 +189,12 @@ public Builder withLogging(Logger logger, LoggingLevel loggingLevel, return this; } - public Builder withHandler(InvocationEventHandler handler) { + public Builder withHandler(InvocationEventHandler handler) { checkNotNull(handler, "handler"); return withHandlers(Collections.singleton(handler)); } - public Builder withHandlers(Iterable> additionalHandlers) { + public Builder withHandlers(Iterable> additionalHandlers) { checkNotNull(additionalHandlers, "additionalHandlers"); this.handlers.addAll(additionalHandlers); return this; diff --git a/tritium-lib/src/main/java/com/palantir/tritium/proxy/InstrumentationProxy.java b/tritium-lib/src/main/java/com/palantir/tritium/proxy/InstrumentationProxy.java index 3d6228915..aded5d9b0 100644 --- a/tritium-lib/src/main/java/com/palantir/tritium/proxy/InstrumentationProxy.java +++ b/tritium-lib/src/main/java/com/palantir/tritium/proxy/InstrumentationProxy.java @@ -17,17 +17,14 @@ package com.palantir.tritium.proxy; import com.palantir.tritium.api.event.InstrumentationFilter; -import com.palantir.tritium.event.InvocationContext; import com.palantir.tritium.event.InvocationEventHandler; -import java.util.List; -class InstrumentationProxy extends InvocationEventProxy { +class InstrumentationProxy extends InvocationEventProxy { private final T delegate; - InstrumentationProxy(InstrumentationFilter instrumentationFilter, - List> handlers, T delegate) { - super(handlers, instrumentationFilter); + InstrumentationProxy(InstrumentationFilter instrumentationFilter, InvocationEventHandler handler, T delegate) { + super(handler, instrumentationFilter); this.delegate = delegate; } diff --git a/tritium-lib/src/main/java/com/palantir/tritium/proxy/InvocationEventProxy.java b/tritium-lib/src/main/java/com/palantir/tritium/proxy/InvocationEventProxy.java index da0ac4033..86a8b0183 100644 --- a/tritium-lib/src/main/java/com/palantir/tritium/proxy/InvocationEventProxy.java +++ b/tritium-lib/src/main/java/com/palantir/tritium/proxy/InvocationEventProxy.java @@ -24,41 +24,25 @@ import com.palantir.logsafe.UnsafeArg; import com.palantir.logsafe.exceptions.SafeIllegalStateException; import com.palantir.tritium.api.event.InstrumentationFilter; -import com.palantir.tritium.event.CompositeInvocationEventHandler; -import com.palantir.tritium.event.InstrumentationFilters; -import com.palantir.tritium.event.InvocationContext; import com.palantir.tritium.event.InvocationEventHandler; import java.lang.reflect.InvocationHandler; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; -import java.util.List; import javax.annotation.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -abstract class InvocationEventProxy implements InvocationHandler { +abstract class InvocationEventProxy implements InvocationHandler { private static final Logger logger = LoggerFactory.getLogger(InvocationEventProxy.class); private static final Object[] EMPTY_ARRAY = new Object[0]; private final InstrumentationFilter filter; - private final InvocationEventHandler eventHandler; + private final InvocationEventHandler eventHandler; - /** - * Always enabled instrumentation handler. - * - * @param handlers event handlers - */ - protected InvocationEventProxy(List> handlers) { - this(handlers, InstrumentationFilters.INSTRUMENT_ALL); - } - - protected InvocationEventProxy(List> handlers, - InstrumentationFilter filter) { - checkNotNull(filter, "filter"); - checkNotNull(handlers, "handlers"); - this.eventHandler = CompositeInvocationEventHandler.of(handlers); - this.filter = filter; + protected InvocationEventProxy(InvocationEventHandler handler, InstrumentationFilter filter) { + this.eventHandler = checkNotNull(handler, "handler"); + this.filter = checkNotNull(filter, "filter"); } /** @@ -97,7 +81,7 @@ public final Object invoke(Object proxy, Method method, @Nullable Object[] nulla return handleSpecialMethod(proxy, method, arguments); } if (isEnabled(proxy, method, arguments)) { - InvocationContext context = handlePreInvocation(proxy, method, arguments); + T context = handlePreInvocation(proxy, method, arguments); try { Object result = method.invoke(getDelegate(), arguments); return handleOnSuccess(context, result); @@ -148,7 +132,7 @@ private static boolean isToString(Method method, Object[] arguments) { @Nullable @VisibleForTesting - final InvocationContext handlePreInvocation(Object instance, Method method, Object[] args) { + final T handlePreInvocation(Object instance, Method method, Object[] args) { try { return eventHandler.preInvocation(instance, method, args); } catch (RuntimeException e) { @@ -159,7 +143,7 @@ final InvocationContext handlePreInvocation(Object instance, Method method, Obje @Nullable @VisibleForTesting - final Object handleOnSuccess(@Nullable InvocationContext context, @Nullable Object result) { + final Object handleOnSuccess(@Nullable T context, @Nullable Object result) { try { eventHandler.onSuccess(context, result); } catch (RuntimeException e) { @@ -168,14 +152,14 @@ final Object handleOnSuccess(@Nullable InvocationContext context, @Nullable Obje return result; } - private Throwable invocationFailed(@Nullable InvocationContext context, Throwable cause) { + private Throwable invocationFailed(@Nullable T context, Throwable cause) { if (cause instanceof InvocationTargetException) { return handleOnFailure(context, cause.getCause()); } return handleOnFailure(context, cause); } - final Throwable handleOnFailure(@Nullable InvocationContext context, Throwable cause) { + final Throwable handleOnFailure(@Nullable T context, Throwable cause) { try { eventHandler.onFailure(context, cause); } catch (RuntimeException e) { @@ -185,14 +169,14 @@ final Throwable handleOnFailure(@Nullable InvocationContext context, Throwable c } private static void logInvocationWarningOnSuccess( - @Nullable InvocationContext context, + @Nullable Object context, @Nullable Object result, Exception cause) { logInvocationWarning("onSuccess", context, result, cause); } private static void logInvocationWarningOnFailure( - @Nullable InvocationContext context, + @Nullable Object context, @Nullable Throwable result, Exception cause) { logInvocationWarning("onFailure", context, result, cause); @@ -204,7 +188,7 @@ private static SafeArg safeSimpleClassName(@CompileTimeConstant String n static void logInvocationWarning( String event, - @Nullable InvocationContext context, + @Nullable Object context, @Nullable Object result, Throwable cause) { if (logger.isWarnEnabled()) { diff --git a/tritium-lib/src/test/java/com/palantir/tritium/proxy/InvocationEventProxyTest.java b/tritium-lib/src/test/java/com/palantir/tritium/proxy/InvocationEventProxyTest.java index 04f4b612a..59b558b42 100644 --- a/tritium-lib/src/test/java/com/palantir/tritium/proxy/InvocationEventProxyTest.java +++ b/tritium-lib/src/test/java/com/palantir/tritium/proxy/InvocationEventProxyTest.java @@ -25,17 +25,15 @@ import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; -import com.google.common.collect.ImmutableList; import com.palantir.tritium.api.event.InstrumentationFilter; import com.palantir.tritium.event.DefaultInvocationContext; import com.palantir.tritium.event.InstrumentationFilters; import com.palantir.tritium.event.InvocationContext; import com.palantir.tritium.event.InvocationEventHandler; +import com.palantir.tritium.event.NoOpInvocationEventHandler; import com.palantir.tritium.test.TestImplementation; import com.palantir.tritium.test.TestInterface; import java.lang.reflect.Method; -import java.util.Collections; -import java.util.List; import java.util.function.BooleanSupplier; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -58,8 +56,9 @@ public class InvocationEventProxyTest { @Test @SuppressWarnings("checkstyle:illegalthrows") public void testDisabled() throws Throwable { - InvocationEventProxy proxy = new InvocationEventProxy( - Collections.emptyList(), InstrumentationFilters.from((BooleanSupplier) () -> false)) { + + InvocationEventProxy proxy = new InvocationEventProxy( + NoOpInvocationEventHandler.INSTANCE, InstrumentationFilters.from((BooleanSupplier) () -> false)) { @Override Object getDelegate() { return "disabled"; @@ -81,11 +80,8 @@ public void testInstrumentPreInvocation() throws Throwable { assertThat(result2).isInstanceOf(DefaultInvocationContext.class) .asString().contains(InvocationEventProxyTest.class.getName()); - InvocationContext context = proxy.handlePreInvocation(this, getStringLengthMethod(), EMPTY_ARGS); - assertThat(context).asString() - .contains("startTimeNanos") - .contains("instance") - .contains("method"); + Object context = proxy.handlePreInvocation(this, getStringLengthMethod(), EMPTY_ARGS); + assertThat(context).isNotNull(); } @Test @@ -117,7 +113,7 @@ public void onSuccess(@Nullable InvocationContext unusedContext, @Nullable Objec } }; - InvocationEventProxy proxy = createTestProxy(testHandler); + InvocationEventProxy proxy = createTestProxy(testHandler); Object result = proxy.invoke(this, getStringLengthMethod(), EMPTY_ARGS); @@ -142,7 +138,7 @@ public void onFailure(@Nullable InvocationContext unusedContext, @Nonnull Throwa } }; - InvocationEventProxy proxy = createTestProxy(testHandler); + InvocationEventProxy proxy = createTestProxy(testHandler); Object result = proxy.invoke(this, getStringLengthMethod(), EMPTY_ARGS); assertThat(result).isEqualTo("test".length()); @@ -168,8 +164,8 @@ public void testToInvocationContextDebugString() throws Exception { @Test public void testInstrumentToString() { - List> handlers = Collections.emptyList(); - InvocationEventProxy proxy = new InvocationEventProxy(handlers) { + InvocationEventProxy proxy = new InvocationEventProxy( + NoOpInvocationEventHandler.INSTANCE, InstrumentationFilters.INSTRUMENT_ALL) { @Override Object getDelegate() { return "Hello, world"; @@ -282,20 +278,21 @@ public void testThrowingHandler() throws Throwable { verifyZeroInteractions(mockFilter); } - private static InvocationEventProxy createSimpleTestProxy() { - return new TestProxy( + private static InvocationEventProxy createSimpleTestProxy() { + return new TestProxy<>( new TestImplementation(), - ImmutableList.of(new SimpleHandler()), + new SimpleHandler(), InstrumentationFilters.INSTRUMENT_ALL); } - private static InvocationEventProxy createTestProxy( + private static InvocationEventProxy createTestProxy( InvocationEventHandler handler, InstrumentationFilter filter) { - return new TestProxy("test", ImmutableList.of(handler), filter); + return new TestProxy<>("test", handler, filter); } - private static InvocationEventProxy createTestProxy(InvocationEventHandler handler) { + private static InvocationEventProxy createTestProxy( + InvocationEventHandler handler) { return createTestProxy(handler, InstrumentationFilters.INSTRUMENT_ALL); } @@ -337,14 +334,14 @@ public void onSuccess(@Nullable InvocationContext unusedContext, @Nullable Objec public void onFailure(@Nullable InvocationContext unusedContext, @Nonnull Throwable unusedCause) {} } - private static class TestProxy extends InvocationEventProxy { + private static class TestProxy extends InvocationEventProxy { private final Object delegate; TestProxy( Object delegate, - List> handlers, + InvocationEventHandler handler, InstrumentationFilter filter) { - super(handlers, filter); + super(handler, filter); this.delegate = delegate; } diff --git a/tritium-metrics/src/test/java/com/palantir/tritium/event/metrics/TaggedMetricsServiceInvocationEventHandlerTest.java b/tritium-metrics/src/test/java/com/palantir/tritium/event/metrics/TaggedMetricsServiceInvocationEventHandlerTest.java index d286db415..9448a6d7a 100644 --- a/tritium-metrics/src/test/java/com/palantir/tritium/event/metrics/TaggedMetricsServiceInvocationEventHandlerTest.java +++ b/tritium-metrics/src/test/java/com/palantir/tritium/event/metrics/TaggedMetricsServiceInvocationEventHandlerTest.java @@ -82,7 +82,8 @@ void testTaggedServiceMetricsCapturedAsErrors(TaggedMetricRegistry registry) thr @SuppressWarnings("SameParameterValue") private static void invokeMethod( - AbstractInvocationEventHandler handler, Object obj, String methodName, Object result, boolean success) + AbstractInvocationEventHandler handler, Object obj, String methodName, Object result, + boolean success) throws Exception { InvocationContext context = DefaultInvocationContext.of(obj, obj.getClass().getMethod(methodName), null); if (success) {