-
Notifications
You must be signed in to change notification settings - Fork 169
Add dynamically changing the inferred span interval #2153
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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; | ||||||
|
|
@@ -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. | ||||||
|
|
@@ -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); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -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)) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
So the javadoc for
If the thing finishes right before/when this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||||||
|
|
||||||
| 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)); | ||
| } | ||
| } |
There was a problem hiding this comment.
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:
reschedule()is executed and schedules profiling execution very far in the futureWhen we then "enable it at runtime", it means:
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.
There was a problem hiding this comment.
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