Skip to content

Commit

Permalink
switch to no op monitor registry by default (#466)
Browse files Browse the repository at this point in the history
Internally Servo is deprecated and delegates to Spectator.
This change would reduce the overhead for things that are
still using the Servo APIs. The main benefit is that the
data would no longer be exported to JMX.

Note, this could be quite disruptive to anyone else using
Servo. Since it is all set up via system properties, there
isn't a convenient way to only change the default we use
internally.

After making this change, I noticed that loading the JMX
monitor registry explicitly using the property no longer
worked. That has been fixed so the old behavior can be
achieved using a system property.
  • Loading branch information
brharrington committed Apr 13, 2020
1 parent 6480bc4 commit 0a3c21d
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 23 deletions.
Expand Up @@ -29,9 +29,9 @@
* {@code com.netflix.servo.DefaultMonitorRegistry.registryClass} property. The
* specified registry class must have a constructor with no arguments. If the
* property is not specified or the class cannot be loaded an instance of
* {@link com.netflix.servo.jmx.JmxMonitorRegistry} will be used.
* {@link com.netflix.servo.NoopMonitorRegistry} will be used.
* <p/>
* If the default {@link com.netflix.servo.jmx.JmxMonitorRegistry} is used, the property
* If using {@link com.netflix.servo.jmx.JmxMonitorRegistry}, the property
* {@code com.netflix.servo.DefaultMonitorRegistry.jmxMapperClass} can optionally be
* specified to control how monitors are mapped to JMX {@link javax.management.ObjectName}.
* This property specifies the {@link com.netflix.servo.jmx.ObjectNameMapper}
Expand Down Expand Up @@ -70,24 +70,27 @@ public static MonitorRegistry getInstance() {
*/
DefaultMonitorRegistry(Properties props) {
final String className = props.getProperty(REGISTRY_CLASS_PROP);
final String registryName = props.getProperty(REGISTRY_NAME_PROP, DEFAULT_REGISTRY_NAME);
if (className != null) {
MonitorRegistry r;
try {
Class<?> c = Class.forName(className);
r = (MonitorRegistry) c.newInstance();
} catch (Throwable t) {
LOG.error(
"failed to create instance of class " + className + ", "
+ "using default class "
+ JmxMonitorRegistry.class.getName(),
t);
r = new JmxMonitorRegistry(registryName);
if (className.equals(JmxMonitorRegistry.class.getName())) {
final String registryName = props.getProperty(REGISTRY_NAME_PROP, DEFAULT_REGISTRY_NAME);
r = new JmxMonitorRegistry(registryName, getObjectNameMapper(props));
} else {
try {
Class<?> c = Class.forName(className);
r = (MonitorRegistry) c.newInstance();
} catch (Throwable t) {
LOG.error(
"failed to create instance of class " + className + ", "
+ "using default class "
+ NoopMonitorRegistry.class.getName(),
t);
r = new NoopMonitorRegistry();
}
}
registry = r;
} else {
registry = new JmxMonitorRegistry(registryName,
getObjectNameMapper(props));
registry = new NoopMonitorRegistry();
}
}

Expand Down
@@ -0,0 +1,49 @@
/*
* Copyright 2020 Netflix, Inc.
* <p/>
* 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
* <p/>
* http://www.apache.org/licenses/LICENSE-2.0
* <p/>
* 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.netflix.servo;

import com.netflix.servo.monitor.Monitor;

import java.util.Collection;
import java.util.Collections;

/**
* Monitor registry implementation that does as little as possible.
*/
public class NoopMonitorRegistry implements MonitorRegistry {

/** Create a new instance. */
public NoopMonitorRegistry() {
}

@Override
public Collection<Monitor<?>> getRegisteredMonitors() {
return Collections.emptyList();
}

@Override
public void register(Monitor<?> monitor) {
}

@Override
public void unregister(Monitor<?> monitor) {
}

@Override
public boolean isRegistered(Monitor<?> monitor) {
return false;
}
}
Expand Up @@ -32,6 +32,8 @@ public class DefaultMonitorRegistryTest {
@Test
public void testCustomJmxObjectMapper() {
Properties props = new Properties();
props.put("com.netflix.servo.DefaultMonitorRegistry.registryClass",
"com.netflix.servo.jmx.JmxMonitorRegistry");
props.put("com.netflix.servo.DefaultMonitorRegistry.jmxMapperClass",
"com.netflix.servo.DefaultMonitorRegistryTest$ChangeDomainMapper");
DefaultMonitorRegistry registry = new DefaultMonitorRegistry(props);
Expand All @@ -47,6 +49,8 @@ public void testCustomJmxObjectMapper() {
@Test
public void testInvalidMapperDefaults() {
Properties props = new Properties();
props.put("com.netflix.servo.DefaultMonitorRegistry.registryClass",
"com.netflix.servo.jmx.JmxMonitorRegistry");
props.put("com.netflix.servo.DefaultMonitorRegistry.jmxMapperClass",
"com.my.invalid.class");
DefaultMonitorRegistry registry = new DefaultMonitorRegistry(props);
Expand Down
Expand Up @@ -98,14 +98,6 @@ public void testNewObjectMonitorWithParentClass() throws Exception {
assertEquals(monitors.size(), 10);
}

@Test
public void testIsRegistered() throws Exception {
ClassWithMonitors obj = new ClassWithMonitors();
assertFalse(Monitors.isObjectRegistered("id", obj));
Monitors.registerObject("id", obj);
assertTrue(Monitors.isObjectRegistered("id", obj));
}

@Test
public void testNewObjectConfig() throws Exception {
ClassWithMonitors obj = new ClassWithMonitors() {
Expand Down

0 comments on commit 0a3c21d

Please sign in to comment.