From 0a3c21dc7b49c8ae7ed9549e23cc09cc470e95d0 Mon Sep 17 00:00:00 2001 From: brharrington Date: Mon, 13 Apr 2020 11:58:32 -0500 Subject: [PATCH] switch to no op monitor registry by default (#466) 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. --- .../netflix/servo/DefaultMonitorRegistry.java | 33 +++++++------ .../netflix/servo/NoopMonitorRegistry.java | 49 +++++++++++++++++++ .../servo/DefaultMonitorRegistryTest.java | 4 ++ .../netflix/servo/monitor/MonitorsTest.java | 8 --- 4 files changed, 71 insertions(+), 23 deletions(-) create mode 100644 servo-core/src/main/java/com/netflix/servo/NoopMonitorRegistry.java diff --git a/servo-core/src/main/java/com/netflix/servo/DefaultMonitorRegistry.java b/servo-core/src/main/java/com/netflix/servo/DefaultMonitorRegistry.java index e1f06be0..24b26d7f 100644 --- a/servo-core/src/main/java/com/netflix/servo/DefaultMonitorRegistry.java +++ b/servo-core/src/main/java/com/netflix/servo/DefaultMonitorRegistry.java @@ -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. *

- * 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} @@ -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(); } } diff --git a/servo-core/src/main/java/com/netflix/servo/NoopMonitorRegistry.java b/servo-core/src/main/java/com/netflix/servo/NoopMonitorRegistry.java new file mode 100644 index 00000000..5fed5d89 --- /dev/null +++ b/servo-core/src/main/java/com/netflix/servo/NoopMonitorRegistry.java @@ -0,0 +1,49 @@ +/* + * Copyright 2020 Netflix, Inc. + *

+ * 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.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> getRegisteredMonitors() { + return Collections.emptyList(); + } + + @Override + public void register(Monitor monitor) { + } + + @Override + public void unregister(Monitor monitor) { + } + + @Override + public boolean isRegistered(Monitor monitor) { + return false; + } +} diff --git a/servo-core/src/test/java/com/netflix/servo/DefaultMonitorRegistryTest.java b/servo-core/src/test/java/com/netflix/servo/DefaultMonitorRegistryTest.java index 021fd50b..e070de5c 100644 --- a/servo-core/src/test/java/com/netflix/servo/DefaultMonitorRegistryTest.java +++ b/servo-core/src/test/java/com/netflix/servo/DefaultMonitorRegistryTest.java @@ -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); @@ -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); diff --git a/servo-core/src/test/java/com/netflix/servo/monitor/MonitorsTest.java b/servo-core/src/test/java/com/netflix/servo/monitor/MonitorsTest.java index a7ae9846..8d62fff3 100644 --- a/servo-core/src/test/java/com/netflix/servo/monitor/MonitorsTest.java +++ b/servo-core/src/test/java/com/netflix/servo/monitor/MonitorsTest.java @@ -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() {