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

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 1 commit
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
15 changes: 15 additions & 0 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 @@ -48,6 +48,21 @@ public static void start(Options options) {
}
}

/**
* stop is used to stop profiling
simonswine marked this conversation as resolved.
Show resolved Hide resolved
* @param options
*/
public static void stop(Options options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think rather than passing Options here you, you should go a slightly different path.

I am proposing you replace the AtomicBool with an AtomicReference<Options>, which gets set by start().

Then the stop method can stop those <Options> of the previous start call and set it back to null.

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense, let me give that a shot and get back to you with questions.

Copy link
Author

Choose a reason for hiding this comment

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

@simonswine pls re-review and let me know comments

Logger logger = options.logger;
logger.log(Logger.Level.DEBUG, "Config: %s", options.config);
try {
options.scheduler.stop(options.profiler);
logger.log(Logger.Level.INFO, "Profiling started");
Krithika3 marked this conversation as resolved.
Show resolved Hide resolved
} catch (final Throwable e) {
logger.log(Logger.Level.ERROR, "Error starting profiler %s", e);
Krithika3 marked this conversation as resolved.
Show resolved Hide resolved
}
}

/**
* Options allow to swap pyroscope components:
* - io.pyroscope.javaagent.api.ProfilingScheduler
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
17 changes: 16 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 @@ -31,6 +31,21 @@ public static void main(String[] args) {
);
Pyroscope.setStaticLabels(mapOf("region", "us-east-1"));

Thread.sleep(5 * 60 * 1000);

// This is a naive implementation for the stop method
PyroscopeAgent.stop(
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())
.build()
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This code will start a second profiler and stop it immediately

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your review @simonswine , it makes sense to add AtomicReference and stop everything that was set up above. I will let you know if I run into anything

Copy link
Author

@Krithika3 Krithika3 Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I also noticed again when I ran this test, it starts the profiler and then waits for sometime with Thread.sleep and stops it but the start thread is still running so we get an error there saying Error Dumping profiler

Copy link
Author

Choose a reason for hiding this comment

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

@simonswine pls re-review. Thanks


appLogic();
}

Expand Down
Loading