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

Thread count and classes loaded handlers #571

Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@
import io.opentelemetry.api.metrics.MeterProvider;
import io.opentelemetry.contrib.jfr.metrics.internal.RecordedEventHandler;
import io.opentelemetry.contrib.jfr.metrics.internal.ThreadGrouper;
import io.opentelemetry.contrib.jfr.metrics.internal.classLoading.ClassesLoadedHandler;
import io.opentelemetry.contrib.jfr.metrics.internal.container.ContainerConfigurationHandler;
import io.opentelemetry.contrib.jfr.metrics.internal.cpu.ContextSwitchRateHandler;
import io.opentelemetry.contrib.jfr.metrics.internal.cpu.LongLockHandler;
import io.opentelemetry.contrib.jfr.metrics.internal.cpu.OverallCPULoadHandler;
import io.opentelemetry.contrib.jfr.metrics.internal.cpu.ThreadCountHandler;
import io.opentelemetry.contrib.jfr.metrics.internal.memory.G1HeapSummaryHandler;
import io.opentelemetry.contrib.jfr.metrics.internal.memory.GCHeapSummaryHandler;
import io.opentelemetry.contrib.jfr.metrics.internal.memory.ObjectAllocationInNewTLABHandler;
Expand Down Expand Up @@ -68,7 +70,9 @@ static HandlerRegistry createDefault(MeterProvider meterProvider) {
new ContextSwitchRateHandler(),
new OverallCPULoadHandler(),
new ContainerConfigurationHandler(),
new LongLockHandler(grouper));
new LongLockHandler(grouper),
new ThreadCountHandler(),
new ClassesLoadedHandler());
handlers.addAll(basicHandlers);

var meter =
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.contrib.jfr.metrics.internal.classLoading;

import static io.opentelemetry.contrib.jfr.metrics.internal.RecordedEventHandler.defaultMeter;

import io.opentelemetry.api.metrics.Meter;
import io.opentelemetry.contrib.jfr.metrics.internal.RecordedEventHandler;
import java.time.Duration;
import java.util.Optional;
import jdk.jfr.consumer.RecordedEvent;

