From a5bbc5c80de2ed35aad8e7b51b31a89dc26de107 Mon Sep 17 00:00:00 2001 From: jplumb Date: Fri, 18 Sep 2020 13:42:09 -0700 Subject: [PATCH 1/2] pull up static coupling to ServiceFactory (campsite) --- .../internal/IntrospectorServiceManager.java | 31 ++++++++++++++++--- .../com/newrelic/agent/jmx/JmxService.java | 9 ++---- .../agent/service/ServiceManagerImpl.java | 7 ++++- .../newrelic/agent/MockServiceManager.java | 4 +-- 4 files changed, 35 insertions(+), 16 deletions(-) diff --git a/instrumentation-test/src/main/java/com/newrelic/agent/introspec/internal/IntrospectorServiceManager.java b/instrumentation-test/src/main/java/com/newrelic/agent/introspec/internal/IntrospectorServiceManager.java index 4b53791929..6427c0f754 100644 --- a/instrumentation-test/src/main/java/com/newrelic/agent/introspec/internal/IntrospectorServiceManager.java +++ b/instrumentation-test/src/main/java/com/newrelic/agent/introspec/internal/IntrospectorServiceManager.java @@ -7,13 +7,23 @@ package com.newrelic.agent.introspec.internal; -import com.newrelic.agent.*; +import com.newrelic.agent.ExpirationService; +import com.newrelic.agent.HarvestService; +import com.newrelic.agent.RPMServiceManager; +import com.newrelic.agent.ThreadService; +import com.newrelic.agent.TracerService; +import com.newrelic.agent.TransactionService; import com.newrelic.agent.attributes.AttributesService; import com.newrelic.agent.browser.BrowserService; import com.newrelic.agent.cache.CacheService; import com.newrelic.agent.circuitbreaker.CircuitBreakerService; import com.newrelic.agent.commands.CommandParser; -import com.newrelic.agent.config.*; +import com.newrelic.agent.config.AgentConfig; +import com.newrelic.agent.config.AgentConfigFactory; +import com.newrelic.agent.config.AgentConfigImpl; +import com.newrelic.agent.config.ConfigService; +import com.newrelic.agent.config.ConfigServiceFactory; +import com.newrelic.agent.config.DistributedTracingConfig; import com.newrelic.agent.core.CoreService; import com.newrelic.agent.database.DatabaseService; import com.newrelic.agent.environment.EnvironmentService; @@ -37,7 +47,13 @@ import com.newrelic.agent.service.Service; import com.newrelic.agent.service.ServiceFactory; import com.newrelic.agent.service.ServiceManager; -import com.newrelic.agent.service.analytics.*; +import com.newrelic.agent.service.analytics.CollectorSpanEventReservoirManager; +import com.newrelic.agent.service.analytics.CollectorSpanEventSender; +import com.newrelic.agent.service.analytics.InsightsService; +import com.newrelic.agent.service.analytics.SpanEventCreationDecider; +import com.newrelic.agent.service.analytics.SpanEventsService; +import com.newrelic.agent.service.analytics.TransactionDataToDistributedTraceIntrinsics; +import com.newrelic.agent.service.analytics.TransactionEventsService; import com.newrelic.agent.service.async.AsyncTransactionService; import com.newrelic.agent.service.module.JarCollectorService; import com.newrelic.agent.sql.SqlTraceService; @@ -48,7 +64,11 @@ import com.newrelic.agent.tracing.DistributedTraceServiceImpl; import com.newrelic.agent.utilization.UtilizationService; -import java.util.*; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; class IntrospectorServiceManager extends AbstractService implements ServiceManager { @@ -154,7 +174,8 @@ private void setup(Map config) { remoteInstrumentationService = new RemoteInstrumentationServiceImpl(); sourceLanguageService = new SourceLanguageService(); classTransformerService = new NoOpClassTransformerService(); - jmxService = new JmxService(); + boolean jmxEnabled = configService.getDefaultAgentConfig().getJmxConfig().isEnabled(); + jmxService = new JmxService(jmxEnabled); attributesService = new AttributesService(); circuitBreakerService = new CircuitBreakerService(); AgentConfig agentConfig = createAgentConfig(config, (Map) config.get("distributed_tracing")); diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/jmx/JmxService.java b/newrelic-agent/src/main/java/com/newrelic/agent/jmx/JmxService.java index d4277e0668..d52548f367 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/jmx/JmxService.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/jmx/JmxService.java @@ -10,8 +10,6 @@ import com.google.common.annotations.VisibleForTesting; import com.newrelic.agent.Agent; import com.newrelic.agent.HarvestListener; -import com.newrelic.agent.config.AgentConfig; -import com.newrelic.agent.config.JmxConfig; import com.newrelic.agent.extension.Extension; import com.newrelic.agent.jmx.create.JmxGet; import com.newrelic.agent.jmx.create.JmxInvoke; @@ -21,7 +19,6 @@ import com.newrelic.agent.service.ServiceFactory; import com.newrelic.agent.stats.StatsEngine; - import javax.management.Attribute; import javax.management.AttributeNotFoundException; import javax.management.MBeanServer; @@ -76,11 +73,9 @@ public class JmxService extends AbstractService implements HarvestListener { */ private final Set toRemoveMBeanServers = new CopyOnWriteArraySet<>(); - public JmxService() { + public JmxService(boolean enabled) { super(JmxService.class.getSimpleName()); - AgentConfig config = ServiceFactory.getConfigService().getDefaultAgentConfig(); - JmxConfig jmxConfig = config.getJmxConfig(); - enabled = jmxConfig.isEnabled(); + this.enabled = enabled; jmxMetricFactory = JmxObjectFactory.createJmxFactory(); } diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/service/ServiceManagerImpl.java b/newrelic-agent/src/main/java/com/newrelic/agent/service/ServiceManagerImpl.java index 6f9bf7d3bd..e6bbd7a4fd 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/service/ServiceManagerImpl.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/service/ServiceManagerImpl.java @@ -27,7 +27,9 @@ import com.newrelic.agent.cache.CacheService; import com.newrelic.agent.circuitbreaker.CircuitBreakerService; import com.newrelic.agent.commands.CommandParser; +import com.newrelic.agent.config.AgentConfig; import com.newrelic.agent.config.ConfigService; +import com.newrelic.agent.config.JmxConfig; import com.newrelic.agent.core.CoreService; import com.newrelic.agent.database.DatabaseService; import com.newrelic.agent.deadlock.DeadlockDetectorService; @@ -159,7 +161,10 @@ protected synchronized void doStart() throws Exception { threadService = new ThreadService(); circuitBreakerService = new CircuitBreakerService(); classTransformerService = new ClassTransformerServiceImpl(coreService.getInstrumentation()); - jmxService = new JmxService(); + + AgentConfig config = configService.getDefaultAgentConfig(); + JmxConfig jmxConfig = config.getJmxConfig(); + jmxService = new JmxService(jmxConfig.isEnabled()); Logger jarCollectorLogger = Agent.LOG.getChildLogger("com.newrelic.jar_collector"); boolean jarCollectorEnabled = configService.getDefaultAgentConfig().getJarCollectorConfig().isEnabled(); diff --git a/newrelic-agent/src/test/java/com/newrelic/agent/MockServiceManager.java b/newrelic-agent/src/test/java/com/newrelic/agent/MockServiceManager.java index a82383d6a2..636bc54805 100644 --- a/newrelic-agent/src/test/java/com/newrelic/agent/MockServiceManager.java +++ b/newrelic-agent/src/test/java/com/newrelic/agent/MockServiceManager.java @@ -124,8 +124,6 @@ public MockServiceManager(ConfigService configService) { Mockito.when(statsService.getMetricAggregator()).thenReturn(metricAggregator); harvestService = Mockito.mock(HarvestService.class); sqlTraceService = Mockito.mock(SqlTraceService.class); - // browserService; - // cacheService; dbService = new DatabaseService(); extensionService = new ExtensionService(configService, ExtensionsLoadedListener.NOOP); jarCollectorService = Mockito.mock(JarCollectorService.class); @@ -142,7 +140,7 @@ public MockServiceManager(ConfigService configService) { commandParser = new CommandParser(); remoteInstrumentationService = Mockito.mock(RemoteInstrumentationService.class); classTransformerService = Mockito.mock(ClassTransformerService.class); - jmxService = new JmxService(); + jmxService = new JmxService(true); circuitBreakerService = new CircuitBreakerService(); spanEventsService = Mockito.mock(SpanEventsService.class); insights = Mockito.mock(InsightsServiceImpl.class); From d3a94e57e7d2961b2ab31af8630a47f625d581f6 Mon Sep 17 00:00:00 2001 From: jplumb Date: Fri, 18 Sep 2020 14:28:05 -0700 Subject: [PATCH 2/2] move LinkingMetadataMBean registration to JMXService --- .../main/java/com/newrelic/agent/Agent.java | 8 ---- .../com/newrelic/agent/jmx/JmxService.java | 6 +++ .../jmx/LinkingMetadataRegistration.java | 2 +- .../jmx/LinkingMetadataRegistrationTest.java | 37 ++++++++++++++++--- 4 files changed, 38 insertions(+), 15 deletions(-) diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/Agent.java b/newrelic-agent/src/main/java/com/newrelic/agent/Agent.java index 6cb1a18c61..350392531f 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/Agent.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/Agent.java @@ -16,7 +16,6 @@ import com.newrelic.agent.config.JavaVersionUtils; import com.newrelic.agent.core.CoreService; import com.newrelic.agent.core.CoreServiceImpl; -import com.newrelic.agent.jmx.LinkingMetadataRegistration; import com.newrelic.agent.logging.AgentLogManager; import com.newrelic.agent.logging.IAgentLogger; import com.newrelic.agent.service.ServiceFactory; @@ -167,8 +166,6 @@ public static void continuePremain(String agentArgs, Instrumentation inst, long logAnyFilesFoundInEndorsedDirs(); - registerAgentMBeans(); - if (serviceManager.getConfigService().getDefaultAgentConfig().isStartupTimingEnabled()) { recordPremainTime(serviceManager.getStatsService(), startTime); } @@ -201,11 +198,6 @@ public static void continuePremain(String agentArgs, Instrumentation inst, long } } - private static void registerAgentMBeans() { - // This registers the mbean that exposes linking metadata - new LinkingMetadataRegistration(LOG).registerLinkingMetadata(); - } - private static boolean tryToInitializeServiceManager(Instrumentation inst) { try { CoreService coreService = new CoreServiceImpl(inst); diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/jmx/JmxService.java b/newrelic-agent/src/main/java/com/newrelic/agent/jmx/JmxService.java index d52548f367..e1ee22d424 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/jmx/JmxService.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/jmx/JmxService.java @@ -92,6 +92,7 @@ public void addJmxAttributeProcessor(JmxAttributeProcessor attributeProcessor) { @Override protected void doStart() { if (enabled) { + registerAgentMBeans(); jmxMetricFactory.getStartUpJmxObjects(jmxGets, jmxInvokes); if (jmxGets.size() > 0) { ServiceFactory.getHarvestService().addHarvestListener(this); @@ -415,6 +416,11 @@ public void reloadExtensions(Set oldExtensions, Set extens } } + private void registerAgentMBeans() { + // This registers the mbean that exposes linking metadata + new LinkingMetadataRegistration(Agent.LOG).registerLinkingMetadata(); + } + /* * These are examples of the built in metric names. * diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/jmx/LinkingMetadataRegistration.java b/newrelic-agent/src/main/java/com/newrelic/agent/jmx/LinkingMetadataRegistration.java index f55f4e7097..498eb02ffa 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/jmx/LinkingMetadataRegistration.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/jmx/LinkingMetadataRegistration.java @@ -25,7 +25,7 @@ public void registerLinkingMetadata() { Object bean = new LinkingMetadata(); server.registerMBean(bean, name); logger.log(Level.INFO, "JMX LinkingMetadata bean registered"); - } catch (Exception e) { + } catch (Exception | NoClassDefFoundError e) { logger.log(Level.INFO, "Error registering JMX LinkingMetadata MBean", e); } } diff --git a/newrelic-agent/src/test/java/com/newrelic/agent/jmx/LinkingMetadataRegistrationTest.java b/newrelic-agent/src/test/java/com/newrelic/agent/jmx/LinkingMetadataRegistrationTest.java index 98b8ab456a..da6a27f273 100644 --- a/newrelic-agent/src/test/java/com/newrelic/agent/jmx/LinkingMetadataRegistrationTest.java +++ b/newrelic-agent/src/test/java/com/newrelic/agent/jmx/LinkingMetadataRegistrationTest.java @@ -1,6 +1,7 @@ package com.newrelic.agent.jmx; import com.newrelic.api.agent.Logger; +import org.junit.Before; import org.junit.Test; import javax.management.MBeanServer; @@ -16,11 +17,19 @@ public class LinkingMetadataRegistrationTest { + ObjectName name; + Logger logger; + MBeanServer fakeServer; + + @Before + public void setup() throws Exception { + name = new ObjectName(MBEAN_NAME); + logger = mock(Logger.class); + fakeServer = mock(MBeanServer.class); + } + @Test public void testRegisterHappyPath() throws Exception { - ObjectName name = new ObjectName(MBEAN_NAME); - Logger logger = mock(Logger.class); - final MBeanServer fakeServer = mock(MBeanServer.class); LinkingMetadataRegistration testClass = new LinkingMetadataRegistration(logger) { @Override @@ -35,11 +44,27 @@ protected MBeanServer getMbeanServer() { @Test public void testExceptionsAreHandled() throws Exception { - ObjectName name = new ObjectName(MBEAN_NAME); RuntimeException exception = new RuntimeException("Bad beans exception"); - Logger logger = mock(Logger.class); - final MBeanServer fakeServer = mock(MBeanServer.class); + when(fakeServer.registerMBean(isA(LinkingMetadataMBean.class), eq(name))).thenThrow(exception); + + LinkingMetadataRegistration testClass = new LinkingMetadataRegistration(logger) { + @Override + protected MBeanServer getMbeanServer() { + return fakeServer; + } + }; + + testClass.registerLinkingMetadata(); + //success + verify(logger).log(eq(Level.INFO), eq("JMX LinkingMetadata started, registering MBean: com.newrelic.jfr:type=LinkingMetadata")); + verify(logger).log(eq(Level.INFO), eq("Error registering JMX LinkingMetadata MBean"), eq(exception)); + } + + @Test + public void testNoClassDefErrorAlsoHandled() throws Exception { + NoClassDefFoundError exception = new NoClassDefFoundError("I am broken."); + when(fakeServer.registerMBean(isA(LinkingMetadataMBean.class), eq(name))).thenThrow(exception); LinkingMetadataRegistration testClass = new LinkingMetadataRegistration(logger) {