Skip to content

Commit

Permalink
Merge pull request #252 from ryanrupp/timedInterfaceFix
Browse files Browse the repository at this point in the history 
Fixed an issue that timers created by the TimedInterface wouldn't be reg...
  • Loading branch information
@dmuino
dmuino committed May 25, 2014
2 parents 0e302df + 41c19d6 commit 2f71d3c
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 29 deletions.
42 changes: 20 additions & 22 deletions servo-core/src/main/java/com/netflix/servo/monitor/TimedInterface.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,16 @@
*/
package com.netflix.servo.monitor;

import com.netflix.servo.jsr166e.ConcurrentHashMapV8;
import com.netflix.servo.tag.BasicTagList;
import com.netflix.servo.tag.TagList;

import java.lang.reflect.InvocationHandler;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

/**
* This class creates a {@link java.lang.reflect.Proxy} monitor that tracks all calls to methods
Expand Down Expand Up @@ -51,19 +52,21 @@ public final class TimedInterface {
static final String CLASS_TAG = "class";
static final String ID_TAG = "id";
private static class TimedHandler<T> implements InvocationHandler, CompositeMonitor<Long> {
final T concrete;
private final T concrete;
private final Map<String, Timer> timers;
private final MonitorConfig baseConfig;
private final TagList baseTagList;

/**
* {@inheritDoc}
*/
@Override
@SuppressWarnings("unchecked")
public List<Monitor<?>> getMonitors() {
List dynamicTimers = new ArrayList();
List<Monitor<?>> dynamicTimers = new ArrayList<Monitor<?>>();
for (Timer timer : timers.values()) {
dynamicTimers.add(timer);
}
return (List<Monitor<?>>) dynamicTimers;
return dynamicTimers;
}

@Override
Expand All @@ -84,10 +87,6 @@ public MonitorConfig getConfig() {
return baseConfig;
}

final ConcurrentHashMapV8<String, Timer> timers;
final MonitorConfig baseConfig;
final TagList baseTagList;

