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

Exposed the AsyncProfiler stop method #121

Closed
wants to merge 10 commits into from
44 changes: 36 additions & 8 deletions agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@
import io.pyroscope.javaagent.impl.*;

import java.lang.instrument.Instrumentation;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;

public class PyroscopeAgent {
private static final AtomicBoolean started = new AtomicBoolean(false);
// this is used to store the options passed to the agent
private static final AtomicReference<Options> startOptions = new AtomicReference<>(null);

private static final String stopLock = "";

public static void premain(final String agentArgs,
final Instrumentation inst) {
Expand All @@ -35,14 +38,12 @@ public static void start(Config config) {

public static void start(Options options) {
Logger logger = options.logger;

if (!options.config.agentEnabled) {
logger.log(Logger.Level.INFO, "Pyroscope agent start disabled by configuration");
if (!startOptions.compareAndSet(null, options)) {
logger.log(Logger.Level.ERROR, "Failed to start profiling - already started");
return;
}

if (!started.compareAndSet(false, true)) {
logger.log(Logger.Level.ERROR, "Failed to start profiling - already started");
if (!options.config.agentEnabled) {
logger.log(Logger.Level.INFO, "Pyroscope agent start disabled by configuration");
return;
}
logger.log(Logger.Level.DEBUG, "Config: %s", options.config);
Expand All @@ -54,6 +55,33 @@ public static void start(Options options) {
}
}

/**
* stop is used to stop profiling
simonswine marked this conversation as resolved.
Show resolved Hide resolved
*/
public static void stop() {
synchronized (stopLock) {
if (startOptions.get() == null) {
return;
}
ProfilingScheduler scheduler = startOptions.get().scheduler;
Logger logger = startOptions.get().logger;
Profiler profiler = startOptions.get().profiler;

if (logger == null) {
return;
}
if (scheduler == null || profiler == null) {
logger.log(Logger.Level.ERROR, "Failed to stop profiling - already stopped");
return;
}

logger.log(Logger.Level.DEBUG, "Config: %s", startOptions.get().config);
scheduler.stop(profiler);
startOptions.set(null);
logger.log(Logger.Level.INFO, "Profiling stopped");
}
}

/**
* Options allow to swap pyroscope components:
* - io.pyroscope.javaagent.api.ProfilingScheduler
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,11 @@ public interface ProfilingScheduler {
*
**/
void start(Profiler profiler);

/**
* stop is ised tp stp[ the profiler
Krithika3 marked this conversation as resolved.
Show resolved Hide resolved
* {@link Profiler#stop()}
* {@link Profiler#dumpProfile(Instant, Instant)}
* **/
void stop(Profiler profiler);
}
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,15 @@ public void start(Profiler profiler) {

}

/**
* Stops the profiling scheduler which stops the AsyncProfiler.
* @param profiler
*/
@Override
public void stop(Profiler profiler) {
profiler.stop();
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this stop the this.job task somehow ?
What happens if this stop method is executed concurrently with dumpProfile runnable? Is it possible? Are they synchronized in any way? should they?

Copy link
Author

Choose a reason for hiding this comment

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

This may not be stopping the this.job. Let me take a look at that. Also let me see if it can be executed concurrently with dumpProfile.

Copy link
Author

Choose a reason for hiding this comment

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

I have added the logic to stop this.job task as well. Let me look into the second part of running stop concurrently with dumpProfile runnable.

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure if the dumProfile and stop should be synchronized. @korniltsev any insights?

Copy link
Collaborator

@korniltsev korniltsev Nov 14, 2023

Choose a reason for hiding this comment

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

I think it should.

T1(dump) calls profiler.stop
T2(stopper) calls scheduler.stop
T1(dump) calls profiler.start

And now it profiles forever

Copy link
Author

Choose a reason for hiding this comment

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

Ok let me make that change,

Copy link
Author

Choose a reason for hiding this comment

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

I have made the change @korniltsev

}

private void stop() {
if (job != null) {
job.cancel(true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,15 @@ public void start(Profiler profiler) {
);
}

/**
* Stops the profiling scheduler which stops the AsyncProfiler.
* @param profiler
*/
@Override
public void stop(Profiler profiler) {
profiler.stop();
}

private void dumpProfile(final Profiler profiler, final long samplingDurationMillis, final Duration uploadInterval) {
Instant profilingStartTime = Instant.now();
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ void startupTestWithEnabledAgent() {
PyroscopeAgent.start(optionsAgentEnabled);

verify(profilingScheduler, times(1)).start(any());

PyroscopeAgent.stop();

verify(profilingScheduler, times(1)).stop(any());
}

@Test
Expand All @@ -58,4 +62,5 @@ void startupTestWithDisabledAgent() {

verify(profilingScheduler, never()).start(any());
}

}
35 changes: 34 additions & 1 deletion demo/src/main/java/App.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
public class App {
public static final int N_THREADS = 8;

public static void main(String[] args) {
public static void main(String[] args) throws InterruptedException {
PyroscopeAgent.start(
new PyroscopeAgent.Options.Builder(
new Config.Builder()
Expand All @@ -32,6 +32,39 @@ public static void main(String[] args) {
Pyroscope.setStaticLabels(mapOf("region", "us-east-1"));

appLogic();

// Test this by creating a new thread, stop the agent, sleeping for a second, and then restarting the agent.
Thread newThread = new Thread(new Runnable() {
public void run()
{
// Sleep for a second to make sure the agent has started
try {
Thread.sleep(1000L);
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
// Stop the agent after a second
PyroscopeAgent.stop();
try {
Thread.sleep(1000L);
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
// Start the agent back up again after a second
PyroscopeAgent.start(
new PyroscopeAgent.Options.Builder(
new Config.Builder()
.setApplicationName("demo.app{qweqwe=asdasd}")
.setServerAddress("http://localhost:4040")
.setFormat(Format.JFR)
.setLogLevel(Logger.Level.DEBUG)
.setLabels(mapOf("user", "tolyan"))
.build())
// .setExporter(new MyStdoutExporter())
.build()
);
}});
newThread.start();
}

private static void appLogic() {
Expand Down