Exposed the AsyncProfiler stop method #121

Krithika3 · 2023-10-10T05:47:45Z

As per #119 I had requested for the ability to do on-demand profiling. I have added a PR here to expose the stop method so we could stop profiling anytime in production after collecting samples for an amount of time. Please review and provide feedback. Apologies if standards are not followed

Krithika3 · 2023-10-10T05:48:14Z

Noticed that there are not many tests, so any recommendations on adding tests. Thanks

Krithika3 · 2023-10-10T05:48:38Z

@korniltsev please review in reference to #119

agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java

simonswine

Thanks for you contribution.

I think you need a different approach to achieve, stopping of the Profiler you started before. Let me know if my instructions make sense to you.

agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java

simonswine · 2023-10-18T14:26:39Z

demo/src/main/java/App.java

+        // 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()
+        );


This code will start a second profiler and stop it immediately

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

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

@simonswine pls re-review. Thanks

simonswine · 2023-10-18T14:29:12Z

agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java

+     * stop is used to stop profiling
+     * @param options
+     */
+    public static void stop(Options options) {


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.

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

@simonswine pls re-review and let me know comments

Co-authored-by: Christian Simon <simon@swine.de>

simonswine · 2023-10-23T14:53:32Z

agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java

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


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

I have removed it

simonswine · 2023-10-23T14:55:49Z

agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java

+        logger.log(Logger.Level.DEBUG, "Config: %s", options.config);
+        try {
+            startOptions.compareAndSet(null, options);
+            startOptions.get().scheduler.start(startOptions.get().profiler);


Simpler like that

Suggested change

startOptions.get().scheduler.start(startOptions.get().profiler);

options.scheduler.start(options.profiler);

simonswine · 2023-10-23T17:05:10Z

demo/src/main/java/App.java

+
+        Thread.sleep(2 * 60 * 1000);
+
+        // This is a naive implementation for the stop method
+        PyroscopeAgent.stop();


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.

I have added the test. Please re-review. Let me know if I am making any blunders as well

simonswine · 2023-10-23T17:06:10Z

agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java

+     * stop is used to stop profiling
+     */
+    public static void stop() {
+        Logger logger = startOptions.get().logger;


This will crash when startOptions is null

Added a null check

simonswine · 2023-10-23T17:08:25Z

agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java

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


Unsure if we need the try catch block here.

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

korniltsev · 2023-10-25T07:03:22Z

@Krithika3 Do you think you can implement this logic as a custom ProfilingScheduler at your application level (not in the sdk)?

Here's a quick dirty example
https://gist.github.com/korniltsev/68afb0dd09c596b7fd0306fa8b14ba6b

Krithika3 · 2023-10-25T14:30:00Z

@korniltsev thanks for the example. The main reason we wanted to do it here at the sdk level was because we may need to do this in multiple apps (and establish a pattern for profiling across the company ) and we did not want to add a CustomScheduler in everyone of them. So if we can have something added at the java agent level it could be great. Thanks and appreciate it

Krithika3 · 2023-10-25T16:13:44Z

Do we see any concerns doing the above changes? Thank you

korniltsev · 2023-10-26T05:02:09Z

start method was safe to call from multiple threads

new stop method is not safe to call from multiple threads

I suggest you replace AtomicReference with plain reference and guard it with synchronized block.

@Krithika3 Did you run your changes? Do they work?

Krithika3 · 2023-10-26T14:55:28Z

@korniltsev I ran the changes locally, with pyroscope running locally and the sample java app as an example. I did see behave as expected, let me attach a screenshot. One issue I observed was the ```

Krithika3 · 2023-10-31T01:13:08Z

@korniltsev I left the AtomicReferenceOptions> as is but guarded the stop with a synchronized block. Let me know if that makes sense

korniltsev · 2023-10-31T08:12:50Z

Start method should be synchronized as well.

I dont see a reason for AtomicReference, do you?

agent/src/main/java/io/pyroscope/javaagent/api/ProfilingScheduler.java

korniltsev · 2023-10-31T08:21:49Z

agent/src/main/java/io/pyroscope/javaagent/impl/ContinuousProfilingScheduler.java

+     */
+    @Override
+    public void stop(Profiler profiler) {
+        profiler.stop();


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?

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.

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.

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

I think it should.

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

And now it profiles forever

Ok let me make that change,

I have made the change @korniltsev

Krithika3 · 2023-11-01T05:34:22Z

Start method should be synchronized as well.
I have synchronized the start method as well
I dont see a reason for AtomicReference, do you?

I was not able to get a proper class to initialize Reference with

Krithika3 · 2023-11-01T05:41:48Z

The current run screenshot

This does not have the error with dumping profile data

Krithika3 · 2023-11-01T05:42:34Z

@korniltsev One worry I had was since we now have the need for synchronized block and adding locks are we introducing any performance issues with the Start and Stop methods?

Krithika3 · 2023-11-15T06:26:10Z

@korniltsev One worry I had was since we now have the need for synchronized block and adding locks are we introducing any performance issues with the Start and Stop methods?

Any thoughts here?

korniltsev · 2023-11-17T12:27:53Z

I see no changes for problem discussed here #121 (comment)

korniltsev · 2024-04-23T10:32:57Z

Closing this PR because of inactivity.

Will continue here #149

exposed the stop method

9334c93

Krithika3 commented Oct 10, 2023

View reviewed changes

agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java Show resolved Hide resolved

Krithika3 marked this pull request as ready for review October 11, 2023 14:21

simonswine reviewed Oct 18, 2023

View reviewed changes

Krithika3 and others added 3 commits October 18, 2023 08:03

Update agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java

3a96305

Co-authored-by: Christian Simon <simon@swine.de>

Update agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java

a5feb74

Co-authored-by: Christian Simon <simon@swine.de>

Added as mentioned by reviewer comments

6adb778

Krithika3 requested a review from simonswine October 19, 2023 13:36

Krithika3 added 2 commits October 19, 2023 06:55

Fixed conflicts

2d20500

Added a test

fc97fb0

simonswine reviewed Oct 23, 2023

View reviewed changes

Krithika3 added 2 commits October 23, 2023 11:49

Added changes as per review comments

63450bd

Fixed some more review comments

9ecee93

Krithika3 requested a review from simonswine October 24, 2023 05:06

Added synchronized stop

67e1315

korniltsev reviewed Oct 31, 2023

View reviewed changes

agent/src/main/java/io/pyroscope/javaagent/api/ProfilingScheduler.java Outdated Show resolved Hide resolved

korniltsev reviewed Oct 31, 2023

View reviewed changes

Added some additional changes as per review

5ae13eb

Krithika3 requested a review from korniltsev November 15, 2023 14:26

korniltsev closed this Apr 23, 2024

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

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

Exposed the AsyncProfiler stop method #121

Exposed the AsyncProfiler stop method #121

Conversation

Krithika3 commented Oct 10, 2023

Krithika3 commented Oct 10, 2023

Krithika3 commented Oct 10, 2023

simonswine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krithika3 Oct 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

korniltsev commented Oct 25, 2023

Krithika3 commented Oct 25, 2023

Krithika3 commented Oct 25, 2023

korniltsev commented Oct 26, 2023

Krithika3 commented Oct 26, 2023 • edited

Krithika3 commented Oct 31, 2023

korniltsev commented Oct 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

korniltsev Nov 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krithika3 commented Nov 1, 2023 • edited

Krithika3 commented Nov 1, 2023

Krithika3 commented Nov 1, 2023 • edited

Krithika3 commented Nov 15, 2023

korniltsev commented Nov 17, 2023

korniltsev commented Apr 23, 2024

Krithika3 Oct 19, 2023 •

edited

Krithika3 commented Oct 26, 2023 •

edited

korniltsev Nov 14, 2023 •

edited

Krithika3 commented Nov 1, 2023 •

edited

Krithika3 commented Nov 1, 2023 •

edited