Use Byte Buddy for Instrumentation by default (#389)

Use Byte Buddy for Instrumentation by default, reducing excess stack frames from instrumentation.
palantir · Sep 9, 2019 · 124f7b5 · 124f7b5
1 parent 3132380
commit 124f7b5
Show file tree

Hide file tree

Showing 4 changed files with 102 additions and 4 deletions.
diff --git a/changelog/@unreleased/pr-389.v2.yml b/changelog/@unreleased/pr-389.v2.yml
@@ -0,0 +1,6 @@
+type: feature
+feature:
+  description: Use Byte Buddy for Instrumentation by default, reducing excess stack
+    frames from instrumentation.
+  links:
+  - https://github.com/palantir/tritium/pull/389
diff --git a/tritium-core/src/main/java/com/palantir/tritium/event/InstrumentationProperties.java b/tritium-core/src/main/java/com/palantir/tritium/event/InstrumentationProperties.java
@@ -26,6 +26,7 @@
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
 import java.util.function.Supplier;
+import javax.annotation.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -46,8 +47,22 @@ public static BooleanSupplier getSystemPropertySupplier(String name) {
 
     @SuppressWarnings("WeakerAccess") // public API
     public static boolean isSpecificEnabled(String name) {
-        String qualifiedValue = instrumentationProperties().get(INSTRUMENT_PREFIX + "." + name);
-        return "true".equalsIgnoreCase(qualifiedValue) || qualifiedValue == null;
+        return isSpecificEnabled(name, true);
+    }
+
+    @SuppressWarnings("WeakerAccess") // public API
+    public static boolean isSpecificEnabled(String name, boolean defaultValue) {
+        String qualifiedValue = getSpecific(name);
+        if (qualifiedValue == null) {
+            return defaultValue;
+        }
+        return "true".equalsIgnoreCase(qualifiedValue);
+    }
+
+    /** Applies the {@link #INSTRUMENT_PREFIX} and returns the current value. */
+    @Nullable
+    private static String getSpecific(String name) {
+        return instrumentationProperties().get(INSTRUMENT_PREFIX + "." + name);
     }
 
     @SuppressWarnings("WeakerAccess") // public API
@@ -100,5 +115,4 @@ private static Map<String, String> createInstrumentationSystemProperties() {
         log.debug("Reloaded instrumentation properties {}", map);
         return map;
     }
-
 }
diff --git a/tritium-core/src/test/java/com/palantir/tritium/event/InstrumentationPropertiesTest.java b/tritium-core/src/test/java/com/palantir/tritium/event/InstrumentationPropertiesTest.java
@@ -173,4 +173,82 @@ public void onFailure(Throwable throwable) {
             assertThat(barrier.getNumberWaiting()).isZero();
         });
     }
+
+    @Test
+    void testIsSpecificEnabled_notSet() {
+        assertThat(InstrumentationProperties.isSpecificEnabled("not_set")).isTrue();
+    }
+
+    @Test
+    void testIsSpecificEnabled_notSet_defaultTrue() {
+        assertThat(InstrumentationProperties.isSpecificEnabled("not_set", true)).isTrue();
+    }
+
+    @Test
+    void testIsSpecificEnabled_notSet_defaultFalse() {
+        assertThat(InstrumentationProperties.isSpecificEnabled("not_set", false)).isFalse();
+    }
+
+    @Test
+    void testIsSpecificEnabled_setGarbage() {
+        System.setProperty("instrument.garbage", "garbage");
+        InstrumentationProperties.reload();
+        assertThat(InstrumentationProperties.isSpecificEnabled("garbage")).isFalse();
+    }
+
+    @Test
+    void testIsSpecificEnabled_setGarbage_defaultTrue() {
+        System.setProperty("instrument.garbage", "garbage");
+        InstrumentationProperties.reload();
+        assertThat(InstrumentationProperties.isSpecificEnabled("garbage", true)).isFalse();
+    }
+
+    @Test
+    void testIsSpecificEnabled_setGarbage_defaultFalse() {
+        System.setProperty("instrument.garbage", "garbage");
+        InstrumentationProperties.reload();
+        assertThat(InstrumentationProperties.isSpecificEnabled("garbage", false)).isFalse();
+    }
+
+    @Test
+    void testIsSpecificEnabled_setTrue() {
+        System.setProperty("instrument.true", "true");
+        InstrumentationProperties.reload();
+        assertThat(InstrumentationProperties.isSpecificEnabled("true")).isTrue();
+    }
+
+    @Test
+    void testIsSpecificEnabled_setTrue_defaultTrue() {
+        System.setProperty("instrument.true", "true");
+        InstrumentationProperties.reload();
+        assertThat(InstrumentationProperties.isSpecificEnabled("true", true)).isTrue();
+    }
+
+    @Test
+    void testIsSpecificEnabled_setTrue_defaultFalse() {
+        System.setProperty("instrument.true", "true");
+        InstrumentationProperties.reload();
+        assertThat(InstrumentationProperties.isSpecificEnabled("true", false)).isTrue();
+    }
+
+    @Test
+    void testIsSpecificEnabled_setFalse() {
+        System.setProperty("instrument.false", "false");
+        InstrumentationProperties.reload();
+        assertThat(InstrumentationProperties.isSpecificEnabled("false")).isFalse();
+    }
+
+    @Test
+    void testIsSpecificEnabled_setFalse_defaultTrue() {
+        System.setProperty("instrument.false", "false");
+        InstrumentationProperties.reload();
+        assertThat(InstrumentationProperties.isSpecificEnabled("false", true)).isFalse();
+    }
+
+    @Test
+    void testIsSpecificEnabled_setFalse_defaultFalse() {
+        System.setProperty("instrument.false", "false");
+        InstrumentationProperties.reload();
+        assertThat(InstrumentationProperties.isSpecificEnabled("false", false)).isFalse();
+    }
 }
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
@@ -59,7 +59,7 @@ static <T, U extends T> T wrap(Class<T> interfaceClass,
             return delegate;
         }
 
-        if (InstrumentationProperties.isSpecificEnabled("dynamic-proxy")) {
+        if (InstrumentationProperties.isSpecificEnabled("dynamic-proxy", false)) {
             return Proxies.newProxy(interfaceClass, delegate,
                     new InstrumentationProxy<>(instrumentationFilter, handlers, delegate));
         } else {