public final class ClassesLoadedHandler implements RecordedEventHandler {
private static final String METRIC_NAME = "process.runtime.jvm.cpu.loaded_class_count";
private static final String EVENT_NAME = "jdk.ClassLoadingStatistics";
private static final String METRIC_DESCRIPTION = "Number of loaded classes";

private volatile long value = 0;

public ClassesLoadedHandler() {
initializeMeter(defaultMeter());
}

@Override
public void accept(RecordedEvent ev) {
value = ev.getLong("loadedClassCount");
}

@Override
public String getEventName() {
return EVENT_NAME;
}

@Override
public void initializeMeter(Meter meter) {
meter
.gaugeBuilder(METRIC_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

This event has unloadedClassCount and loadedClassCount, both of which seem like they would be better modeled by monotonically increasing counters instead of a gauge. While unloads may be uncommon, they do happen, and if one of the goals of this class is to give an idea of the total classes currently loaded, both values should be tracked and the delta reported in the callback.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, please take a look at the OpenTelemetry JVM Metrics specification, which currently lists these metrics, which you should conform to:

  • process.runtime.jvm.classes.loaded
  • process.runtime.jvm.classes.unloaded
  • process.runtime.jvm.classes.current_loaded

I think this handler can probably provide all 3, but if you wanted to wait an do current_loaded in another implementation/PR that would be fine too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review! I've changed it to use monotonically increasing counters and also added an implementation for process.runtime.jvm.classes.current_loaded with an upDownCounter. I've corrected the instrument, unit, and descriptions so that they now should conform to the specification.

.ofLongs()
.setDescription(METRIC_DESCRIPTION)
.buildWithCallback(measurement -> measurement.record(value));
}

@Override
public Optional<Duration> getPollingDuration() {
return Optional.of(Duration.ofSeconds(1));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would appreciate a unit test that covers the behavior of this class in isolation.

Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.contrib.jfr.metrics.internal.cpu;

import static io.opentelemetry.contrib.jfr.metrics.internal.RecordedEventHandler.defaultMeter;

import io.opentelemetry.api.metrics.Meter;
import io.opentelemetry.contrib.jfr.metrics.internal.RecordedEventHandler;
import java.time.Duration;
import java.util.Optional;
import jdk.jfr.consumer.RecordedEvent;

public final class ThreadCountHandler implements RecordedEventHandler {
private static final String METRIC_NAME = "process.runtime.jvm.cpu.active_threads";
Copy link
Member

Choose a reason for hiding this comment

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

The OpenTelemetry spec defines process.runtime.jvm.threads.count, can you make sure that this implementation of JVM metrics conforms to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! I've changed the thread count handler so that it now properly implements the specification.

private static final String EVENT_NAME = "jdk.JavaThreadStatistics";
private static final String METRIC_DESCRIPTION = "Number of active threads";

private volatile long value = 0;

public ThreadCountHandler() {
initializeMeter(defaultMeter());
}

@Override
public void accept(RecordedEvent ev) {
value = ev.getLong("activeCount");
}

@Override
public String getEventName() {
return EVENT_NAME;
}

@Override
public void initializeMeter(Meter meter) {
meter
.gaugeBuilder(METRIC_NAME)
.ofLongs()
.setDescription(METRIC_DESCRIPTION)
.buildWithCallback(measurement -> measurement.record(value));
}

@Override
public Optional<Duration> getPollingDuration() {
return Optional.of(Duration.ofSeconds(1));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would like to see a unit test that covers this class in isolation as well.

Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,14 @@ public class AbstractMetricsTest {

static SdkMeterProvider meterProvider;
static InMemoryMetricReader metricReader;
static boolean isInitialized = false;

@BeforeAll
static void initializeOpenTelemetry() {
if (isInitialized) {
return;
}
isInitialized = true;
metricReader = InMemoryMetricReader.create();
meterProvider = SdkMeterProvider.builder().registerMetricReader(metricReader).build();
GlobalOpenTelemetry.set(OpenTelemetrySdk.builder().setMeterProvider(meterProvider).build());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.contrib.jfr.metrics;

import java.util.ArrayList;
import java.util.List;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

public class JfrPeriodicMetricsTest extends AbstractMetricsTest {
static class Stressor {
private Stressor() {}

public static void execute(int numberOfThreads, Runnable task) throws Exception {
List<Thread> threads = new ArrayList<>();
for (int n = 0; n < numberOfThreads; ++n) {
Thread t = new Thread(task);
threads.add(t);
t.start();
}
for (Thread t : threads) {
t.join();
}
}
}

private static final int THREADS = 10;
private static final int MILLIS = 50;

static Object monitor = new Object();
private static int count = 0;

private static void doWork(Object obj) throws InterruptedException {
count++;
synchronized (obj) {
// Spin until everyone is at critical section. All threads must be running now.
while (count < THREADS) {
Thread.sleep(MILLIS);
}
}
}

@Test
public void shouldHaveJfrPeriodicEvents() throws Exception {
Copy link
Contributor

@breedx-splk breedx-splk Nov 8, 2022

Choose a reason for hiding this comment

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

I think there's bad precedent elsewhere in this repo, but with JUnit5 you shouldn't need the test class nor test method to be public I think. Preference is to just make them package to save chars and improve readability. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

// This should generate some events

Runnable r =
() -> {
// create contention between threads for one lock
try {
doWork(monitor);
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
};
Stressor.execute(THREADS, r);

waitAndAssertMetrics(
metric ->
metric
.hasName("process.runtime.jvm.cpu.active_threads")
.hasLongGaugeSatisfying(
sum ->
sum.hasPointsSatisfying(
point ->
point.satisfies(
pointData ->
Assertions.assertTrue(pointData.getValue() >= THREADS)))),
metric ->
metric
.hasName("process.runtime.jvm.cpu.loaded_class_count")
.hasLongGaugeSatisfying(
sum ->
sum.hasPointsSatisfying(
point ->
point.satisfies(
pointData ->
Assertions.assertTrue(pointData.getValue() >= 0)))));
}
}