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

Add ThreadFactory tags to differentiate platform and virtual threads #1794

Merged
merged 3 commits into from
Sep 17, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions tritium-metrics/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ dependencies {

implementation 'com.google.code.findbugs:jsr305'
implementation 'com.google.guava:guava'
implementation 'com.palantir.nylon:nylon-threads'
implementation 'com.palantir.safe-logging:logger'
implementation 'com.palantir.safe-logging:preconditions'
implementation 'com.palantir.safe-logging:safe-logging'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
import com.codahale.metrics.Meter;
import com.palantir.logsafe.Preconditions;
import com.palantir.logsafe.Safe;
import com.palantir.nylon.threads.VirtualThreads;
import com.palantir.tritium.metrics.ExecutorMetrics.ThreadsCreated_ThreadType;
import com.palantir.tritium.metrics.ExecutorMetrics.ThreadsRunning_ThreadType;
import java.util.concurrent.ThreadFactory;

/**
Expand All @@ -28,42 +31,64 @@
final class TaggedMetricsThreadFactory implements ThreadFactory {

private final ThreadFactory delegate;
private final Meter created;
private final Counter running;
// Note that there's no guarantee a given ThreadFactory implementation
// will always produce the same kind of thread for every invocation, so
// we must track both variants.
private final Meter createdPlatform;
private final Meter createdVirtual;
private final Counter runningPlatform;
private final Counter runningVirtual;

TaggedMetricsThreadFactory(ThreadFactory delegate, ExecutorMetrics metrics, @Safe String name) {
this.delegate = Preconditions.checkNotNull(delegate, "ThreadFactory is required");
Preconditions.checkNotNull(name, "Name is required");
Preconditions.checkNotNull(metrics, "ExecutorMetrics is required");
this.created = metrics.threadsCreated(name);
this.running = metrics.threadsRunning(name);
this.createdPlatform = metrics.threadsCreated()
.executor(name)
.threadType(ThreadsCreated_ThreadType.PLATFORM)
.build();
this.createdVirtual = metrics.threadsCreated()
.executor(name)
.threadType(ThreadsCreated_ThreadType.VIRTUAL)
.build();
this.runningPlatform = metrics.threadsRunning()
.executor(name)
.threadType(ThreadsRunning_ThreadType.PLATFORM)
.build();
this.runningVirtual = metrics.threadsRunning()
.executor(name)
.threadType(ThreadsRunning_ThreadType.VIRTUAL)
.build();
}

@Override
public Thread newThread(Runnable runnable) {
Thread result = delegate.newThread(
new InstrumentedTask(Preconditions.checkNotNull(runnable, "Runnable is required"), running));
created.mark();
Thread result =
delegate.newThread(new InstrumentedTask(Preconditions.checkNotNull(runnable, "Runnable is required")));
createdMeterFor(result).mark();
return result;
}

private Meter createdMeterFor(Thread thread) {
return VirtualThreads.isVirtual(thread) ? createdVirtual : createdPlatform;
}

@Override
public String toString() {
return "TaggedMetricsThreadFactory{delegate=" + delegate + '}';
}

private static final class InstrumentedTask implements Runnable {
private final class InstrumentedTask implements Runnable {

private final Runnable delegate;
private final Counter running;

InstrumentedTask(Runnable delegate, Counter running) {
InstrumentedTask(Runnable delegate) {
this.delegate = delegate;
this.running = running;
}

@Override
public void run() {
Counter running = runningCounterFor(Thread.currentThread());
running.inc();
try {
delegate.run();
Expand All @@ -72,6 +97,10 @@ public void run() {
}
}

private Counter runningCounterFor(Thread thread) {
return VirtualThreads.isVirtual(thread) ? runningVirtual : runningPlatform;
}

@Override
public String toString() {
return "InstrumentedTask{delegate=" + delegate + '}';
Expand Down
10 changes: 8 additions & 2 deletions tritium-metrics/src/main/metrics/metrics.yml
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,15 @@ namespaces:
# ThreadFactory metrics
threads.created:
type: meter
tags: [executor]
tags:
- executor
- name: thread-type
values: [platform, virtual]
docs: Rate that new threads are created for this executor.
threads.running:
type: counter
tags: [executor]
tags:
- executor
- name: thread-type
values: [platform, virtual]
docs: Number of live threads created by this executor.
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@
import com.google.common.util.concurrent.ThreadFactoryBuilder;
import com.google.common.util.concurrent.Uninterruptibles;
import com.palantir.logsafe.exceptions.SafeNullPointerException;
import com.palantir.nylon.threads.VirtualThreads;
import com.palantir.tritium.metrics.ExecutorMetrics.ThreadsCreated_ThreadType;
import com.palantir.tritium.metrics.ExecutorMetrics.ThreadsRunning_ThreadType;
import com.palantir.tritium.metrics.registry.DefaultTaggedMetricRegistry;
import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;
import java.time.Duration;
Expand All @@ -44,8 +47,55 @@ void testInstrumentation() {
.build();
ThreadFactory instrumented = MetricRegistries.instrument(registry, delegate, name);
ExecutorMetrics metrics = ExecutorMetrics.of(registry);
Counter running = metrics.threadsRunning(name);
Meter created = metrics.threadsCreated(name);
Counter running = metrics.threadsRunning()
.executor(name)
.threadType(ThreadsRunning_ThreadType.PLATFORM)
.build();
Meter created = metrics.threadsCreated()
.executor(name)
.threadType(ThreadsCreated_ThreadType.PLATFORM)
.build();
assertThat(running.getCount()).isZero();
assertThat(created.getCount()).isZero();
CountDownLatch latch = new CountDownLatch(1);
Thread thread = instrumented.newThread(() -> Uninterruptibles.awaitUninterruptibly(latch));
assertThat(created.getCount()).isOne();
// thread has not started yet
assertThat(running.getCount()).isZero();
thread.start();
// Allow the thread to start in the background
Awaitility.waitAtMost(Duration.ofSeconds(3)).untilAsserted(() -> {
assertThat(created.getCount()).isOne();
assertThat(running.getCount()).isOne();
});
latch.countDown();
Awaitility.waitAtMost(Duration.ofSeconds(3)).untilAsserted(() -> {
assertThat(created.getCount()).isOne();
assertThat(running.getCount()).isZero();
});
Awaitility.waitAtMost(Duration.ofSeconds(1))
.untilAsserted(() -> assertThat(thread.isAlive()).isFalse());
}

@Test
void testVirtualThreadInstrumentation() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests share a fair bit of code. I bias away from sharing functionality between tests because it often results in refactors down the line that reduce the efficacy of some tests. In this case the tests are validating very similar things with different inputs, so there may be a stronger argument for sharing test code

String name = "name";
TaggedMetricRegistry registry = new DefaultTaggedMetricRegistry();
ThreadFactory delegate = VirtualThreads.get()
.orElseThrow(() -> new AssertionError("Expected jdk21+"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we assumeThat(VirtualThreads.get).isPresent() or similar to allow skipping test on JDK < 21?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

.ofVirtual()
.name("virtual-test-", 0)
.factory();
ThreadFactory instrumented = MetricRegistries.instrument(registry, delegate, name);
ExecutorMetrics metrics = ExecutorMetrics.of(registry);
Counter running = metrics.threadsRunning()
.executor(name)
.threadType(ThreadsRunning_ThreadType.VIRTUAL)
.build();
Meter created = metrics.threadsCreated()
.executor(name)
.threadType(ThreadsCreated_ThreadType.VIRTUAL)
.build();
assertThat(running.getCount()).isZero();
assertThat(created.getCount()).isZero();
CountDownLatch latch = new CountDownLatch(1);
Expand Down
13 changes: 7 additions & 6 deletions versions.lock
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@ com.google.j2objc:j2objc-annotations:2.8 (1 constraints: be09f5a0)
com.palantir.delegate.processors:delegate-processors:1.1.0 (1 constraints: 0405f335)
com.palantir.goethe:goethe:0.11.0 (1 constraints: 711660f3)
com.palantir.jvm.diagnostics:jvm-diagnostics:0.3.0 (1 constraints: 0505f435)
com.palantir.safe-logging:logger:3.6.0 (2 constraints: aa11c1f9)
com.palantir.nylon:nylon-threads:0.4.0 (1 constraints: 0605f735)
com.palantir.safe-logging:logger:3.6.0 (3 constraints: f21f1b22)
com.palantir.safe-logging:logger-slf4j:3.6.0 (1 constraints: 040e6542)
com.palantir.safe-logging:logger-spi:3.6.0 (2 constraints: 171e6d7b)
com.palantir.safe-logging:preconditions:3.6.0 (2 constraints: aa11c1f9)
com.palantir.safe-logging:safe-logging:3.6.0 (5 constraints: 2d4085dc)
com.palantir.safe-logging:preconditions:3.6.0 (3 constraints: f21f1b22)
com.palantir.safe-logging:safe-logging:3.6.0 (6 constraints: 754ef87e)
com.palantir.tracing:tracing:6.18.0 (1 constraints: 41055f3b)
com.palantir.tracing:tracing-api:6.18.0 (2 constraints: 17121d19)
com.squareup:javapoet:1.13.0 (3 constraints: 9e27444d)
Expand All @@ -24,9 +25,12 @@ io.dropwizard.metrics:metrics-jvm:4.2.19 (1 constraints: 4205483b)
net.bytebuddy:byte-buddy:1.14.7 (3 constraints: 001ca9f4)
org.checkerframework:checker-qual:3.37.0 (3 constraints: de24cc63)
org.hdrhistogram:HdrHistogram:2.1.12 (1 constraints: 3805313b)
org.jboss.logging:jboss-logging:3.4.3.Final (3 constraints: f2300ed8)
org.jboss.threads:jboss-threads:3.5.0.Final (3 constraints: b92aebe5)
org.jetbrains:annotations:24.0.1 (2 constraints: 11204d00)
org.mpierce.metrics.reservoir:hdrhistogram-metrics-reservoir:1.1.3 (1 constraints: 0705f635)
org.slf4j:slf4j-api:1.7.36 (7 constraints: d75d16df)
org.wildfly.common:wildfly-common:1.5.4.Final (2 constraints: 741cfbf1)

[Test dependencies]
com.google.auto.value:auto-value:1.7.4 (1 constraints: 1f1221fb)
Expand All @@ -48,8 +52,6 @@ org.assertj:assertj-core:3.24.2 (1 constraints: 3d05473b)
org.awaitility:awaitility:4.2.0 (1 constraints: 08050536)
org.hamcrest:hamcrest:2.1 (1 constraints: 6f0b2cce)
org.hamcrest:hamcrest-core:1.3 (1 constraints: cc05fe3f)
org.jboss.logging:jboss-logging:3.4.3.Final (3 constraints: f2300ed8)
org.jboss.threads:jboss-threads:3.5.0.Final (2 constraints: 5a1a5743)
org.jboss.xnio:xnio-api:3.8.8.Final (2 constraints: 791a6546)
org.jboss.xnio:xnio-nio:3.8.8.Final (1 constraints: c90dd230)
org.jetbrains.kotlin:kotlin-stdlib:1.3.40 (2 constraints: 01173b5e)
Expand All @@ -67,4 +69,3 @@ org.opentest4j:opentest4j:1.3.0 (6 constraints: 7846fee1)
org.ow2.asm:asm:9.0 (1 constraints: 030aa6a4)
org.slf4j:slf4j-simple:1.7.36 (1 constraints: 43054b3b)
org.wildfly.client:wildfly-client-config:1.0.1.Final (1 constraints: 940c6308)
org.wildfly.common:wildfly-common:1.5.4.Final (2 constraints: 741cfbf1)
1 change: 1 addition & 0 deletions versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ com.google.guava:guava = 32.1.2-jre
com.google.testing.compile:* = 0.19
com.palantir.delegate.processors:* = 1.1.0
com.palantir.jvm.diagnostics:* = 0.3.0
com.palantir.nylon:* = 0.4.0
com.palantir.safe-logging:* = 3.6.0
com.palantir.tracing:* = 6.18.0
com.squareup:javapoet = 1.13.0
Expand Down