Skip to content

Commit

Permalink
Remove unused @MetricGroup annotation helpers (#1478)
Browse files Browse the repository at this point in the history 
Remove unused @MetricGroup annotation helpers

This was never used and the code for it has rotted, so time to purge.
The @MetricGroup annotation still exists to prevent any immediate ABI
breaks, but have been deprecated and will be removed in future.
  • Loading branch information
@schlosna
schlosna authored May 5, 2022
1 parent 54ba114 commit 4edebab
Show file tree
Hide file tree
Showing 8 changed files with 36 additions and 391 deletions.
10 changes: 10 additions & 0 deletions changelog/@unreleased/pr-1478.v2.yml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
type: deprecation
deprecation:
description: |-
Remove unused @MetricGroup annotation helpers
This was never used and the code for it has rotted, so time to purge.
The @MetricGroup annotation still exists to prevent any immediate ABI
breaks, but have been deprecated and will be removed in future.
links:
- https://github.com/palantir/tritium/pull/1478
4 changes: 0 additions & 4 deletions tritium-lib/src/test/java/com/palantir/tritium/TritiumTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,6 @@ public void rethrowOutOfMemoryErrorMetrics() {
.meter(MetricRegistry.name(methodMetricName, "failures"))
.getCount())
.isOne();
assertThat(metricRegistry
.meter(MetricRegistry.name(methodMetricName, "failures", "java.lang.OutOfMemoryError"))
.getCount())
.isOne();
}

