Allow dynamic update of agent parameters #45

amuraru · 2019-04-18T07:40:05Z

Made the agent profilers re-entrant allowing
to attach and update profiling parameters at runtime

This is useful when the VM is not started with the agent
but can be later attached and controlled, e.g:

update profiling interval
enabling/disabling profilers

For time being only profilers parameters can be updated.
Transformers, reporter not

CLAassistant · 2019-04-18T07:40:12Z

All committers have signed the CLA.

hiboyang · 2019-04-20T03:13:16Z

src/main/java/com/uber/profiling/ProfilerRunner.java

@@ -27,6 +28,7 @@

    private final Profiler profiler;
    private final AtomicLong errorCounter = new AtomicLong(0);
+    private ScheduledFuture<?> handler;


This "handler" field is only used by setter/getter. Kind of weird. Any reason add it here?

This is needed to allow the the profilers to be "rescheduled" in the Agent, that is - cancel the previous registration and start a new one if profiling frequency changes

I do not see your code invoke setHandler/getHandler. Maybe I miss something?

AgentImpl#scheduleProfilers calls both

Yeah, sorry I missed that previously. It is still weird that we only have setHandler/getHandler in this class. Looks like we should move the "handler" out of this class?

Ok - refactored this even more and actually get rid of the ProfilerRunner altogether.
Made the Profiler itself Runnable.

hiboyang · 2019-04-20T03:17:00Z

src/main/java/com/uber/profiling/profilers/ProcessInfoBase.java

 import com.uber.profiling.util.NetworkUtils;
 import com.uber.profiling.util.ProcFileUtils;
 import com.uber.profiling.util.ProcessUtils;
 import com.uber.profiling.util.SparkUtils;

 import java.util.UUID;

-public class ProcessInfoBase {
+public abstract class ProcessInfoBase extends Profiler {


The semantic for class inheritance is establishing "is-a" relationship, see here. ProcessInfoBase should not be a Profiler.

as part of this change, all classes from com.uber.profiling.profilers are now profilers.

Agree that ProcessInfoBase extends Profiler seems forced naming wise, but if you look for example at ProcessInfoProfiler extends ProcessInfoBase there is a profiler eventually extending base Profiler

So my suggestion would be to ProcessInfoBase to ProcessInfoBaseProfiler.
Would it make sense then?

Yeah, the old code "ProcessInfoProfiler extends ProcessInfoBase" looks not good as well. Let me do some refactor there.

I updated the code to rename ProcessInfoBase to ProfilerBase, which makes more sense now.

ok - rebased on top of master

hiboyang · 2019-04-20T03:18:34Z

src/main/java/com/uber/profiling/Profiler.java

+
+  public abstract void profile();
+
+  void setRunner(ProfilerRunner runner){


Adding setRunner/getRunner here makes Profiler depends on ProfilerRunner, but it should not depend on it. Would suggest removing setRunner/getRunner here.

Agree. Don't like that as well :)
Thinking of it - would it make sense to actually have the ProfilerRunner an inner class in the base Profiler abstract class or even better make the Profiler base class Runnable so it can be:

run once OR

scheduled
depending on the Profiler type

Again, my whole idea here is to make all profilers runnable (and hence cover both one-time and scheduled types).

I see. I do not see your code call setRunner/getRunner though. Still prefer profiler not depending on this runner.

hiboyang

Thanks for the code change here. It is usefully to support "attaching agent". I added several comments though.

Made the agent profilers re-entrant allowing to attach and update profiling parameters at runtime This is useful when the VM is not started with the agent but can be later attached and controlled, e.g: - update profiling interval - enabling/disabling profilers For time being only profilers parameters can be updated. Transformers, reporter not

amuraru · 2019-05-02T10:04:01Z

src/main/java/com/uber/profiling/Profiler.java


-    void setReporter(Reporter reporter);
+public abstract class Profiler implements Runnable {


Profiler now is a Runnable so it can be:

either run one-off if it's not scheduled run periodically

or, scheduled to run periodically

amuraru · 2019-05-02T10:04:35Z

src/main/java/com/uber/profiling/Profiler.java

+    }
+  }
+
+  public ScheduledFuture<?> getScheduleHandler() {


this is needed so we can reschedule the profiler when the scheduling interval is updated at runtime

hiboyang · 2019-05-06T05:38:25Z

src/main/java/com/uber/profiling/AgentImpl.java

+        for (Profiler profiler : profilers.values()) {
+            if (profiler.getScheduleHandler() != null) {
+                //cancel previous task if already scheduled
+                profiler.getScheduleHandler().cancel(false);


Suggest removing getScheduleHandler() method and hide the logic of this cancelling inside the profiler. e.g.

if (profiler.isRunning()) {
profiler.cancel()
}

good call - updated

amuraru · 2019-05-29T22:40:47Z

@boy-uber is this something you wanna integrate? Tx

amuraru force-pushed the reentrant_profilers branch from 70d1eb0 to 1bdcde7 Compare April 18, 2019 07:44

felixcheung requested a review from hiboyang April 20, 2019 02:50

hiboyang reviewed Apr 20, 2019

View reviewed changes

amuraru force-pushed the reentrant_profilers branch 2 times, most recently from 416a4df to 5dd6680 Compare April 23, 2019 20:59

amuraru force-pushed the reentrant_profilers branch from 5dd6680 to 5b9e9ce Compare May 2, 2019 09:58

amuraru force-pushed the reentrant_profilers branch from 5b9e9ce to d5b0ade Compare May 2, 2019 10:00

amuraru commented May 2, 2019

View reviewed changes

hiboyang reviewed May 6, 2019

View reviewed changes

implemented review

fa975e6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow dynamic update of agent parameters #45

Allow dynamic update of agent parameters #45

amuraru commented Apr 18, 2019

CLAassistant commented Apr 18, 2019 •

edited

Loading

hiboyang Apr 20, 2019

amuraru Apr 20, 2019

hiboyang Apr 22, 2019

amuraru Apr 22, 2019

hiboyang Apr 25, 2019 •

edited

Loading

amuraru May 2, 2019

hiboyang Apr 20, 2019

amuraru Apr 20, 2019

hiboyang Apr 22, 2019

hiboyang Apr 25, 2019

amuraru May 2, 2019

hiboyang Apr 20, 2019

amuraru Apr 20, 2019

hiboyang Apr 22, 2019

hiboyang left a comment

amuraru May 2, 2019

amuraru May 2, 2019

hiboyang May 6, 2019

amuraru May 24, 2019

amuraru commented May 29, 2019


		public abstract void profile();

		void setRunner(ProfilerRunner runner){


		void setReporter(Reporter reporter);
		public abstract class Profiler implements Runnable {

Allow dynamic update of agent parameters #45

Are you sure you want to change the base?

Allow dynamic update of agent parameters #45

Conversation

amuraru commented Apr 18, 2019

CLAassistant commented Apr 18, 2019 • edited Loading

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

hiboyang Apr 25, 2019 • edited Loading

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

hiboyang left a 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

amuraru commented May 29, 2019

CLAassistant commented Apr 18, 2019 •

edited

Loading

hiboyang Apr 25, 2019 •

edited

Loading