From b1a712add815127928c3da59e85f7d2e5f20cbc3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 27 Feb 2020 06:42:45 -0800 Subject: [PATCH] Integrate "ServiceProfiler" into the base "Future" Summary: Depends on D21036. Ref T11968. Ref T13177. Currently, each Future integrates separately with ServiceProfiler, but much of the code is similar. Move integration to the base class and lift up most of the implementation details. Test Plan: Ran `arc diff --trace`, saw sensible output. Maniphest Tasks: T13177, T11968 Differential Revision: https://secure.phabricator.com/D21038 --- src/future/Future.php | 43 ++++++++++++++++++++ src/future/FutureProxy.php | 8 ++++ src/future/exec/ExecFuture.php | 55 +++++++++++--------------- src/future/exec/PhutilExecPassthru.php | 36 ++++++++++------- src/future/http/HTTPFuture.php | 17 ++++---- src/future/http/HTTPSFuture.php | 20 +++++----- 6 files changed, 114 insertions(+), 65 deletions(-) diff --git a/src/future/Future.php b/src/future/Future.php index f69f14bc..8ce48eb6 100644 --- a/src/future/Future.php +++ b/src/future/Future.php @@ -13,6 +13,7 @@ abstract class Future extends Phobject { private $result; private $exception; private $futureKey; + private $serviceProfilerCallID; /** * Is this future's process complete? Specifically, can this future be @@ -60,6 +61,7 @@ abstract class Future extends Phobject { } $this->hasStarted = true; + $this->startServiceProfiler(); $this->isReady(); } @@ -96,8 +98,49 @@ abstract class Future extends Phobject { 'than once.')); } $this->hasEnded = true; + + $this->endServiceProfiler(); } + private function startServiceProfiler() { + + // NOTE: This is a soft dependency so that we don't need to build the + // ServiceProfiler into the Phage agent. Normally, this class is always + // available. + + if (!class_exists('PhutilServiceProfiler')) { + return; + } + + $params = $this->getServiceProfilerStartParameters(); + + $profiler = PhutilServiceProfiler::getInstance(); + $call_id = $profiler->beginServiceCall($params); + + $this->serviceProfilerCallID = $call_id; + } + + private function endServiceProfiler() { + $call_id = $this->serviceProfilerCallID; + if ($call_id === null) { + return; + } + + $params = $this->getServiceProfilerResultParameters(); + + $profiler = PhutilServiceProfiler::getInstance(); + $profiler->endServiceCall($call_id, $params); + } + + protected function getServiceProfilerStartParameters() { + return array(); + } + + protected function getServiceProfilerResultParameters() { + return array(); + } + + /** * Retrieve a list of sockets which we can wait to become readable while * a future is resolving. If your future has sockets which can be diff --git a/src/future/FutureProxy.php b/src/future/FutureProxy.php index 65cbffa2..77c8c5bb 100644 --- a/src/future/FutureProxy.php +++ b/src/future/FutureProxy.php @@ -63,6 +63,14 @@ abstract class FutureProxy extends Future { return $this; } + protected function getServiceProfilerStartParameters() { + return $this->getProxiedFuture()->getServiceProfilerStartParameters(); + } + + protected function getServiceProfilerResultParameters() { + return $this->getProxiedFuture()->getServiceProfilerResultParameters(); + } + abstract protected function didReceiveResult($result); } diff --git a/src/future/exec/ExecFuture.php b/src/future/exec/ExecFuture.php index df8ce772..afb0fd14 100644 --- a/src/future/exec/ExecFuture.php +++ b/src/future/exec/ExecFuture.php @@ -530,24 +530,13 @@ final class ExecFuture extends PhutilExecutableFuture { * @task internal */ public function isReady() { - // NOTE: We have soft dependencies on PhutilServiceProfiler and - // PhutilErrorTrap here. These dependencies are soft to avoid the need to - // build them into the Phage agent. Under normal circumstances, these - // classes are always available. + // NOTE: We have a soft dependencies on PhutilErrorTrap here, to avoid + // the need to build it into the Phage agent. Under normal circumstances, + // this class are always available. if (!$this->pipes) { $is_windows = phutil_is_windows(); - // NOTE: See note above about Phage. - if (class_exists('PhutilServiceProfiler')) { - $profiler = PhutilServiceProfiler::getInstance(); - $this->profilerCallID = $profiler->beginServiceCall( - array( - 'type' => 'exec', - 'command' => phutil_string_cast($this->getCommand()), - )); - } - if (!$this->start) { // We might already have started the timer via initiating resolution. $this->start = microtime(true); @@ -843,23 +832,6 @@ final class ExecFuture extends PhutilExecutableFuture { unset($this->windowsStdoutTempFile); unset($this->windowsStderrTempFile); - - if ($this->profilerCallID !== null) { - if ($this->hasResult()) { - $result = $this->getResult(); - $err = idx($result, 0); - } else { - $err = null; - } - - $profiler = PhutilServiceProfiler::getInstance(); - $profiler->endServiceCall( - $this->profilerCallID, - array( - 'err' => $err, - )); - $this->profilerCallID = null; - } } @@ -958,4 +930,25 @@ final class ExecFuture extends PhutilExecutableFuture { } } + protected function getServiceProfilerStartParameters() { + return array( + 'type' => 'exec', + 'command' => phutil_string_cast($this->getCommand()), + ); + } + + protected function getServiceProfilerResultParameters() { + if ($this->hasResult()) { + $result = $this->getResult(); + $err = idx($result, 0); + } else { + $err = null; + } + + return array( + 'err' => $err, + ); + } + + } diff --git a/src/future/exec/PhutilExecPassthru.php b/src/future/exec/PhutilExecPassthru.php index 8a761923..44eef97e 100644 --- a/src/future/exec/PhutilExecPassthru.php +++ b/src/future/exec/PhutilExecPassthru.php @@ -34,14 +34,6 @@ final class PhutilExecPassthru extends PhutilExecutableFuture { public function execute() { $command = $this->getCommand(); - $profiler = PhutilServiceProfiler::getInstance(); - $call_id = $profiler->beginServiceCall( - array( - 'type' => 'exec', - 'subtype' => 'passthru', - 'command' => $command, - )); - $spec = array(STDIN, STDOUT, STDERR); $pipes = array(); @@ -85,12 +77,6 @@ final class PhutilExecPassthru extends PhutilExecutableFuture { $err = proc_close($proc); - $profiler->endServiceCall( - $call_id, - array( - 'err' => $err, - )); - return $err; } @@ -111,4 +97,26 @@ final class PhutilExecPassthru extends PhutilExecutableFuture { return true; } + + + protected function getServiceProfilerStartParameters() { + return array( + 'type' => 'exec', + 'subtype' => 'passthru', + 'command' => phutil_string_cast($this->getCommand()), + ); + } + + protected function getServiceProfilerResultParameters() { + if ($this->hasResult()) { + $err = $this->getResult(); + } else { + $err = null; + } + + return array( + 'err' => $err, + ); + } + } diff --git a/src/future/http/HTTPFuture.php b/src/future/http/HTTPFuture.php index 738cde27..91dd709a 100644 --- a/src/future/http/HTTPFuture.php +++ b/src/future/http/HTTPFuture.php @@ -123,13 +123,6 @@ final class HTTPFuture extends BaseHTTPFuture { if (!$this->socket) { return $this->stateReady; } - - $profiler = PhutilServiceProfiler::getInstance(); - $this->profilerCallID = $profiler->beginServiceCall( - array( - 'type' => 'http', - 'uri' => $this->getURI(), - )); } if (!$this->stateConnected) { @@ -225,9 +218,6 @@ final class HTTPFuture extends BaseHTTPFuture { $this->setResult($this->parseRawHTTPResponse($this->response)); } - $profiler = PhutilServiceProfiler::getInstance(); - $profiler->endServiceCall($this->profilerCallID, array()); - return true; } @@ -303,4 +293,11 @@ final class HTTPFuture extends BaseHTTPFuture { $data; } + protected function getServiceProfilerStartParameters() { + return array( + 'type' => 'http', + 'uri' => phutil_string_cast($this->getURI()), + ); + } + } diff --git a/src/future/http/HTTPSFuture.php b/src/future/http/HTTPSFuture.php index f65af3e8..70804332 100644 --- a/src/future/http/HTTPSFuture.php +++ b/src/future/http/HTTPSFuture.php @@ -211,13 +211,9 @@ final class HTTPSFuture extends BaseHTTPFuture { $uri_object = new PhutilURI($uri); $proxy = PhutilHTTPEngineExtension::buildHTTPProxyURI($uri_object); - $profiler = PhutilServiceProfiler::getInstance(); - $this->profilerCallID = $profiler->beginServiceCall( - array( - 'type' => 'http', - 'uri' => $uri, - 'proxy' => (string)$proxy, - )); + // TODO: Currently, the "proxy" is not passed to the ServiceProfiler + // because of changes to how ServiceProfiler is integrated. It would + // be nice to pass it again. if (!self::$multi) { self::$multi = curl_multi_init(); @@ -526,9 +522,6 @@ final class HTTPSFuture extends BaseHTTPFuture { } } - $profiler = PhutilServiceProfiler::getInstance(); - $profiler->endServiceCall($this->profilerCallID, array()); - return true; } @@ -821,4 +814,11 @@ final class HTTPSFuture extends BaseHTTPFuture { return ($this->downloadPath !== null); } + protected function getServiceProfilerStartParameters() { + return array( + 'type' => 'http', + 'uri' => phutil_string_cast($this->getURI()), + ); + } + }