feat: add sampling scheduler #87
Conversation
| } | ||
| profiler.stop(); | ||
|
|
||
| Snapshot snapshot = profiler.dumpProfile(profilingStartTime); |
There was a problem hiding this comment.
we need profilingStartTime to be truncated to 10s(uploadInterval), see #40
There was a problem hiding this comment.
Changed to truncate(profilingStartTime, uploadInterval)
| import java.util.concurrent.ScheduledExecutorService; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| public class SamplingProfilingScheduler implements ProfilingScheduler { |
There was a problem hiding this comment.
I would maybe mark this class and corresponding configuration options as experimental ( for example just add comment in javadoc of the class)
This way we'll be able to change API in the future, for example someone may wish to do profiling for 10s or only when the moon is full or some other sophisticated schedule
There was a problem hiding this comment.
Comments added for class SamplingProfilingScheduler, config string PYROSCOPE_SAMPLING_RATE and PYROSCOPE_SAMPLING_DURATION, please check if it is ok.
|
Also I wonder if you really need it to be in the pyroscope-java or maybe you could jsut implement it in your app and plug it in with our builder api? |
|
Builder API is quite wonderful, but in some cases we want to use pyroscope just as a java agent, so it seems to be a little hard to customize the sampling way if there is no configuration support. I do missed the aligning feature, thanks for your review, I will adjust my code later. |
There was a problem hiding this comment.
Looks good.
I am a bit concerned that profiling end time is not correctly reported - it is startTime + uploadInterval, while here it should be startTime + uploadInterval*samplingRate or smth. Another thing that may be slightly incorrect is a sampleRate / total time during rendering - if a single profile reports 1s cpu time - it will show 1s cpu time in ui, but in reality it may have been 10s. I guess we can deal with it later if this becomes a problem.
|
It's OK. Thanks for you reviews. |
Full profiling is great but sometimes too large for production applications, sampling may help. For example, profiling at a sampling rate 0.1 means profiling for 1s every 10s. The sampling rate could based on
PYROSCOPE_UPLOAD_INTERVALSupported configs:
PYROSCOPE_SAMPLING_DURATION, specify sampling duration directly, must be less thanPYROSCOPE_UPLOAD_INTERVALPYROSCOPE_SAMPLING_RATE, sampling rate, used to calculate sampling duration, multiplied byPYROSCOPE_UPLOAD_INTERVAL. Its value must be in (0, 1) area.Set one of them should be enough for sampling. If both
PYROSCOPE_SAMPLING_DURATIONandPYROSCOPE_SAMPLING_RATEare set,PYROSCOPE_SAMPLING_RATEwill be ignored.Not sure this feature is useful or not, need some reviews,
thanks.