@Test
Expand Down
44 changes: 0 additions & 44 deletions tritium-lib/src/test/java/com/palantir/tritium/proxy/InstrumentationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
import com.palantir.tritium.event.log.LoggingInvocationEventHandler;
import com.palantir.tritium.event.metrics.MetricsInvocationEventHandler;
import com.palantir.tritium.event.metrics.TaggedMetricsServiceInvocationEventHandler;
import com.palantir.tritium.event.metrics.annotations.MetricGroup;
import com.palantir.tritium.metrics.MetricRegistries;
import com.palantir.tritium.metrics.registry.DefaultTaggedMetricRegistry;
import com.palantir.tritium.metrics.registry.MetricName;
Expand Down Expand Up @@ -80,17 +79,6 @@
@SuppressWarnings({"NullAway", "SystemOut", "WeakerAccess"}) // mock injection, dumping metrics to standard out
public abstract class InstrumentationTest {

@MetricGroup("DEFAULT")
public interface AnnotatedInterface {
@MetricGroup("ONE")
void method();

@MetricGroup("ONE")
void otherMethod();

void defaultMethod();
}

private static final String EXPECTED_METRIC_NAME = TestInterface.class.getName() + ".test";

// Exceed the HotSpot JIT thresholds
Expand Down Expand Up @@ -177,38 +165,6 @@ void testBuilder() {
.report();
}

@Test
void testMetricGroupBuilder() {
AnnotatedInterface delegate = mock(AnnotatedInterface.class);
String globalPrefix = "com.business.service";

MetricRegistry metricRegistry = MetricRegistries.createWithHdrHistogramReservoirs();

AnnotatedInterface instrumentedService = Instrumentation.builder(AnnotatedInterface.class, delegate)
.withMetrics(metricRegistry, globalPrefix)
.withPerformanceTraceLogging()
.build();
// call
instrumentedService.method();
instrumentedService.otherMethod();
instrumentedService.defaultMethod();

assertThat(metricRegistry
.timer(AnnotatedInterface.class.getName() + ".ONE")
.getCount())
.isEqualTo(2L);
assertThat(metricRegistry.timer(globalPrefix + ".ONE").getCount()).isEqualTo(2L);
assertThat(metricRegistry
.timer(AnnotatedInterface.class.getName() + ".DEFAULT")
.getCount())
.isOne();
assertThat(metricRegistry.timer(globalPrefix + ".DEFAULT").getCount()).isOne();
assertThat(metricRegistry
.timer(AnnotatedInterface.class.getName() + ".method")
.getCount())
.isOne();
}

private void executeManyTimes(TestInterface instrumentedService, int invocations) {
Stopwatch timer = Stopwatch.createStarted();
for (int i = 0; i < invocations; i++) {
Expand Down
88 changes: 6 additions & 82 deletions ...trics/src/main/java/com/palantir/tritium/event/metrics/MetricsInvocationEventHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,12 @@
import static com.palantir.logsafe.Preconditions.checkNotNull;

import com.codahale.metrics.MetricRegistry;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableMap;
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.event.InvocationEventHandler;
import com.palantir.tritium.event.metrics.annotations.AnnotationHelper;
import com.palantir.tritium.event.metrics.annotations.MetricGroup;
import java.lang.reflect.Method;
import java.util.concurrent.TimeUnit;
import javax.annotation.Nonnull;
Expand All @@ -44,33 +40,22 @@ public final class MetricsInvocationEventHandler extends AbstractInvocationEvent
@Safe
private final String serviceName;

// consider creating annotation handlers as separate objects
private final ImmutableMap<AnnotationHelper.MethodSignature, String> metricGroups;

@Safe
@Nullable
private final String globalGroupPrefix;

@SuppressWarnings("WeakerAccess") // public API
public MetricsInvocationEventHandler(MetricRegistry metricRegistry, @Safe String serviceName) {
super(getEnabledSupplier(serviceName));
this.metricRegistry = checkNotNull(metricRegistry, "metricRegistry");
this.serviceName = checkNotNull(serviceName, "serviceName");
this.metricGroups = ImmutableMap.of();
this.globalGroupPrefix = null;
}

@SuppressWarnings("InconsistentOverloads")
public MetricsInvocationEventHandler(
MetricRegistry metricRegistry,
Class<?> serviceClass,
Class<?> _serviceClass,
@Safe String serviceName,
@Safe @Nullable String globalGroupPrefix) {
@Safe @Nullable String _globalGroupPrefix) {
super(getEnabledSupplier(serviceName));
this.metricRegistry = checkNotNull(metricRegistry, "metricRegistry");
this.serviceName = checkNotNull(serviceName, "serviceName");
this.metricGroups = createMethodGroupMapping(checkNotNull(serviceClass));
this.globalGroupPrefix = Strings.emptyToNull(globalGroupPrefix);
}

@SuppressWarnings("WeakerAccess") // public API
Expand All @@ -79,26 +64,6 @@ public MetricsInvocationEventHandler(
this(metricRegistry, serviceClass, checkNotNull(serviceClass.getName()), globalGroupPrefix);
}

private static ImmutableMap<AnnotationHelper.MethodSignature, String> createMethodGroupMapping(
Class<?> serviceClass) {
ImmutableMap.Builder<AnnotationHelper.MethodSignature, String> builder = ImmutableMap.builder();

MetricGroup classGroup = AnnotationHelper.getSuperTypeAnnotation(serviceClass, MetricGroup.class);

for (Method method : serviceClass.getMethods()) {
AnnotationHelper.MethodSignature sig = AnnotationHelper.MethodSignature.of(method);
MetricGroup methodGroup = AnnotationHelper.getMethodAnnotation(MetricGroup.class, serviceClass, sig);

if (methodGroup != null) {
builder.put(sig, methodGroup.value());
} else if (classGroup != null) {
builder.put(sig, classGroup.value());
}
}

return builder.build();
}

// explicitly qualifying BooleanSupplier types for deconfliction
@SuppressWarnings({"NoFunctionalReturnType", "UnnecessarilyFullyQualified"})
static java.util.function.BooleanSupplier getEnabledSupplier(String serviceName) {
Expand All @@ -114,31 +79,23 @@ public InvocationContext preInvocation(@Nonnull Object instance, @Nonnull Method
public void onSuccess(@Nullable InvocationContext context, @Nullable Object _result) {
debugIfNullContext(context);
if (context != null) {
long nanos = updateTimer(context);
handleSuccessAnnotations(context, nanos);
updateTimer(context);
}
}

@Override
public void onFailure(@Nullable InvocationContext context, @Nonnull Throwable cause) {
public void onFailure(@Nullable InvocationContext context, @Nonnull Throwable _cause) {
markGlobalFailure();
debugIfNullContext(context);
if (context != null) {
String failuresMetricName = getBaseMetricName(context) + '.' + FAILURES;
metricRegistry.meter(failuresMetricName).mark();
metricRegistry
.meter(failuresMetricName + '.' + cause.getClass().getName())
.mark();
long nanos = updateTimer(context);
handleFailureAnnotations(context, nanos);
metricRegistry.meter(getBaseMetricName(context) + '.' + FAILURES).mark();
}
}

@SuppressWarnings("PreferJavaTimeOverload") // performance sensitive
private long updateTimer(InvocationContext context) {
private void updateTimer(InvocationContext context) {
long nanos = System.nanoTime() - context.getStartTimeNanos();
metricRegistry.timer(getBaseMetricName(context)).update(nanos, TimeUnit.NANOSECONDS);
return nanos;
}

private String getBaseMetricName(InvocationContext context) {
Expand All @@ -148,37 +105,4 @@ private String getBaseMetricName(InvocationContext context) {
private void markGlobalFailure() {
metricRegistry.meter(FAILURES).mark();
}

@SuppressWarnings("PreferJavaTimeOverload") // performance sensitive
private void handleSuccessAnnotations(InvocationContext context, long nanos) {
String metricName = getAnnotatedMetricName(context);
if (metricName != null) {
metricRegistry.timer(serviceName + '.' + metricName).update(nanos, TimeUnit.NANOSECONDS);

if (globalGroupPrefix != null) {
metricRegistry.timer(globalGroupPrefix + '.' + metricName).update(nanos, TimeUnit.NANOSECONDS);
}
}
}

@SuppressWarnings("PreferJavaTimeOverload") // performance sensitive
private void handleFailureAnnotations(InvocationContext context, long nanos) {
String metricName = getAnnotatedMetricName(context);
if (metricName != null) {
metricRegistry
.timer(serviceName + '.' + metricName + '.' + FAILURES)
.update(nanos, TimeUnit.NANOSECONDS);

if (globalGroupPrefix != null) {
metricRegistry
.timer(globalGroupPrefix + '.' + metricName + '.' + FAILURES)
.update(nanos, TimeUnit.NANOSECONDS);
}
}
}

@Nullable
private String getAnnotatedMetricName(InvocationContext context) {
return metricGroups.get(AnnotationHelper.MethodSignature.of(context.getMethod()));
}
}
77 changes: 17 additions & 60 deletions ...etrics/src/main/java/com/palantir/tritium/event/metrics/annotations/AnnotationHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,90 +18,47 @@

import static com.palantir.logsafe.Preconditions.checkNotNull;

import com.google.common.annotations.VisibleForTesting;
import java.lang.annotation.Annotation;
import java.lang.reflect.Method;
import java.util.Arrays;
import java.util.Collections;
import java.util.IdentityHashMap;
import java.util.Set;
import javax.annotation.Nullable;

/**
* Do not use, will be removed in future release.
* @deprecated Do not use, will be removed in future release.
*/
@Deprecated
@SuppressWarnings("InlineMeSuggester") // this will just go away
public final class AnnotationHelper {

private AnnotationHelper() {
throw new UnsupportedOperationException();
}

/**
* Annotation as implemented on passed in type or parent of that type, works for both super classes and interfaces.
*
* @param clazz - Class type to scan for annotations
* @param annotation - Annotation type to scan for
* @return - First matching annotation found in depth first search, or null if not found
* Do not use, will be removed in future release.
* @return always null
* @deprecated Do not use, will be removed in future release.
*/
@Deprecated
@Nullable
public static <T extends Annotation> T getSuperTypeAnnotation(Class<?> clazz, Class<T> annotation) {
if (clazz.isAnnotationPresent(annotation)) {
return clazz.getAnnotation(annotation);
}

for (Class<?> ifaces : getParentClasses(clazz)) {
T superAnnotation = getSuperTypeAnnotation(ifaces, annotation);
if (superAnnotation != null) {
return superAnnotation;
}
}

public static <T extends Annotation> T getSuperTypeAnnotation(Class<?> _clazz, Class<T> _annotation) {
return null;
}

/**
* Depth first search up the Type hierarchy to find a matching annotation, Types which do not implement the
* specified method signature are ignored.
*
* @param annotation - Annotation type to scan for
* @param clazz - Class type to scan for matching annotations
* @param methodSignature - Method to search annotation for
* @return - First found matching annotation or null
* Do not use, will be removed in future release.
* @return always null
* @deprecated Do not use, will be removed in future release.
*/
@Deprecated
@Nullable
public static <T extends Annotation> T getMethodAnnotation(
Class<T> annotation, Class<?> clazz, MethodSignature methodSignature) {
Method method;
try {
method = clazz.getMethod(methodSignature.getMethodName(), methodSignature.getParameterTypes());
} catch (NoSuchMethodException e) {
return null;
}

if (method.isAnnotationPresent(annotation)) {
return method.getAnnotation(annotation);
}

for (Class<?> iface : getParentClasses(clazz)) {
T foundAnnotation = getMethodAnnotation(annotation, iface, methodSignature);
if (foundAnnotation != null) {
return foundAnnotation;
}
}

Class<T> _annotation, Class<?> _clazz, MethodSignature _methodSignature) {
return null;
}

@VisibleForTesting
private static Set<Class<?>> getParentClasses(Class<?> clazz) {
Set<Class<?>> parentClasses = Collections.newSetFromMap(new IdentityHashMap<>());
parentClasses.addAll(Arrays.asList(clazz.getInterfaces()));
Class<?> superclass = clazz.getSuperclass();
while (superclass != null) {
parentClasses.addAll(Arrays.asList(superclass.getInterfaces()));
parentClasses.add(superclass);
superclass = superclass.getSuperclass();
}
return parentClasses;
}

@Deprecated
public static final class MethodSignature {
private final String methodName;
private final Class<?>[] parameterTypes;
Expand Down
5 changes: 3 additions & 2 deletions ...ium-metrics/src/main/java/com/palantir/tritium/event/metrics/annotations/MetricGroup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,12 @@
import java.lang.annotation.Target;

/**
* Instrumentation instruction to tritium to group calls into a common metric name. Setting Type (class) level applies a
* default metric group to all Methods that are annotated
* Do not use, will be removed in future release.
* @deprecated Do not use, will be removed in future release.
*/
@Target({ElementType.METHOD, ElementType.TYPE})
@Retention(RetentionPolicy.RUNTIME)
@Deprecated
public @interface MetricGroup {
/**
* String identifier grouped metrics.
Expand Down
Loading

0 comments on commit 4edebab

Please sign in to comment.