diff --git a/changelog/@unreleased/pr-1962.v2.yml b/changelog/@unreleased/pr-1962.v2.yml new file mode 100644 index 000000000..081960ade --- /dev/null +++ b/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 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 8ab51864c..15fd81a23 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 @@ -148,13 +148,13 @@ public Builder 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 withTaggedMetrics(TaggedMetricRegistry metricRegistry, String prefix) { + public Builder 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; } diff --git a/tritium-lib/src/test/java/com/palantir/tritium/TritiumTest.java b/tritium-lib/src/test/java/com/palantir/tritium/TritiumTest.java deleted file mode 100644 index 79643e6b3..000000000 --- a/tritium-lib/src/test/java/com/palantir/tritium/TritiumTest.java +++ /dev/null @@ -1,180 +0,0 @@ -/* - * (c) Copyright 2016 Palantir Technologies Inc. All rights reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.palantir.tritium; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; - -import com.codahale.metrics.ConsoleReporter; -import com.codahale.metrics.Metric; -import com.codahale.metrics.MetricRegistry; -import com.codahale.metrics.Slf4jReporter; -import com.codahale.metrics.Timer; -import com.palantir.tritium.event.log.LoggingLevel; -import com.palantir.tritium.metrics.MetricRegistries; -import com.palantir.tritium.metrics.registry.DefaultTaggedMetricRegistry; -import com.palantir.tritium.metrics.registry.MetricName; -import com.palantir.tritium.metrics.registry.TaggedMetricRegistry; -import com.palantir.tritium.test.TestImplementation; -import com.palantir.tritium.test.TestInterface; -import java.util.Map; -import java.util.SortedMap; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeAll; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.slf4j.impl.SimpleLogger; -import org.slf4j.impl.TestLogs; -import uk.org.webcompere.systemstubs.jupiter.SystemStub; -import uk.org.webcompere.systemstubs.jupiter.SystemStubsExtension; -import uk.org.webcompere.systemstubs.properties.SystemProperties; - -@ExtendWith(SystemStubsExtension.class) -@SuppressWarnings("NullAway") -public class TritiumTest { - @SystemStub - private static SystemProperties systemProperties; - - @BeforeAll - static void beforeAll() { - systemProperties.set("org.slf4j.simpleLogger.log.performance", LoggingLevel.TRACE.name()); - TestLogs.setLevel("performance", LoggingLevel.TRACE.name()); - TestLogs.logTo("/dev/null"); - } - - private static final String EXPECTED_METRIC_NAME = TestInterface.class.getName() + ".test"; - private static final String LOG_KEY = SimpleLogger.LOG_KEY_PREFIX + "com.palantir"; - - private final TestImplementation delegate = new TestImplementation(); - private final MetricRegistry metricRegistry = MetricRegistries.createWithHdrHistogramReservoirs(); - private final TaggedMetricRegistry taggedMetricRegistry = new DefaultTaggedMetricRegistry(); - private final TestInterface instrumentedService = Tritium.instrument(TestInterface.class, delegate, metricRegistry); - private final TestInterface taggedInstrumentedService = - Tritium.instrument(TestInterface.class, delegate, taggedMetricRegistry); - private static final MetricName EXPECTED_TAGGED_METRIC_NAME = MetricName.builder() - .safeName("com.palantir.tritium.test.TestInterface") - .putSafeTags("service-name", "TestInterface") - .putSafeTags("endpoint", "test") - .build(); - - @BeforeEach - public void before() { - systemProperties.set(LOG_KEY, LoggingLevel.TRACE.name()); - } - - @AfterEach - public void after() { - systemProperties.remove(LOG_KEY); - - try (ConsoleReporter reporter = - ConsoleReporter.forRegistry(metricRegistry).build()) { - reporter.report(); - Tagged.report(reporter, taggedMetricRegistry); - } - } - - @Test - @SuppressWarnings("JdkObsolete") // SortedMap is part of Metrics API - public void testInstrument() { - assertThat(delegate.invocationCount()).isZero(); - assertThat(metricRegistry.getTimers()).doesNotContainKey(Runnable.class.getName()); - - instrumentedService.test(); - assertThat(delegate.invocationCount()).isOne(); - - SortedMap timers = metricRegistry.getTimers(); - assertThat(timers.keySet()).hasSize(1); - assertThat(timers.keySet()).containsExactlyInAnyOrder(EXPECTED_METRIC_NAME); - assertThat(timers).containsKey(EXPECTED_METRIC_NAME); - assertThat(timers.get(EXPECTED_METRIC_NAME).getCount()).isOne(); - - instrumentedService.test(); - - assertThat(timers.get(EXPECTED_METRIC_NAME).getCount()).isEqualTo(delegate.invocationCount()); - assertThat(timers.get(EXPECTED_METRIC_NAME).getSnapshot().getMax()).isGreaterThan(-1L); - - Slf4jReporter.forRegistry(metricRegistry) - .withLoggingLevel(Slf4jReporter.LoggingLevel.INFO) - .build() - .report(); - } - - @Test - public void testInstrumentWithTags() { - assertThat(delegate.invocationCount()).isZero(); - assertThat(taggedMetricRegistry.getMetrics()) - // The global failures metric is created eagerly - .containsOnlyKeys(MetricName.builder().safeName("failures").build()); - - taggedInstrumentedService.test(); - assertThat(delegate.invocationCount()).isOne(); - - Map metrics = taggedMetricRegistry.getMetrics(); - assertThat(metrics.keySet()) - .containsExactly( - EXPECTED_TAGGED_METRIC_NAME, - MetricName.builder().safeName("failures").build()); - Metric actual = metrics.get(EXPECTED_TAGGED_METRIC_NAME); - assertThat(actual).isInstanceOf(Timer.class); - Timer timer = (Timer) actual; - assertThat(timer.getCount()).isOne(); - - taggedInstrumentedService.test(); - - assertThat(metrics.get(EXPECTED_TAGGED_METRIC_NAME)).isSameAs(timer); - assertThat(timer.getCount()).isEqualTo(2); - assertThat(timer.getSnapshot().getMax()).isGreaterThan(-1); - } - - @Test - public void rethrowOutOfMemoryError() { - assertThatThrownBy(instrumentedService::throwsOutOfMemoryError) - .isInstanceOf(OutOfMemoryError.class) - .hasMessage("Testing OOM"); - } - - @Test - public void rethrowOutOfMemoryErrorMetrics() { - String methodMetricName = MetricRegistry.name(TestInterface.class, "throwsOutOfMemoryError"); - assertThat(metricRegistry - .meter(MetricRegistry.name(methodMetricName, "failures")) - .getCount()) - .isZero(); - assertThat(metricRegistry - .meter(MetricRegistry.name(methodMetricName, "failures", "java.lang.OutOfMemoryError")) - .getCount()) - .isZero(); - - assertThatThrownBy(instrumentedService::throwsOutOfMemoryError) - .isInstanceOf(OutOfMemoryError.class) - .hasMessage("Testing OOM"); - - assertThat(metricRegistry - .meter(MetricRegistry.name(methodMetricName, "failures")) - .getCount()) - .isOne(); - } - - @Test - public void testToString() { - assertThat(instrumentedService.toString()).isEqualTo(TestImplementation.class.getName()); - assertThat(Tritium.instrument(TestInterface.class, instrumentedService, metricRegistry) - .toString()) - .isEqualTo(TestImplementation.class.getName()); - } -} diff --git a/tritium-lib/src/test/java/com/palantir/tritium/proxy/InstrumentationTest.java b/tritium-lib/src/test/java/com/palantir/tritium/proxy/InstrumentationTest.java index ff3546799..353d28c22 100644 --- a/tritium-lib/src/test/java/com/palantir/tritium/proxy/InstrumentationTest.java +++ b/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 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 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 diff --git a/tritium-metrics/src/main/java/com/palantir/tritium/event/metrics/TaggedMetricsServiceInvocationEventHandler.java b/tritium-metrics/src/main/java/com/palantir/tritium/event/metrics/TaggedMetricsServiceInvocationEventHandler.java index 7a2045a01..634e53924 100644 --- a/tritium-metrics/src/main/java/com/palantir/tritium/event/metrics/TaggedMetricsServiceInvocationEventHandler.java +++ b/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 @@ * *
    *
  • Metric Name: the service name supplied to the constructor - *
  • Tag - service-name: The simple name of the invoked class + *
  • Tag - service-name: The service name *
  • Tag - endpoint: The name of the method that was invoked - *
  • Tag - cause: When an error is hit, this will be filled with the full class name of the cause. + *
  • Tag - result: {@code success} if the method completed normally or {@code failure} if the method completed + * exceptionally. *
*/ public class TaggedMetricsServiceInvocationEventHandler extends AbstractInvocationEventHandler { - 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 timerCache = new ConcurrentHashMap<>(); + private final ConcurrentMap successTimerCache = new ConcurrentHashMap<>(); + private final ConcurrentMap failureTimerCache = new ConcurrentHashMap<>(); private final Function onSuccessTimerMappingFunction; + private final Function 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); + } } diff --git a/tritium-metrics/src/main/metrics/event-metrics.yml b/tritium-metrics/src/main/metrics/event-metrics.yml new file mode 100644 index 000000000..956aa7971 --- /dev/null +++ b/tritium-metrics/src/main/metrics/event-metrics.yml @@ -0,0 +1,15 @@ +options: + javaPackage: com.palantir.tritium.event.metrics + javaVisibility: packagePrivate +namespaces: + instrumentation: + docs: Instrumentation metrics. + metrics: + invocation: + type: timer + tags: + - name: service-name + - name: endpoint + - name: result + values: [ success, failure ] + docs: A timer of the time it took to execute an invocation. 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 c20241143..66a0462ae 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 @@ -23,6 +23,7 @@ import com.palantir.tritium.event.AbstractInvocationEventHandler; import com.palantir.tritium.event.DefaultInvocationContext; import com.palantir.tritium.event.InvocationContext; +import com.palantir.tritium.event.metrics.InstrumentationMetrics.Invocation_Result; import com.palantir.tritium.metrics.registry.MetricName; import com.palantir.tritium.metrics.registry.TaggedMetricRegistry; import com.palantir.tritium.metrics.test.TestTaggedMetricRegistries; @@ -46,16 +47,17 @@ void testTaggedServiceMetricsCaptured(TaggedMetricRegistry registry) throws Exce TestImplementation testInterface = new TestImplementation(); TaggedMetricsServiceInvocationEventHandler handler = - new TaggedMetricsServiceInvocationEventHandler(registry, "quux"); + new TaggedMetricsServiceInvocationEventHandler(registry, "serviceName"); invokeMethod(handler, testInterface, "doFoo", "bar", /* success= */ true); Map metrics = registry.getMetrics(); - MetricName expectedMetricName = MetricName.builder() - .safeName("quux") - .putSafeTags("service-name", "TestImplementation") - .putSafeTags("endpoint", "doFoo") - .build(); + MetricName expectedMetricName = InstrumentationMetrics.of(registry) + .invocation() + .serviceName("serviceName") + .endpoint("doFoo") + .result(Invocation_Result.SUCCESS) + .buildMetricName(); assertThat(metrics).containsKey(expectedMetricName); } @@ -65,17 +67,17 @@ void testTaggedServiceMetricsCapturedAsErrors(TaggedMetricRegistry registry) thr TestImplementation testInterface = new TestImplementation(); TaggedMetricsServiceInvocationEventHandler handler = - new TaggedMetricsServiceInvocationEventHandler(registry, "quux"); + new TaggedMetricsServiceInvocationEventHandler(registry, "serviceName"); invokeMethod(handler, testInterface, "doFoo", "bar", /* success= */ false); Map metrics = registry.getMetrics(); - MetricName expectedMetricName = MetricName.builder() - .safeName("quux-failures") - .putSafeTags("service-name", "TestImplementation") - .putSafeTags("endpoint", "doFoo") - .putSafeTags("cause", SafeRuntimeException.class.getName()) - .build(); + MetricName expectedMetricName = InstrumentationMetrics.of(registry) + .invocation() + .serviceName("serviceName") + .endpoint("doFoo") + .result(Invocation_Result.FAILURE) + .buildMetricName(); assertThat(metrics).containsKey(expectedMetricName); }