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.

Sign up for GitHub

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

Jump to bottom

Exposed the AsyncProfiler stop method #121

Closed
wants to merge 10 commits into from
Closed

Exposed the AsyncProfiler stop method #121

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9334c93
exposed the stop method
Krithika3 Oct 10, 2023
3a96305
Update agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java
Krithika3 Oct 18, 2023
a5feb74
Update agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java
Krithika3 Oct 18, 2023
6adb778
Added as mentioned by reviewer comments
Krithika3 Oct 19, 2023
2d20500
Fixed conflicts
Krithika3 Oct 19, 2023
fc97fb0
Added a test
Krithika3 Oct 22, 2023
63450bd
Added changes as per review comments
Krithika3 Oct 23, 2023
9ecee93
Fixed some more review comments
Krithika3 Oct 24, 2023
67e1315
Added synchronized stop
Krithika3 Oct 31, 2023
5ae13eb
Added some additional changes as per review
Krithika3 Nov 1, 2023
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
34 changes: 27 additions & 7 deletions agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@

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);

public static void premain(final String agentArgs,
final Instrumentation inst) {
Expand All @@ -35,22 +37,40 @@ public static void start(Config config) {

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

if( !startOptions.compareAndSet(null, options)) {
logger.log(Logger.Level.ERROR, "Failed to start profiling - already started");
return;
}
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);
try {
startOptions.compareAndSet(null, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will fail, as it will always happen in line 40. You can safely remove it:

Suggested change
startOptions.compareAndSet(null, options);
startOptions.compareAndSet(null, options);

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 removed it

startOptions.get().scheduler.start(startOptions.get().profiler);
Copy link
Contributor

Choose a reason for hiding this comment

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

Simpler like that

Suggested change
startOptions.get().scheduler.start(startOptions.get().profiler);
options.scheduler.start(options.profiler);

Copy link
Author

Choose a reason for hiding this comment

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

Done

logger.log(Logger.Level.INFO, "Profiling started");
} catch (final Throwable e) {
logger.log(Logger.Level.ERROR, "Error starting profiler %s", e);
}
}

if (!started.compareAndSet(false, true)) {
/**
* stop is used to stop profiling
simonswine marked this conversation as resolved.
Show resolved Hide resolved
*/
public static void stop() {
Logger logger = startOptions.get().logger;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will crash when startOptions is null

Copy link
Author

Choose a reason for hiding this comment

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

Added a null check

logger.log(Logger.Level.DEBUG, "Config: %s", startOptions.get().config);
if( startOptions.get() == null) {
logger.log(Logger.Level.ERROR, "Failed to start profiling - already started");
return;
}
logger.log(Logger.Level.DEBUG, "Config: %s", options.config);
try {
options.scheduler.start(options.profiler);
logger.log(Logger.Level.INFO, "Profiling started");
startOptions.get().scheduler.stop(startOptions.get().profiler);
startOptions.set(null);
logger.log(Logger.Level.INFO, "Profiling stopped");
} catch (final Throwable e) {
logger.log(Logger.Level.ERROR, "Error starting profiler %s", e);
logger.log(Logger.Level.ERROR, "Error stopping profiler %s", e);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure if we need the try catch block here.

Copy link
Author

Choose a reason for hiding this comment

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

Ya I removed the try/catch block. Might not even be needed in start

}

Expand Down
7 changes: 7 additions & 0 deletions agent/src/main/java/io/pyroscope/javaagent/api/ProfilingScheduler.java
View file
Open in desktop
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);
}
9 changes: 9 additions & 0 deletions agent/src/main/java/io/pyroscope/javaagent/impl/ContinuousProfilingScheduler.java
View file
Open in desktop
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 •
edited
Loading

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
9 changes: 9 additions & 0 deletions agent/src/main/java/io/pyroscope/javaagent/impl/SamplingProfilingScheduler.java
View file
Open in desktop
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
5 changes: 5 additions & 0 deletions agent/src/test/java/io/pyroscope/javaagent/PyroscopeAgentTest.java
View file
Open in desktop
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());
}

}
7 changes: 6 additions & 1 deletion demo/src/main/java/App.java
View file
Open in desktop
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,11 @@ public static void main(String[] args) {
Pyroscope.setStaticLabels(mapOf("region", "us-east-1"));

appLogic();

Thread.sleep(2 * 60 * 1000);

// This is a naive implementation for the stop method
PyroscopeAgent.stop();
Copy link
Contributor

Choose a reason for hiding this comment

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

To test this properly I suggest, you Start it, then in a separate thread: stop it after a 1 second and then start it again a second later. This will make sure it works as expected.

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 test. Please re-review. Let me know if I am making any blunders as well

}

private static void appLogic() {
Expand Down