Skip to content
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;

import javax.inject.Inject;
Expand Down Expand Up @@ -101,6 +102,7 @@ public String toString() {
}

private static List<Item> metricsItems = new ArrayList<>();
private volatile long lastMetricsUpdateTime = 0L;

@Inject
private DataCenterDao dcDao;
Expand Down Expand Up @@ -488,7 +490,15 @@ private void addVMsBySizeMetrics(final List<Item> metricsList, final long dcId,
}

@Override
public void updateMetrics() {
public synchronized void updateMetrics() {
final long minIntervalMs = TimeUnit.SECONDS.toMillis(PrometheusExporterServer.PrometheusExporterMinRefreshInterval.value());
final long now = System.currentTimeMillis();
if (now - lastMetricsUpdateTime < minIntervalMs) {
logger.debug("Skipping metrics recomputation, last update was " + (now - lastMetricsUpdateTime) + "ms ago (min interval: " + minIntervalMs + "ms)");
return;
}

final long startNanos = System.nanoTime();
final List<Item> latestMetricsItems = new ArrayList<Item>();
try {
for (final DataCenterVO dc : dcDao.listAll()) {
Expand All @@ -508,8 +518,12 @@ public void updateMetrics() {
addDomainResourceCount(latestMetricsItems);
} catch (Exception e) {
logger.warn("Getting metrics failed ", e);
} finally {
final long elapsedMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startNanos);
logger.info("Prometheus metrics update completed in " + elapsedMs + " ms");
}
metricsItems = latestMetricsItems;
lastMetricsUpdateTime = System.currentTimeMillis();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,7 @@ public interface PrometheusExporterServer extends Manager {

ConfigKey<Integer> PrometheusExporterOfferingCountLimit = new ConfigKey<>("Advanced", Integer.class, "prometheus.exporter.offering.output.limit", "-1",
"Limit the number of output for cloudstack_vms_total_by_size to the provided value. -1 for unlimited output.", true);

ConfigKey<Integer> PrometheusExporterMinRefreshInterval = new ConfigKey<>("Advanced", Integer.class, "prometheus.exporter.metrics.min.refresh.interval", "5",
"Minimum interval in seconds between metrics recomputations. Scrapes arriving faster than this interval reuse the previously computed metrics.", true, EnablePrometheusExporter.key());
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,13 @@
import java.net.InetSocketAddress;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

public class PrometheusExporterServerImpl extends ManagerBase implements PrometheusExporterServer, Configurable {

private static HttpServer httpServer;
private ExecutorService httpExecutor;

@Inject
private PrometheusExporter prometheusExporter;
Expand Down Expand Up @@ -79,6 +82,8 @@ public boolean start() {
if (EnablePrometheusExporter.value()) {
try {
httpServer = HttpServer.create(new InetSocketAddress(PrometheusExporterServerPort.value()), 0);
httpExecutor = Executors.newFixedThreadPool(2);
httpServer.setExecutor(httpExecutor);
httpServer.createContext("/metrics", new ExporterHandler(prometheusExporter));
httpServer.createContext("/", new HttpHandler() {
@Override
Expand All @@ -105,9 +110,15 @@ public void handle(HttpExchange httpExchange) throws IOException {
@Override
public boolean stop() {
if (httpServer != null) {
httpServer.setExecutor(null);
httpServer.stop(0);
logger.debug("Stopped Prometheus exporter http server");
}
if (httpExecutor != null) {
httpExecutor.shutdownNow();
logger.debug("Shut down Prometheus exporter http executor");
}
Comment on lines +117 to +120

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should httpExecutor also be set to null on httpServer?

something like

Suggested change
if (httpExecutor != null) {
httpExecutor.shutdownNow();
logger.debug("Shut down Prometheus exporter http executor");
}
if (httpExecutor != null) {
httpExecutor.shutdownNow();
logger.debug("Shut down Prometheus exporter http executor");
}
httpServer.setExecutor(null); // ¡¡¡¡¡Did not test!!!!!

?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, I genuinely don’t understand this part of the change, I’ll read the co-pilot comment on the original issue as well. So if the executor is shutdown(), can start() still be called on it? (for instance)
And why a newFixedThreadPool of 2?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both fair points:

  • setExecutor(null): not functionally needed since start() always
    creates a fresh HttpServer instance (the old one, executor included,
    just gets discarded/GC'd — and JDK's HttpServer can't be restarted
    after stop() anyway). But happy to add it defensively, no downside.
  • FixedThreadPool(2): kept deliberately small since the TTL guard
    means most scrapes now skip updateMetrics() entirely — 2 threads is
    just enough to stop one slow request blocking others, without
    over-engineering. Can make it configurable if you'd prefer.

Will push a follow-up commit nulling out the executor per your suggestion.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, I genuinely don’t understand this part of the change, I’ll read the co-pilot comment on the original issue as well. So if the executor is shutdown(), can start() still be called on it? (for instance) And why a newFixedThreadPool of 2?

Pushed 8637b6d — scoped setExecutor(null) inside the existing
httpServer null-check (your suggested diff had it unconditional, which
would NPE if start() never created a server) and nulled out
httpExecutor too.

httpExecutor = null;
return true;
}

Expand All @@ -122,7 +133,8 @@ public ConfigKey<?>[] getConfigKeys() {
EnablePrometheusExporter,
PrometheusExporterServerPort,
PrometheusExporterAllowedAddresses,
PrometheusExporterOfferingCountLimit
PrometheusExporterOfferingCountLimit,
PrometheusExporterMinRefreshInterval
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import java.lang.reflect.Field;
import java.util.Collections;

import com.cloud.dc.dao.DataCenterDao;

import org.junit.Test;

Expand Down Expand Up @@ -105,4 +114,66 @@ public void testItemHostCertExpiryContainsTimestampValue() {
assertTrue("Metric should contain correct timestamp value",
metricsString.endsWith(" " + CERT_EXPIRY_EPOCH));
}

/**
* Two rapid calls to updateMetrics() within the min refresh interval
* should result in only one actual recomputation (one call to dcDao.listAll()).
*/
@Test
public void testUpdateMetricsTTLGuardSkipsSecondCall() throws Exception {
PrometheusExporterImpl exporter = new PrometheusExporterImpl();

DataCenterDao mockDcDao = mock(DataCenterDao.class);
when(mockDcDao.listAll()).thenReturn(Collections.emptyList());
setField(exporter, "dcDao", mockDcDao);

// First call should trigger recomputation
exporter.updateMetrics();
// Second immediate call should be skipped by the TTL guard
exporter.updateMetrics();

verify(mockDcDao, times(1)).listAll();
}

/**
* After the min refresh interval has elapsed, updateMetrics() should
* trigger a fresh recomputation.
*/
@Test
public void testUpdateMetricsTTLGuardAllowsAfterInterval() throws Exception {
PrometheusExporterImpl exporter = new PrometheusExporterImpl();

DataCenterDao mockDcDao = mock(DataCenterDao.class);
when(mockDcDao.listAll()).thenReturn(Collections.emptyList());
setField(exporter, "dcDao", mockDcDao);

// First call
exporter.updateMetrics();

// Simulate that the min interval has already elapsed by resetting lastMetricsUpdateTime
setField(exporter, "lastMetricsUpdateTime", 0L);

// Second call should now trigger recomputation
exporter.updateMetrics();

verify(mockDcDao, times(2)).listAll();
}

private static void setField(Object target, String fieldName, Object value) throws Exception {
Field field = null;
Class<?> clazz = target.getClass();
while (clazz != null) {
try {
field = clazz.getDeclaredField(fieldName);
break;
} catch (NoSuchFieldException e) {
clazz = clazz.getSuperclass();
}
}
if (field == null) {
throw new NoSuchFieldException(fieldName);
}
field.setAccessible(true);
field.set(target, value);
}
}