Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Guice + use Singleton to fix initilization potential deadlock #682

Merged
merged 1 commit into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

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);
}

}
}
Loading