Skip to content
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.contrib.inferredspans;

import java.time.Duration;
import javax.annotation.Nullable;

/**
* A global accessor for the {@link InferredSpansProcessor} instance.
*
* <p>This class is for internal use only and may be removed in a future release.
*/
public final class InferredSpans {

@Nullable private static volatile InferredSpansProcessor instance;

private InferredSpans() {}

/**
* Sets the {@link InferredSpansProcessor} instance.
*
* @param processor the processor instance
*/
public static void setInstance(@Nullable InferredSpansProcessor processor) {
instance = processor;
}

/**
* Returns whether inferred spans are enabled.
*
* @return whether inferred spans are enabled
*/
public static boolean isEnabled() {
return instance != null;
}

/**
* Sets the profiler interval.
*
* @param interval the new profiler interval
*/
public static void setProfilerInterval(Duration interval) {
InferredSpansProcessor p = instance;
if (p != null) {
p.setProfilerInterval(interval);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.time.Duration;
import java.util.Properties;
import java.util.concurrent.Executors;
import java.util.concurrent.ThreadFactory;
Expand All @@ -38,6 +39,7 @@ public class InferredSpansProcessor implements SpanProcessor {

// Visible for testing
final SamplingProfiler profiler;
private final InferredSpansConfiguration config;

private Supplier<TracerProvider> tracerProvider = GlobalOpenTelemetry::getTracerProvider;
@Nullable private volatile Tracer tracer;
Expand All @@ -48,12 +50,18 @@ public class InferredSpansProcessor implements SpanProcessor {
boolean startScheduledProfiling,
@Nullable File activationEventsFile,
@Nullable File jfrFile) {
this.config = config;
profiler = new SamplingProfiler(config, clock, this::getTracer, activationEventsFile, jfrFile);
if (startScheduledProfiling) {
profiler.start();
}
}

public void setProfilerInterval(Duration interval) {
config.setProfilerInterval(interval);
profiler.reschedule();
Comment on lines +61 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

If I get this correctly, when we "disable at runtime", it means:

  • a very long profiler interval is configured
  • reschedule() is executed and schedules profiling execution very far in the future

When we then "enable it at runtime", it means:

  • a relatively short profiler interval is configured
  • reschedule() is executed and schedules profiling execution accordingly to the interval (so in the close future)

In the case where the value set to disable is not large enough, for example 1 day, then it means that we will get one extra scheduled execution, which is probably not what we expect here.

I think it would probably be better to define a specific value/threshold to indicate that the profiler should be considered as disabled (zero or a significant value), it could then allow to avoid re-scheduling when such value is configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is documentation rather than anything for the code. The code change is not disabling anything, it's purely making the interval dynamically changeable.

A separate note for the doc (I'll do that in another change as I need to think about also how to explain using this dynamic capability for an example) is that choosing a very long time (eg max long) effectively disables the inferred spans, while leaving it in a state where it could be re-enabled

}

public static InferredSpansProcessorBuilder builder() {
return new InferredSpansProcessorBuilder();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,11 @@ public InferredSpansProcessor build() {
profilingDuration,
profilerLibDirectory,
parentOverrideHandler);
return new InferredSpansProcessor(
config, clock, startScheduledProfiling, activationEventsFile, jfrFile);
InferredSpansProcessor processor =
new InferredSpansProcessor(
config, clock, startScheduledProfiling, activationEventsFile, jfrFile);
InferredSpans.setInstance(processor);
return processor;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public class InferredSpansConfiguration {
private final Duration inferredSpansMinDuration;
private final List<WildcardMatcher> includedClasses;
private final List<WildcardMatcher> excludedClasses;
private final Duration profilerInterval;
private volatile Duration profilerInterval;
private final Duration profilingDuration;
@Nullable private final String profilerLibDirectory;
private final BiConsumer<SpanBuilder, SpanContext> parentOverrideHandler;
Expand Down Expand Up @@ -84,6 +84,10 @@ public Duration getProfilingInterval() {
return profilerInterval;
}

public void setProfilerInterval(Duration profilerInterval) {
this.profilerInterval = profilerInterval;
}

public Duration getProfilingDuration() {
return profilingDuration;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import java.util.Locale;
import java.util.Objects;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.LockSupport;
Expand Down Expand Up @@ -151,6 +152,7 @@ public class SamplingProfiler implements Runnable {
private final Supplier<Tracer> tracerProvider;

private final AsyncProfiler profiler;
@Nullable private volatile Future<?> profilingTask;

/**
* Creates a sampling profiler, optionally relying on existing files.
Expand Down Expand Up @@ -385,7 +387,7 @@ public void run() {

if (!interrupted && !scheduler.isShutdown()) {
long delay = config.getProfilingInterval().toMillis() - profilingDuration.toMillis();
scheduler.schedule(this, delay, TimeUnit.MILLISECONDS);
profilingTask = scheduler.schedule(this, delay, TimeUnit.MILLISECONDS);
}
}

Expand Down Expand Up @@ -723,7 +725,19 @@ public void copyFromFiles(Path activationEvents, Path traces) throws IOException

@SuppressWarnings("FutureReturnValueIgnored")
public void start() {
scheduler.submit(this);
profilingTask = scheduler.submit(this);
}

@SuppressWarnings({"FutureReturnValueIgnored", "Interruption"})
public void reschedule() {
Future<?> future = this.profilingTask;
if (future != null) {
if (future.cancel(true)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (future.cancel(true)) {
if (future.isDone() || future.cancel(true)) {

So the javadoc for Future.cancel() says that it returns

"false if the task could not be cancelled, typically because it has already completed;

If the thing finishes right before/when this reschedule() is invoked, we still want to kick it off again, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the task has already completed, it has already scheduled the next task, so on isDone() we do NOT want to schedule a new task in this reschedule() method

Duration profilingDuration = config.getProfilingDuration();
long delay = config.getProfilingInterval().toMillis() - profilingDuration.toMillis();
profilingTask = scheduler.schedule(this, delay, TimeUnit.MILLISECONDS);
}
}
}

public void stop() throws InterruptedException, IOException {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.contrib.inferredspans;

import static org.assertj.core.api.Assertions.assertThat;
import static org.awaitility.Awaitility.await;

import io.opentelemetry.contrib.inferredspans.internal.SamplingProfiler;
import io.opentelemetry.contrib.inferredspans.internal.util.DisabledOnOpenJ9;
import java.time.Duration;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.condition.DisabledOnOs;
import org.junit.jupiter.api.condition.OS;

@DisabledOnOs(OS.WINDOWS)
@DisabledOnOpenJ9
class InferredSpansTest {

private ProfilerTestSetup setup;

@BeforeEach
void setUp() {
InferredSpans.setInstance(null);
}

@AfterEach
void tearDown() {
if (setup != null) {
setup.close();
}
InferredSpans.setInstance(null);
}

@Test
void testIsEnabled() {
assertThat(InferredSpans.isEnabled()).isFalse();

setup = ProfilerTestSetup.create(c -> {});

assertThat(InferredSpans.isEnabled()).isTrue();

setup.close();
setup = null;

// In a real-world scenario, the close() method would lead to the processor being garbage
// collected, but to make it deterministic, we manually set the instance to null
InferredSpans.setInstance(null);
assertThat(InferredSpans.isEnabled()).isFalse();
}

@Test
void testSetProfilerIntervalWhenDisabled() {
InferredSpans.setProfilerInterval(Duration.ofMillis(10));

setup =
ProfilerTestSetup.create(
c ->
c.profilerInterval(Duration.ofSeconds(10))
.profilingDuration(Duration.ofMillis(500)));

// assert that the interval set before the profiler was initialized is ignored
assertThat(setup.profiler.getConfig().getProfilingInterval()).isEqualTo(Duration.ofSeconds(10));
}

@Test
void testSetProfilerInterval() {
setup =
ProfilerTestSetup.create(
c ->
c.profilerInterval(Duration.ofSeconds(10))
.profilingDuration(Duration.ofMillis(500)));

SamplingProfiler profiler = setup.profiler;
await()
.untilAsserted(() -> assertThat(profiler.getProfilingSessions()).isGreaterThanOrEqualTo(1));

InferredSpans.setProfilerInterval(Duration.ofMillis(100));

await()
.timeout(Duration.ofSeconds(2))
.untilAsserted(() -> assertThat(profiler.getProfilingSessions()).isGreaterThanOrEqualTo(2));
}
}
Loading