TimedHandler(Class<T> ctype, T concrete, String id) {
this.concrete = concrete;
BasicTagList tagList = BasicTagList.of(
Expand All @@ -99,7 +98,14 @@ public MonitorConfig getConfig() {
baseTagList = tagList;
baseConfig = MonitorConfig.builder(TIMED_INTERFACE).withTags(baseTagList).build();

timers = new ConcurrentHashMapV8<String, Timer>();
timers = new HashMap<String, Timer>();
for (Method method : ctype.getMethods()) {
final MonitorConfig config =
MonitorConfig.builder(method.getName())
.withTags(baseTagList)
.build();
timers.put(method.getName(), new BasicTimer(config));
}
}


Expand All @@ -108,18 +114,10 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
// if the method is one of the CompositeMonitor interface
final Class<?> declaringClass = method.getDeclaringClass();
if (declaringClass.isAssignableFrom(CompositeMonitor.class)) {
return method.invoke(this);
return method.invoke(this, args);
}
final String methodName = method.getName();
final Timer timer = timers.computeIfAbsent(methodName, new ConcurrentHashMapV8.Fun<String, Timer>() {
@Override
public Timer apply(String method) {
final MonitorConfig config = MonitorConfig.builder(method)
.withTags(baseTagList)
.build();
return new BasicTimer(config);
}
});
final Timer timer = timers.get(methodName);
final Stopwatch stopwatch = timer.start();
try {
return method.invoke(concrete, args);
Expand All @@ -139,8 +137,8 @@ private TimedInterface() {
*/
@SuppressWarnings("unchecked")
public static <T> T newProxy(Class<T> ctype, T concrete, String id) {
final InvocationHandler handler = new TimedHandler(ctype, concrete, id);
final Class[] types = new Class[] {ctype, CompositeMonitor.class};
final InvocationHandler handler = new TimedHandler<T>(ctype, concrete, id);
final Class<?>[] types = new Class[] {ctype, CompositeMonitor.class};
return (T) Proxy.newProxyInstance(ctype.getClassLoader(), types, handler);
}

Expand Down
47 changes: 40 additions & 7 deletions servo-core/src/test/java/com/netflix/servo/monitor/TimedInterfaceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@
*/
package com.netflix.servo.monitor;

import com.google.common.collect.Lists;
import com.netflix.servo.DefaultMonitorRegistry;
import com.netflix.servo.tag.BasicTagList;
import com.netflix.servo.tag.TagList;

import org.testng.annotations.Test;

import java.util.List;
Expand All @@ -34,6 +36,10 @@ private interface IDummy {
boolean method2(int n);
Object method3(Object a, Object b);
}

private interface IDummyExtended extends IDummy {
void method4();
}

private static class DummyImpl implements IDummy {
private void sleep(int ms) {
Expand Down Expand Up @@ -61,11 +67,24 @@ public Object method3(Object a, Object b) {
return a;
}
}

private static class ExtendedDummy extends DummyImpl implements IDummyExtended {

@Override
public void method4() {
// do nothing
}

}

@SuppressWarnings("unchecked")
@Test
public void testTimedInterface() {
final IDummy dummy = TimedInterface.newProxy(IDummy.class, new DummyImpl(), "id");
final CompositeMonitor<Long> compositeMonitor = (CompositeMonitor<Long>) dummy;
final List<Monitor<?>> monitors = compositeMonitor.getMonitors();
assertEquals(monitors.size(), 3);
assertEquals(compositeMonitor.getValue().longValue(), 3L);

// you'd register the CompositeMonitor as:
DefaultMonitorRegistry.getInstance().register((CompositeMonitor) dummy);
Expand All @@ -77,11 +96,6 @@ public void testTimedInterface() {
}
}

final CompositeMonitor<Long> compositeMonitor = (CompositeMonitor<Long>) dummy;
final List<Monitor<?>> monitors = compositeMonitor.getMonitors();
assertEquals(monitors.size(), 2);
assertEquals(compositeMonitor.getValue().longValue(), 2L);

final TagList tagList = BasicTagList.of(
TimedInterface.CLASS_TAG, "DummyImpl",
TimedInterface.INTERFACE_TAG, "IDummy",
Expand All @@ -101,12 +115,15 @@ public void testTimedInterface() {
// slow machines
long value = ((Monitor<Long>) monitor).getValue() - 20;
assertTrue(value >= 0 && value <= 9);
} else {
assertEquals(method, "method2");
} else if (method.equals("method2")) {
// expected result is 40, but let's give it a fudge factor to account for
// slow machines
long value = ((Monitor<Long>) monitor).getValue() - 40;
assertTrue(value >= 0 && value <= 9);
} else {
assertEquals(method, "method3");
long value = ((Monitor<Long>) monitor).getValue();
assertEquals(value, 0);
}
}
}
Expand All @@ -127,4 +144,20 @@ public void testTimedInterfaceNoId() {
.withTags(tagList).build();
assertEquals(compositeMonitor.getConfig(), expectedConfig);
}

@SuppressWarnings("unchecked")
@Test
public void testInterfaceInheritence() {
final List<String> expectedNames = Lists.newArrayList("method1", "method2", "method3", "method4");
final IDummyExtended extendedDummy = TimedInterface.newProxy(IDummyExtended.class, new ExtendedDummy());
final CompositeMonitor<Long> compositeMonitor = (CompositeMonitor<Long>) extendedDummy;
DefaultMonitorRegistry.getInstance().register(compositeMonitor);
assertEquals(compositeMonitor.getMonitors().size(), 4);
assertEquals(compositeMonitor.getValue().longValue(), 4L);
assertEquals(compositeMonitor.getValue(100).longValue(), 4L);
final List<Monitor<?>> monitors = compositeMonitor.getMonitors();
for (Monitor<?> monitor : monitors) {
assertTrue(expectedNames.contains(monitor.getConfig().getName()));
}
}
}

0 comments on commit 2f71d3c

Please sign in to comment.