Use metric-schema for instrumentation metrics (#1962)

changelog/@unreleased/pr-1962.v2.yml

@@ -0,0 +1,7 @@
+type: improvement
+improvement:
+  description: Instrumentation metrics are now defined using metric-schema. There
+    is a single metric name for all instrumented classes with tags for the service
+    name, endpoint, and result.
+  links:
+  - https://github.com/palantir/tritium/pull/1962

tritium-lib/src/main/java/com/palantir/tritium/proxy/Instrumentation.java

@@ -148,13 +148,13 @@ public Builder<T, U> withMetrics(MetricRegistry metricRegistry) {
          * are chosen based off of the interface name and invoked method.
          *
          * @param metricRegistry - TaggedMetricsRegistry used for this application.
-         * @param prefix - Metrics name prefix to be used
+         * @param serviceName - Service name to be used
          * @return - InstrumentationBuilder
          */
-        public Builder<T, U> withTaggedMetrics(TaggedMetricRegistry metricRegistry, String prefix) {
+        public Builder<T, U> withTaggedMetrics(TaggedMetricRegistry metricRegistry, String serviceName) {
             checkNotNull(metricRegistry, "metricRegistry");
-            String serviceName = Strings.isNullOrEmpty(prefix) ? interfaceClass.getName() : prefix;
-            this.handlers.add(new TaggedMetricsServiceInvocationEventHandler(metricRegistry, serviceName));
+            this.handlers.add(new TaggedMetricsServiceInvocationEventHandler(
+                    metricRegistry, Strings.isNullOrEmpty(serviceName) ? interfaceClass.getSimpleName() : serviceName));
             return this;
         }
 

tritium-lib/src/test/java/com/palantir/tritium/proxy/InstrumentationTest.java

@@ -64,6 +64,7 @@
 import java.util.Set;
 import java.util.SortedMap;
 import java.util.function.Supplier;
+import org.assertj.core.api.InstanceOfAssertFactories;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -334,28 +335,48 @@ void testNullFilter() {
     void testTaggedMetrics() {
         TestImplementation delegate = new TestImplementation();
         TestInterface runnable = Instrumentation.builder(TestInterface.class, delegate)
-                .withTaggedMetrics(taggedMetricRegistry, "testPrefix")
+                .withTaggedMetrics(taggedMetricRegistry, "")
                 .withMetrics(metrics)
                 .build();
         assertThat(delegate.invocationCount()).isZero();
         runnable.test();
         assertThat(delegate.invocationCount()).isOne();
         Map<MetricName, Metric> taggedMetrics = taggedMetricRegistry.getMetrics();
-        assertThat(taggedMetrics.keySet())
-                .containsExactly(
-                        MetricName.builder()
-                                .safeName("testPrefix")
-                                .putSafeTags("service-name", "TestInterface")
-                                .putSafeTags("endpoint", "test")
-                                .build(),
-                        // The failures metric is created eagerly
-                        MetricName.builder().safeName("failures").build());
-
-        assertThat(taggedMetrics.values())
-                .first()
-                .isInstanceOf(Timer.class)
-                .extracting(m -> ((Timer) m).getCount())
-                .isEqualTo(1L);
+        assertThat(taggedMetrics.entrySet()).singleElement().satisfies(entry -> {
+            assertThat(entry.getKey().safeName()).isEqualTo("instrumentation.invocation");
+            assertThat(entry.getKey().safeTags())
+                    .containsEntry("service-name", "TestInterface")
+                    .containsEntry("endpoint", "test")
+                    .containsEntry("result", "success");
+            assertThat(entry.getValue())
+                    .asInstanceOf(InstanceOfAssertFactories.type(Timer.class))
+                    .extracting(Timer::getCount, InstanceOfAssertFactories.LONG)
+                    .isOne();
+        });
+    }
+
+    @Test
+    void testTaggedMetrics_serviceName() {
+        TestImplementation delegate = new TestImplementation();
+        TestInterface runnable = Instrumentation.builder(TestInterface.class, delegate)
+                .withTaggedMetrics(taggedMetricRegistry, "testServiceName")
+                .withMetrics(metrics)
+                .build();
+        assertThat(delegate.invocationCount()).isZero();
+        runnable.test();
+        assertThat(delegate.invocationCount()).isOne();
+        Map<MetricName, Metric> taggedMetrics = taggedMetricRegistry.getMetrics();
+        assertThat(taggedMetrics.entrySet()).singleElement().satisfies(entry -> {
+            assertThat(entry.getKey().safeName()).isEqualTo("instrumentation.invocation");
+            assertThat(entry.getKey().safeTags())
+                    .containsEntry("service-name", "testServiceName")
+                    .containsEntry("endpoint", "test")
+                    .containsEntry("result", "success");
+            assertThat(entry.getValue())
+                    .asInstanceOf(InstanceOfAssertFactories.type(Timer.class))
+                    .extracting(Timer::getCount, InstanceOfAssertFactories.LONG)
+                    .isOne();
+        });
     }
 
     @Test

tritium-metrics/src/main/java/com/palantir/tritium/event/metrics/TaggedMetricsServiceInvocationEventHandler.java

@@ -16,16 +16,13 @@
 
 package com.palantir.tritium.event.metrics;
 
-import static com.palantir.logsafe.Preconditions.checkNotNull;
-
-import com.codahale.metrics.Meter;
 import com.codahale.metrics.Timer;
 import com.palantir.logsafe.Safe;
 import com.palantir.tritium.event.AbstractInvocationEventHandler;
 import com.palantir.tritium.event.DefaultInvocationContext;
 import com.palantir.tritium.event.InstrumentationProperties;
 import com.palantir.tritium.event.InvocationContext;
-import com.palantir.tritium.metrics.registry.MetricName;
+import com.palantir.tritium.event.metrics.InstrumentationMetrics.Invocation_Result;
 import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;
 import java.lang.reflect.Method;
 import java.util.concurrent.ConcurrentHashMap;
@@ -44,37 +41,33 @@
  *
  * <ul>
  *   <li>Metric Name: the service name supplied to the constructor
- *   <li>Tag - service-name: The simple name of the invoked class
+ *   <li>Tag - service-name: The service name
  *   <li>Tag - endpoint: The name of the method that was invoked
- *   <li>Tag - cause: When an error is hit, this will be filled with the full class name of the cause.
+ *   <li>Tag - result: {@code success} if the method completed normally or {@code failure} if the method completed
+ *   exceptionally.
  * </ul>
  */
 public class TaggedMetricsServiceInvocationEventHandler extends AbstractInvocationEventHandler<InvocationContext> {
 
-    private static final String FAILURES_METRIC_NAME = "failures";
-    private static final MetricName FAILURES_METRIC =
-            MetricName.builder().safeName(FAILURES_METRIC_NAME).build();
-
-    private final TaggedMetricRegistry taggedMetricRegistry;
-
-    @Safe
-    private final String serviceName;
-
-    private final Meter globalFailureMeter;
-    private final ConcurrentMap<Method, Timer> timerCache = new ConcurrentHashMap<>();
+    private final ConcurrentMap<Method, Timer> successTimerCache = new ConcurrentHashMap<>();
+    private final ConcurrentMap<Method, Timer> failureTimerCache = new ConcurrentHashMap<>();
     private final Function<Method, Timer> onSuccessTimerMappingFunction;
+    private final Function<Method, Timer> onFailureTimerMappingFunction;
 
     public TaggedMetricsServiceInvocationEventHandler(
             TaggedMetricRegistry taggedMetricRegistry, @Safe String serviceName) {
         super(getEnabledSupplier(serviceName));
-        this.taggedMetricRegistry = checkNotNull(taggedMetricRegistry, "metricRegistry");
-        this.serviceName = checkNotNull(serviceName, "serviceName");
-        this.globalFailureMeter = taggedMetricRegistry.meter(FAILURES_METRIC);
-        this.onSuccessTimerMappingFunction = method -> taggedMetricRegistry.timer(MetricName.builder()
-                .safeName(serviceName)
-                .putSafeTags("service-name", method.getDeclaringClass().getSimpleName())
-                .putSafeTags("endpoint", method.getName())
-                .build());
+        InstrumentationMetrics metrics = InstrumentationMetrics.of(taggedMetricRegistry);
+        this.onSuccessTimerMappingFunction = method -> metrics.invocation()
+                .serviceName(serviceName)
+                .endpoint(method.getName())
+                .result(Invocation_Result.SUCCESS)
+                .build();
+        this.onFailureTimerMappingFunction = method -> metrics.invocation()
+                .serviceName(serviceName)
+                .endpoint(method.getName())
+                .result(Invocation_Result.FAILURE)
+                .build();
     }
 
     @SuppressWarnings("NoFunctionalReturnType") // helper
@@ -98,24 +91,20 @@ public final void onSuccess(@Nullable InvocationContext context, @Nullable Objec
         }
     }
 
-    private Timer getSuccessTimer(Method method) {
-        return timerCache.computeIfAbsent(method, onSuccessTimerMappingFunction);
-    }
-
     @Override
-    public final void onFailure(@Nullable InvocationContext context, @Nonnull Throwable cause) {
-        globalFailureMeter.mark();
+    public final void onFailure(@Nullable InvocationContext context, @Nonnull Throwable _cause) {
         debugIfNullContext(context);
         if (context != null) {
-            MetricName failuresMetricName = MetricName.builder()
-                    .safeName(serviceName + "-" + FAILURES_METRIC_NAME)
-                    .putSafeTags(
-                            "service-name",
-                            context.getMethod().getDeclaringClass().getSimpleName())
-                    .putSafeTags("endpoint", context.getMethod().getName())
-                    .putSafeTags("cause", cause.getClass().getName())
-                    .build();
-            taggedMetricRegistry.meter(failuresMetricName).mark();
+            long nanos = System.nanoTime() - context.getStartTimeNanos();
+            getFailureTimer(context.getMethod()).update(nanos, TimeUnit.NANOSECONDS);
         }
     }
+
+    private Timer getSuccessTimer(Method method) {
+        return successTimerCache.computeIfAbsent(method, onSuccessTimerMappingFunction);
+    }
+
+    private Timer getFailureTimer(Method method) {
+        return failureTimerCache.computeIfAbsent(method, onFailureTimerMappingFunction);
+    }
 }