Exposed the AsyncProfiler stop method

[Krithika3]

As per https://github.com/grafana/pyroscope-java/issues/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]

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

[Krithika3]

@korniltsev please review in reference to #119

[korniltsev]

@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]

@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]

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

[korniltsev]

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]

@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]

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

[korniltsev]

Start method should be synchronized as well.

I dont see a reason for AtomicReference, do you?

[Krithika3]

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]

The current run screenshot

This does not have the error with dumping profile data

[Krithika3]

@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]

@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]

I see no changes for problem discussed here https://github.com/grafana/pyroscope-java/pull/121#discussion_r1377207099

[korniltsev]

Closing this PR because of inactivity.

Will continue here https://github.com/grafana/pyroscope-java/pull/149