From c3b2184977f50bbd9461637963d91825fc949393 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 1 Jul 2013 12:37:34 -0700 Subject: [PATCH] Mostly modernize Conduit logs Summary: - Add GC support to conduit logs. - Add Query support to conduit logs. - Record the actual user PHID. - Show client name. - Support querying by specific method, so I can link to this from a setup issue. @wez, this migration may not be fast. It took about 8 seconds for me to migrate 800,000 rows in the `conduit_methodcalllog` table. This adds a GC which should keep the table at a more manageable size in the future. You can safely delete all data older than 30 days from this table, although you should do it by `id` instead of `dateCreated` since there's no key on `dateCreated` until this patch. Test Plan: - Ran GC. - Looked at log UI. - Ran Conduit methods. Reviewers: btrahan Reviewed By: btrahan CC: wez, aran Differential Revision: https://secure.phabricator.com/D6332 --- resources/sql/patches/20130701.conduitlog.sql | 14 ++++ src/__phutil_library_map__.php | 8 +- .../PhabricatorConduitAPIController.php | 22 ++--- .../PhabricatorConduitLogController.php | 81 +++++++++++++------ .../query/PhabricatorConduitLogQuery.php | 43 ++++++++++ .../conduit/ssh/ConduitSSHWorkflow.php | 13 +-- .../PhabricatorConduitMethodCallLog.php | 22 ++++- ...abricatorGarbageCollectorConfigOptions.php | 3 + .../PhabricatorGarbageCollectorDaemon.php | 42 ++++++++++ .../patch/PhabricatorBuiltinPatchList.php | 4 + 10 files changed, 209 insertions(+), 43 deletions(-) create mode 100644 resources/sql/patches/20130701.conduitlog.sql create mode 100644 src/applications/conduit/query/PhabricatorConduitLogQuery.php diff --git a/resources/sql/patches/20130701.conduitlog.sql b/resources/sql/patches/20130701.conduitlog.sql new file mode 100644 index 0000000000..2a0a9420ce --- /dev/null +++ b/resources/sql/patches/20130701.conduitlog.sql @@ -0,0 +1,14 @@ +ALTER TABLE {$NAMESPACE}_conduit.conduit_methodcalllog + ADD callerPHID VARCHAR(64); + +ALTER TABLE {$NAMESPACE}_conduit.conduit_methodcalllog + ADD KEY `key_created` (dateCreated); + +ALTER TABLE {$NAMESPACE}_conduit.conduit_methodcalllog + ADD KEY `key_method` (method); + +ALTER TABLE {$NAMESPACE}_conduit.conduit_methodcalllog + ADD KEY `key_callermethod` (callerPHID, method); + +ALTER TABLE {$NAMESPACE}_conduit.conduit_connectionlog + ADD KEY `key_created` (dateCreated); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0f60bc2ddd..3401f13f69 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -920,6 +920,7 @@ phutil_register_library_map(array( 'PhabricatorConduitDAO' => 'applications/conduit/storage/PhabricatorConduitDAO.php', 'PhabricatorConduitListController' => 'applications/conduit/controller/PhabricatorConduitListController.php', 'PhabricatorConduitLogController' => 'applications/conduit/controller/PhabricatorConduitLogController.php', + 'PhabricatorConduitLogQuery' => 'applications/conduit/query/PhabricatorConduitLogQuery.php', 'PhabricatorConduitMethodCallLog' => 'applications/conduit/storage/PhabricatorConduitMethodCallLog.php', 'PhabricatorConduitMethodQuery' => 'applications/conduit/query/PhabricatorConduitMethodQuery.php', 'PhabricatorConduitSearchEngine' => 'applications/conduit/query/PhabricatorConduitSearchEngine.php', @@ -2820,7 +2821,12 @@ phutil_register_library_map(array( 1 => 'PhabricatorApplicationSearchResultsControllerInterface', ), 'PhabricatorConduitLogController' => 'PhabricatorConduitController', - 'PhabricatorConduitMethodCallLog' => 'PhabricatorConduitDAO', + 'PhabricatorConduitLogQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorConduitMethodCallLog' => + array( + 0 => 'PhabricatorConduitDAO', + 1 => 'PhabricatorPolicyInterface', + ), 'PhabricatorConduitMethodQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorConduitSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorConduitTokenController' => 'PhabricatorConduitController', diff --git a/src/applications/conduit/controller/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/PhabricatorConduitAPIController.php index afa1823dc4..37f9505d19 100644 --- a/src/applications/conduit/controller/PhabricatorConduitAPIController.php +++ b/src/applications/conduit/controller/PhabricatorConduitAPIController.php @@ -124,18 +124,18 @@ final class PhabricatorConduitAPIController $connection_id = idx($result, 'connectionID'); } - $log->setConnectionID($connection_id); - $log->setError((string)$error_code); - $log->setDuration(1000000 * ($time_end - $time_start)); + $log + ->setCallerPHID( + isset($conduit_user) + ? $conduit_user->getPHID() + : null) + ->setConnectionID($connection_id) + ->setError((string)$error_code) + ->setDuration(1000000 * ($time_end - $time_start)); - // TODO: This is a hack, but the insert is comparatively expensive and - // we only really care about having these logs for real CLI clients, if - // even that. - if (empty($metadata['authToken'])) { - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $log->save(); - unset($unguarded); - } + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $log->save(); + unset($unguarded); $response = id(new ConduitAPIResponse()) ->setResult($result) diff --git a/src/applications/conduit/controller/PhabricatorConduitLogController.php b/src/applications/conduit/controller/PhabricatorConduitLogController.php index dd6b67db87..1c235d8d8c 100644 --- a/src/applications/conduit/controller/PhabricatorConduitLogController.php +++ b/src/applications/conduit/controller/PhabricatorConduitLogController.php @@ -8,24 +8,26 @@ final class PhabricatorConduitLogController public function processRequest() { $request = $this->getRequest(); + $viewer = $request->getUser(); $conn_table = new PhabricatorConduitConnectionLog(); $call_table = new PhabricatorConduitMethodCallLog(); $conn_r = $call_table->establishConnection('r'); - $pager = new AphrontPagerView(); - $pager->setOffset($request->getInt('page')); - $calls = $call_table->loadAllWhere( - '1 = 1 ORDER BY id DESC LIMIT %d, %d', - $pager->getOffset(), - $pager->getPageSize() + 1); - $calls = $pager->sliceResults($calls); - $pager->setURI(new PhutilURI('/conduit/log/'), 'page'); - $pager->setEnableKeyboardShortcuts(true); + $pager = new AphrontCursorPagerView(); + $pager->readFromRequest($request); + $pager->setPageSize(500); - $min = $pager->getOffset() + 1; - $max = ($min + count($calls) - 1); + $query = id(new PhabricatorConduitLogQuery()) + ->setViewer($viewer); + + $methods = $request->getStrList('methods'); + if ($methods) { + $query->withMethods($methods); + } + + $calls = $query->executeWithCursorPager($pager); $conn_ids = array_filter(mpull($calls, 'getConnectionID')); $conns = array(); @@ -36,10 +38,6 @@ final class PhabricatorConduitLogController } $table = $this->renderCallTable($calls, $conns); - $panel = new AphrontPanelView(); - $panel->setHeader('Conduit Method Calls ('.$min.'-'.$max.')'); - $panel->appendChild($table); - $panel->appendChild($pager); $crumbs = $this->buildApplicationCrumbs(); $crumbs->addCrumb( @@ -49,10 +47,13 @@ final class PhabricatorConduitLogController return $this->buildApplicationPage( array( $crumbs, - $panel, + $table, + $pager, ), array( 'title' => 'Conduit Logs', + 'device' => true, + 'dust' => true, )); } @@ -60,30 +61,61 @@ final class PhabricatorConduitLogController assert_instances_of($calls, 'PhabricatorConduitMethodCallLog'); assert_instances_of($conns, 'PhabricatorConduitConnectionLog'); - $user = $this->getRequest()->getUser(); + $viewer = $this->getRequest()->getUser(); + + $methods = id(new PhabricatorConduitMethodQuery()) + ->setViewer($viewer) + ->execute(); + $methods = mpull($methods, null, 'getAPIMethodName'); $rows = array(); foreach ($calls as $call) { $conn = idx($conns, $call->getConnectionID()); - if (!$conn) { - // If there's no connection, use an empty object. - $conn = new PhabricatorConduitConnectionLog(); + if ($conn) { + $name = $conn->getUserName(); + $client = ' (via '.$conn->getClient().')'; + } else { + $name = null; + $client = null; } + + $method = idx($methods, $call->getMethod()); + if ($method) { + switch ($method->getMethodStatus()) { + case ConduitAPIMethod::METHOD_STATUS_STABLE: + $status = null; + break; + case ConduitAPIMethod::METHOD_STATUS_UNSTABLE: + $status = pht('Unstable'); + break; + case ConduitAPIMethod::METHOD_STATUS_DEPRECATED: + $status = pht('Deprecated'); + break; + } + } else { + $status = pht('Unknown'); + } + $rows[] = array( $call->getConnectionID(), - $conn->getUserName(), - $call->getMethod(), + $name, + array($call->getMethod(), $client), + $status, $call->getError(), number_format($call->getDuration()).' us', - phabricator_datetime($call->getDateCreated(), $user), + phabricator_datetime($call->getDateCreated(), $viewer), ); } - $table = new AphrontTableView($rows); + + $table = id(new AphrontTableView($rows)) + ->setDeviceReadyTable(true); + $table->setHeaders( array( 'Connection', 'User', 'Method', + 'Status', 'Error', 'Duration', 'Date', @@ -94,6 +126,7 @@ final class PhabricatorConduitLogController '', 'wide', '', + '', 'n', 'right', )); diff --git a/src/applications/conduit/query/PhabricatorConduitLogQuery.php b/src/applications/conduit/query/PhabricatorConduitLogQuery.php new file mode 100644 index 0000000000..092f9d59bc --- /dev/null +++ b/src/applications/conduit/query/PhabricatorConduitLogQuery.php @@ -0,0 +1,43 @@ +methods = $methods; + return $this; + } + + + public function loadPage() { + $table = new PhabricatorConduitMethodCallLog(); + $conn_r = $table->establishConnection('r'); + + $data = queryfx_all( + $conn_r, + 'SELECT * FROM %T %Q %Q %Q', + $table->getTableName(), + $this->buildWhereClause($conn_r), + $this->buildOrderClause($conn_r), + $this->buildLimitClause($conn_r)); + + return $table->loadAllFromArray($data);; + } + + private function buildWhereClause(AphrontDatabaseConnection $conn_r) { + $where = array(); + + if ($this->methods) { + $where[] = qsprintf( + $conn_r, + 'method IN (%Ls)', + $this->methods); + } + + $where[] = $this->buildPagingClause($conn_r); + return $this->formatWhereClause($where); + } + +} diff --git a/src/applications/conduit/ssh/ConduitSSHWorkflow.php b/src/applications/conduit/ssh/ConduitSSHWorkflow.php index 79345d683a..ba456efaca 100644 --- a/src/applications/conduit/ssh/ConduitSSHWorkflow.php +++ b/src/applications/conduit/ssh/ConduitSSHWorkflow.php @@ -72,11 +72,12 @@ final class ConduitSSHWorkflow extends PhabricatorSSHWorkflow { $time_end = microtime(true); $connection_id = idx($metadata, 'connectionID'); - $log = new PhabricatorConduitMethodCallLog(); - $log->setConnectionID($connection_id); - $log->setMethod($method); - $log->setError((string)$error_code); - $log->setDuration(1000000 * ($time_end - $time_start)); - $log->save(); + $log = id(new PhabricatorConduitMethodCallLog()) + ->setCallerPHID($this->getUser()->getPHID()) + ->setConnectionID($connection_id) + ->setMethod($method) + ->setError((string)$error_code) + ->setDuration(1000000 * ($time_end - $time_start)) + ->save(); } } diff --git a/src/applications/conduit/storage/PhabricatorConduitMethodCallLog.php b/src/applications/conduit/storage/PhabricatorConduitMethodCallLog.php index e796e3bf58..8edbaac7b9 100644 --- a/src/applications/conduit/storage/PhabricatorConduitMethodCallLog.php +++ b/src/applications/conduit/storage/PhabricatorConduitMethodCallLog.php @@ -3,11 +3,31 @@ /** * @group conduit */ -final class PhabricatorConduitMethodCallLog extends PhabricatorConduitDAO { +final class PhabricatorConduitMethodCallLog extends PhabricatorConduitDAO + implements PhabricatorPolicyInterface { + protected $callerPHID; protected $connectionID; protected $method; protected $error; protected $duration; + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + + public function getCapabilities() { + return array( + PhabricatorPolicyCapability::CAN_VIEW, + ); + } + + public function getPolicy($capability) { + return PhabricatorPolicies::POLICY_USER; + } + + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + return false; + } + } diff --git a/src/applications/config/option/PhabricatorGarbageCollectorConfigOptions.php b/src/applications/config/option/PhabricatorGarbageCollectorConfigOptions.php index 2bacb46857..3f5a42c624 100644 --- a/src/applications/config/option/PhabricatorGarbageCollectorConfigOptions.php +++ b/src/applications/config/option/PhabricatorGarbageCollectorConfigOptions.php @@ -32,6 +32,9 @@ final class PhabricatorGarbageCollectorConfigOptions 'gcdaemon.ttl.general-cache' => array( 30, pht('Number of seconds to retain general cache entries for.')), + 'gcdaemon.ttl.conduit-logs' => array( + 180, + pht('Number of seconds to retain Conduit call logs for.')) ); $result = array(); diff --git a/src/infrastructure/daemon/PhabricatorGarbageCollectorDaemon.php b/src/infrastructure/daemon/PhabricatorGarbageCollectorDaemon.php index dc736e8982..4e9bfda28a 100644 --- a/src/infrastructure/daemon/PhabricatorGarbageCollectorDaemon.php +++ b/src/infrastructure/daemon/PhabricatorGarbageCollectorDaemon.php @@ -18,6 +18,8 @@ final class PhabricatorGarbageCollectorDaemon extends PhabricatorDaemon { $n_cache_ttl = $this->collectGeneralCacheTTL(); $n_cache = $this->collectGeneralCaches(); $n_files = $this->collectExpiredFiles(); + $n_clogs = $this->collectExpiredConduitLogs(); + $n_ccons = $this->collectExpiredConduitConnections(); $collected = array( 'Herald Transcript' => $n_herald, @@ -28,6 +30,8 @@ final class PhabricatorGarbageCollectorDaemon extends PhabricatorDaemon { 'General Cache TTL' => $n_cache_ttl, 'General Cache Entries' => $n_cache, 'Temporary Files' => $n_files, + 'Conduit Logs' => $n_clogs, + 'Conduit Connections' => $n_ccons, ); $collected = array_filter($collected); @@ -219,4 +223,42 @@ final class PhabricatorGarbageCollectorDaemon extends PhabricatorDaemon { return count($files); } + private function collectExpiredConduitLogs() { + $key = 'gcdaemon.ttl.conduit-logs'; + $ttl = PhabricatorEnv::getEnvConfig($key); + if ($ttl <= 0) { + return 0; + } + + $table = new PhabricatorConduitMethodCallLog(); + $conn_w = $table->establishConnection('w'); + queryfx( + $conn_w, + 'DELETE FROM %T WHERE dateCreated < %d + ORDER BY dateCreated ASC LIMIT 100', + $table->getTableName(), + time() - $ttl); + + return $conn_w->getAffectedRows(); + } + + private function collectExpiredConduitConnections() { + $key = 'gcdaemon.ttl.conduit-logs'; + $ttl = PhabricatorEnv::getEnvConfig($key); + if ($ttl <= 0) { + return 0; + } + + $table = new PhabricatorConduitConnectionLog(); + $conn_w = $table->establishConnection('w'); + queryfx( + $conn_w, + 'DELETE FROM %T WHERE dateCreated < %d + ORDER BY dateCreated ASC LIMIT 100', + $table->getTableName(), + time() - $ttl); + + return $conn_w->getAffectedRows(); + } + } diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index 93df6d893e..9c6bdb8027 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1410,6 +1410,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('20130628.legalpadv0.sql'), ), + '20130701.conduitlog.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('20130701.conduitlog.sql'), + ), ); } }