From 08ab68a30de258ed49a0a54243dca35182f84bff Mon Sep 17 00:00:00 2001 From: Allan Burdajewicz Date: Fri, 19 Jul 2024 17:16:57 +1000 Subject: [PATCH] Remove Guice + use Singleton to fix initilization potential deadlock --- .../plugins/prometheus/context/Context.java | 16 ----- .../prometheus/rest/PrometheusAction.java | 9 +-- .../service/DefaultPrometheusMetrics.java | 59 +++++++++++++++---- .../service/PrometheusAsyncWorker.java | 8 +-- .../prometheus/context/ContextTest.java | 50 ---------------- .../prometheus/rest/PrometheusActionTest.java | 48 ++++++++------- .../service/PrometheusAsyncWorkerTest.java | 51 +++++++--------- 7 files changed, 97 insertions(+), 144 deletions(-) delete mode 100644 src/main/java/org/jenkinsci/plugins/prometheus/context/Context.java delete mode 100644 src/test/java/org/jenkinsci/plugins/prometheus/context/ContextTest.java diff --git a/src/main/java/org/jenkinsci/plugins/prometheus/context/Context.java b/src/main/java/org/jenkinsci/plugins/prometheus/context/Context.java deleted file mode 100644 index 5d51634f9..000000000 --- a/src/main/java/org/jenkinsci/plugins/prometheus/context/Context.java +++ /dev/null @@ -1,16 +0,0 @@ -package org.jenkinsci.plugins.prometheus.context; - -import com.google.inject.AbstractModule; -import hudson.Extension; -import org.jenkinsci.plugins.prometheus.service.DefaultPrometheusMetrics; -import org.jenkinsci.plugins.prometheus.service.PrometheusMetrics; - -@Extension -public class Context extends AbstractModule { - - @Override - public void configure() { - bind(PrometheusMetrics.class).to(DefaultPrometheusMetrics.class).in(com.google.inject.Singleton.class); - } - -} diff --git a/src/main/java/org/jenkinsci/plugins/prometheus/rest/PrometheusAction.java b/src/main/java/org/jenkinsci/plugins/prometheus/rest/PrometheusAction.java index 5e8f38d10..a0ea00bda 100644 --- a/src/main/java/org/jenkinsci/plugins/prometheus/rest/PrometheusAction.java +++ b/src/main/java/org/jenkinsci/plugins/prometheus/rest/PrometheusAction.java @@ -1,6 +1,5 @@ package org.jenkinsci.plugins.prometheus.rest; -import com.google.inject.Inject; import hudson.Extension; import hudson.model.UnprotectedRootAction; import hudson.util.HttpResponses; @@ -8,6 +7,7 @@ import jenkins.metrics.api.Metrics; import jenkins.model.Jenkins; import org.jenkinsci.plugins.prometheus.config.PrometheusConfiguration; +import org.jenkinsci.plugins.prometheus.service.DefaultPrometheusMetrics; import org.jenkinsci.plugins.prometheus.service.PrometheusMetrics; import org.kohsuke.stapler.HttpResponse; import org.kohsuke.stapler.StaplerRequest; @@ -16,12 +16,7 @@ @Extension public class PrometheusAction implements UnprotectedRootAction { - private PrometheusMetrics prometheusMetrics; - - @Inject - public void setPrometheusMetrics(PrometheusMetrics prometheusMetrics) { - this.prometheusMetrics = prometheusMetrics; - } + private final PrometheusMetrics prometheusMetrics = DefaultPrometheusMetrics.get(); @Override public String getIconFileName() { diff --git a/src/main/java/org/jenkinsci/plugins/prometheus/service/DefaultPrometheusMetrics.java b/src/main/java/org/jenkinsci/plugins/prometheus/service/DefaultPrometheusMetrics.java index 9024c9c3d..1cf59d43a 100644 --- a/src/main/java/org/jenkinsci/plugins/prometheus/service/DefaultPrometheusMetrics.java +++ b/src/main/java/org/jenkinsci/plugins/prometheus/service/DefaultPrometheusMetrics.java @@ -1,15 +1,24 @@ package org.jenkinsci.plugins.prometheus.service; +import edu.umd.cs.findbugs.annotations.NonNull; import hudson.ExtensionList; +import hudson.init.InitMilestone; +import hudson.init.Initializer; import io.prometheus.client.Collector; import io.prometheus.client.CollectorRegistry; import io.prometheus.client.dropwizard.DropwizardExports; import io.prometheus.client.exporter.common.TextFormat; import io.prometheus.client.hotspot.DefaultExports; import jenkins.metrics.api.Metrics; -import org.jenkinsci.plugins.prometheus.*; +import org.jenkinsci.plugins.prometheus.CodeCoverageCollector; +import org.jenkinsci.plugins.prometheus.DiskUsageCollector; +import org.jenkinsci.plugins.prometheus.ExecutorCollector; +import org.jenkinsci.plugins.prometheus.JenkinsStatusCollector; +import org.jenkinsci.plugins.prometheus.JobCollector; import org.jenkinsci.plugins.prometheus.config.disabledmetrics.FilteredMetricEnumeration; import org.jenkinsci.plugins.prometheus.util.JenkinsNodeBuildsSampleBuilder; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -21,27 +30,51 @@ public class DefaultPrometheusMetrics implements PrometheusMetrics { private static final Logger logger = LoggerFactory.getLogger(DefaultPrometheusMetrics.class); + private static DefaultPrometheusMetrics INSTANCE = null; + private final CollectorRegistry collectorRegistry; private final AtomicReference cachedMetrics; - public DefaultPrometheusMetrics() { + private DefaultPrometheusMetrics() { CollectorRegistry collectorRegistry = CollectorRegistry.defaultRegistry; - collectorRegistry.register(new JobCollector()); - collectorRegistry.register(new JenkinsStatusCollector()); - collectorRegistry.register(new DropwizardExports(Metrics.metricRegistry(), new JenkinsNodeBuildsSampleBuilder())); - collectorRegistry.register(new DiskUsageCollector()); - collectorRegistry.register(new ExecutorCollector()); - collectorRegistry.register(new CodeCoverageCollector()); - - // other collectors from other plugins - ExtensionList.lookup(Collector.class).forEach(collectorRegistry::register); - DefaultExports.initialize(); - this.collectorRegistry = collectorRegistry; this.cachedMetrics = new AtomicReference<>(""); } + public static synchronized DefaultPrometheusMetrics get() { + if(INSTANCE == null) { + INSTANCE = new DefaultPrometheusMetrics(); + } + return INSTANCE; + } + + @Restricted(NoExternalUse.class) + private void registerCollector(@NonNull Collector collector) { + collectorRegistry.register(collector); + logger.debug(String.format("Collector %s registered", collector.getClass().getName())); + } + + @Restricted(NoExternalUse.class) + @Initializer(after = InitMilestone.EXTENSIONS_AUGMENTED, before = InitMilestone.JOB_LOADED) + public static void registerCollectors() { + DefaultPrometheusMetrics instance = get(); + instance.registerCollector(new JenkinsStatusCollector()); + instance.registerCollector(new DropwizardExports(Metrics.metricRegistry(), new JenkinsNodeBuildsSampleBuilder())); + instance.registerCollector(new DiskUsageCollector()); + instance.registerCollector(new ExecutorCollector()); + } + + @Restricted(NoExternalUse.class) + @Initializer(after = InitMilestone.JOB_LOADED, before = InitMilestone.JOB_CONFIG_ADAPTED) + public static void registerJobCollectors() { + DefaultPrometheusMetrics instance = get(); + instance.registerCollector(new JobCollector()); + instance.registerCollector(new CodeCoverageCollector()); + // other collectors from other plugins + ExtensionList.lookup(Collector.class).forEach(instance::registerCollector); + } + @Override public String getMetrics() { return cachedMetrics.get(); diff --git a/src/main/java/org/jenkinsci/plugins/prometheus/service/PrometheusAsyncWorker.java b/src/main/java/org/jenkinsci/plugins/prometheus/service/PrometheusAsyncWorker.java index 295517108..bbf7d295c 100644 --- a/src/main/java/org/jenkinsci/plugins/prometheus/service/PrometheusAsyncWorker.java +++ b/src/main/java/org/jenkinsci/plugins/prometheus/service/PrometheusAsyncWorker.java @@ -1,6 +1,5 @@ package org.jenkinsci.plugins.prometheus.service; -import com.google.inject.Inject; import hudson.Extension; import hudson.model.AsyncPeriodicWork; import hudson.model.TaskListener; @@ -16,17 +15,12 @@ public class PrometheusAsyncWorker extends AsyncPeriodicWork { private static final Logger logger = LoggerFactory.getLogger(PrometheusAsyncWorker.class); - private PrometheusMetrics prometheusMetrics; + private final PrometheusMetrics prometheusMetrics = DefaultPrometheusMetrics.get(); public PrometheusAsyncWorker() { super("prometheus_async_worker"); } - @Inject - public void setPrometheusMetrics(PrometheusMetrics prometheusMetrics) { - this.prometheusMetrics = prometheusMetrics; - } - @Override public long getRecurrencePeriod() { long collectingMetricsPeriodInMillis = diff --git a/src/test/java/org/jenkinsci/plugins/prometheus/context/ContextTest.java b/src/test/java/org/jenkinsci/plugins/prometheus/context/ContextTest.java deleted file mode 100644 index ea8d9358b..000000000 --- a/src/test/java/org/jenkinsci/plugins/prometheus/context/ContextTest.java +++ /dev/null @@ -1,50 +0,0 @@ -package org.jenkinsci.plugins.prometheus.context; - -import com.codahale.metrics.MetricRegistry; -import com.google.inject.Guice; -import com.google.inject.Injector; -import jenkins.metrics.api.Metrics; -import jenkins.model.Jenkins; -import org.jenkinsci.plugins.prometheus.config.PrometheusConfiguration; -import org.jenkinsci.plugins.prometheus.service.DefaultPrometheusMetrics; -import org.jenkinsci.plugins.prometheus.service.PrometheusMetrics; -import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.Test; -import org.mockito.MockedStatic; - -import static org.mockito.Mockito.*; - -public class ContextTest { - - @Test - public void testCanInjectContext() { - try (MockedStatic jenkinsStatic = mockStatic(Jenkins.class); - MockedStatic prometheusConfigurationStatic = mockStatic(PrometheusConfiguration.class); - MockedStatic metricsStatic = mockStatic(Metrics.class)) { - Jenkins mockedJenkins = mock(Jenkins.class); - jenkinsStatic.when(Jenkins::get).thenReturn(mockedJenkins); - - MetricRegistry mockedMetrics = mock(MetricRegistry.class); - metricsStatic.when(Metrics::metricRegistry).thenReturn(mockedMetrics); - - PrometheusConfiguration mockedPrometheusConfiguration = mock(PrometheusConfiguration.class); - when(mockedPrometheusConfiguration.getLabeledBuildParameterNamesAsArray()).thenReturn(new String[]{}); - when(mockedPrometheusConfiguration.getDefaultNamespace()).thenReturn("default"); - prometheusConfigurationStatic.when(PrometheusConfiguration::get).thenReturn(mockedPrometheusConfiguration); - - Injector injector = Guice.createInjector(new Context()); - PrometheusMetrics prometheusMetrics = injector.getInstance(PrometheusMetrics.class); - - Assertions.assertNotNull(prometheusMetrics); - Assertions.assertEquals(DefaultPrometheusMetrics.class, prometheusMetrics.getClass()); - - PrometheusMetrics prometheusMetrics2 = injector.getInstance(PrometheusMetrics.class); - - Assertions.assertEquals(prometheusMetrics, prometheusMetrics2, "Should be the same as it's a singleton!"); - - - } - - } - -} \ No newline at end of file diff --git a/src/test/java/org/jenkinsci/plugins/prometheus/rest/PrometheusActionTest.java b/src/test/java/org/jenkinsci/plugins/prometheus/rest/PrometheusActionTest.java index 58bc253e0..c50ce75e9 100644 --- a/src/test/java/org/jenkinsci/plugins/prometheus/rest/PrometheusActionTest.java +++ b/src/test/java/org/jenkinsci/plugins/prometheus/rest/PrometheusActionTest.java @@ -4,7 +4,7 @@ import jenkins.metrics.api.Metrics; import jenkins.model.Jenkins; import org.jenkinsci.plugins.prometheus.config.PrometheusConfiguration; -import org.jenkinsci.plugins.prometheus.service.PrometheusMetrics; +import org.jenkinsci.plugins.prometheus.service.DefaultPrometheusMetrics; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; @@ -22,8 +22,14 @@ import java.io.PrintWriter; import java.io.StringWriter; -import static java.net.HttpURLConnection.*; -import static org.mockito.Mockito.*; +import static java.net.HttpURLConnection.HTTP_FORBIDDEN; +import static java.net.HttpURLConnection.HTTP_NOT_FOUND; +import static java.net.HttpURLConnection.HTTP_OK; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) public class PrometheusActionTest { @@ -87,25 +93,27 @@ public void shouldThrowExceptionWhenAuthenticationEnabledAndInsufficientPermissi @Test public void shouldReturnMetrics() throws IOException, ServletException { // given - PrometheusAction action = new PrometheusAction(); - PrometheusMetrics prometheusMetrics = mock(PrometheusMetrics.class); + DefaultPrometheusMetrics prometheusMetrics = mock(DefaultPrometheusMetrics.class); String responseBody = "testMetric"; when(prometheusMetrics.getMetrics()).thenReturn(responseBody); - action.setPrometheusMetrics(prometheusMetrics); - StaplerRequest request = mock(StaplerRequest.class); - String url = "prometheus"; - when(request.getRestOfPath()).thenReturn(url); - - // when - HttpResponse actual = action.doDynamic(request); - - // then - AssertStaplerResponse.from(actual) - .call() - .assertHttpStatus(HTTP_OK) - .assertContentType(TextFormat.CONTENT_TYPE_004) - .assertHttpHeader("Cache-Control", "must-revalidate,no-cache,no-store") - .assertBody(responseBody); + try (MockedStatic defaultPrometheusMetricsMockedStatic = mockStatic(DefaultPrometheusMetrics.class)) { + defaultPrometheusMetricsMockedStatic.when(DefaultPrometheusMetrics::get).thenReturn(prometheusMetrics); + PrometheusAction action = new PrometheusAction(); + StaplerRequest request = mock(StaplerRequest.class); + String url = "prometheus"; + when(request.getRestOfPath()).thenReturn(url); + + // when + HttpResponse actual = action.doDynamic(request); + + // then + AssertStaplerResponse.from(actual) + .call() + .assertHttpStatus(HTTP_OK) + .assertContentType(TextFormat.CONTENT_TYPE_004) + .assertHttpHeader("Cache-Control", "must-revalidate,no-cache,no-store") + .assertBody(responseBody); + } } private static class AssertStaplerResponse { diff --git a/src/test/java/org/jenkinsci/plugins/prometheus/service/PrometheusAsyncWorkerTest.java b/src/test/java/org/jenkinsci/plugins/prometheus/service/PrometheusAsyncWorkerTest.java index 8ba367784..9fe678f9d 100644 --- a/src/test/java/org/jenkinsci/plugins/prometheus/service/PrometheusAsyncWorkerTest.java +++ b/src/test/java/org/jenkinsci/plugins/prometheus/service/PrometheusAsyncWorkerTest.java @@ -5,29 +5,35 @@ import org.jvnet.hudson.test.Issue; import org.mockito.MockedStatic; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Level; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; public class PrometheusAsyncWorkerTest { @Test public void shouldCollectMetrics() { - // given - PrometheusAsyncWorker asyncWorker = new PrometheusAsyncWorker(); - PrometheusMetrics metrics = new TestPrometheusMetrics(); - asyncWorker.setPrometheusMetrics(metrics); - - // when - asyncWorker.execute(null); - - // then - String actual = metrics.getMetrics(); - assertEquals("1", actual); + try (MockedStatic defaultPrometheusMetricsMockedStatic = mockStatic(DefaultPrometheusMetrics.class)) { + // given + DefaultPrometheusMetrics metrics = spy(DefaultPrometheusMetrics.class); + doNothing().when(metrics).collectMetrics(); + defaultPrometheusMetricsMockedStatic.when(DefaultPrometheusMetrics::get).thenReturn(metrics); + PrometheusAsyncWorker asyncWorker = new PrometheusAsyncWorker(); + + // when + asyncWorker.execute(null); + + // then + verify(metrics, times(1)).collectMetrics(); + } } @Test @@ -50,21 +56,4 @@ public void ensureLoggingLevel() { Level level = sut.getNormalLoggingLevel(); assertEquals(Level.FINE, level); } - - private static class TestPrometheusMetrics implements PrometheusMetrics { - private final AtomicReference cachedMetrics = new AtomicReference<>(""); - private final AtomicInteger counter = new AtomicInteger(0); - - @Override - public String getMetrics() { - return cachedMetrics.get(); - } - - @Override - public void collectMetrics() { - String metrics = String.valueOf(counter.incrementAndGet()); - cachedMetrics.set(metrics); - } - - } }