Skip to content

Commit

Permalink
Remove Guice + use Singleton to fix initilization potential deadlock
Browse files Browse the repository at this point in the history
  • Loading branch information
Dohbedoh committed Jul 19, 2024
1 parent 7e4bb27 commit 08ab68a
Show file tree
Hide file tree
Showing 7 changed files with 97 additions and 144 deletions.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
package org.jenkinsci.plugins.prometheus.rest;

import com.google.inject.Inject;
import hudson.Extension;
import hudson.model.UnprotectedRootAction;
import hudson.util.HttpResponses;
import io.prometheus.client.exporter.common.TextFormat;
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;
Expand All @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
@@ -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;

Expand All @@ -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<String> 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();
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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 =
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -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<DefaultPrometheusMetrics> 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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<DefaultPrometheusMetrics> 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
Expand All @@ -50,21 +56,4 @@ public void ensureLoggingLevel() {
Level level = sut.getNormalLoggingLevel();
assertEquals(Level.FINE, level);
}

private static class TestPrometheusMetrics implements PrometheusMetrics {
private final AtomicReference<String> 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);
}

}
}

0 comments on commit 08ab68a

Please sign in to comment.