-
-
Notifications
You must be signed in to change notification settings - Fork 48
Track metrics related to dblist generation #790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |
| use Wikimedia\ObjectCache\BagOStuff; | ||
| use Wikimedia\Rdbms\IReadableDatabase; | ||
| use Wikimedia\StaticArrayWriter; | ||
| use Wikimedia\Stats\StatsFactory; | ||
| use function array_keys; | ||
| use function file_put_contents; | ||
| use function function_exists; | ||
|
|
@@ -28,6 +29,7 @@ class CreateWikiDataStore { | |
| public const CONSTRUCTOR_OPTIONS = [ | ||
| ConfigNames::CacheDirectory, | ||
| ConfigNames::CacheType, | ||
| ConfigNames::TrackDatabaseListMetrics, | ||
| MainConfigNames::CacheDirectory, | ||
| MainConfigNames::LocalDatabases, | ||
| ]; | ||
|
|
@@ -41,9 +43,11 @@ class CreateWikiDataStore { | |
| private readonly string $cacheDir; | ||
| private int $timestamp; | ||
| private int $localServerTimestamp; | ||
| private readonly bool $trackDatabaseListMetrics; | ||
|
|
||
| public function __construct( | ||
| ObjectCacheFactory $objectCacheFactory, | ||
| private readonly StatsFactory $statsFactory, | ||
| private readonly CreateWikiDatabaseUtils $databaseUtils, | ||
| private readonly CreateWikiHookRunner $hookRunner, | ||
| private readonly ServiceOptions $options | ||
|
|
@@ -64,6 +68,8 @@ public function __construct( | |
| $this->localServerTimestamp = (int)$this->localServerCache->get( | ||
| $this->localServerCache->makeGlobalKey( self::CACHE_KEY, 'databases-local' ) | ||
| ); | ||
|
|
||
| $this->trackDatabaseListMetrics = $this->options->get( ConfigNames::TrackDatabaseListMetrics ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't really need to do a property here and just use the option directly since its only used once. |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -73,7 +79,7 @@ public function __construct( | |
| */ | ||
| public function syncCache(): void { | ||
| if ( !$this->timestamp ) { | ||
| $this->resetDatabaseLists( isNewChanges: true ); | ||
| $this->resetDatabaseLists( isNewChanges: true, caller: __METHOD__ ); | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -86,7 +92,7 @@ public function syncCache(): void { | |
| // Only regenerate if localServerTimestamp is smaller than timestamp so multiple processes on the | ||
| // same server don't try regenerating the dblists at the same time. | ||
| if ( ( $mtime === 0 || $mtime < $this->timestamp ) && $this->localServerTimestamp < $this->timestamp ) { | ||
| $this->resetDatabaseLists( isNewChanges: false ); | ||
| $this->resetDatabaseLists( isNewChanges: false, caller: __METHOD__ ); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -105,7 +111,20 @@ public function getAllDatabases(): array { | |
| * the updated list to a PHP file within the cache directory. It also updates the | ||
| * modification time (mtime) and stores it in the cache for future reference. | ||
| */ | ||
| public function resetDatabaseLists( bool $isNewChanges ): void { | ||
| // @phan-suppress-next-line PhanDisallowedOptionalMethodParameter | ||
| public function resetDatabaseLists( bool $isNewChanges, ?string $caller = null ): void { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No optional params please. Explicitly pass empty string and check where we dont want it. Easier to read at call sites as well that way. Though really it should just always be passed as |
||
| $timer = null; | ||
| if ( $this->trackDatabaseListMetrics ) { | ||
| /** @phan-suppress-next-line PhanPossiblyUndeclaredMethod */ | ||
| $this->statsFactory->getCounter( 'createwiki_dblist_generation_total' ) | ||
| ->setLabel( 'cause', $caller ?? 'unknown' ) | ||
| ->setLabel( 'new_changes', $isNewChanges ? 'Yes' : 'No' ) | ||
| ->increment(); | ||
|
|
||
| $timer = $this->statsFactory->getTiming( 'createwiki_dblist_generation_seconds' ) | ||
| ->start(); | ||
| } | ||
|
|
||
| $mtime = time(); | ||
| $this->localServerCache->set( | ||
| $this->localServerCache->makeGlobalKey( self::CACHE_KEY, 'databases-local' ), | ||
|
|
@@ -132,6 +151,7 @@ public function resetDatabaseLists( bool $isNewChanges ): void { | |
| $this->writeToFile( $name, $list ); | ||
| } | ||
|
|
||
| $timer?->stop(); | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -171,6 +191,7 @@ public function resetDatabaseLists( bool $isNewChanges ): void { | |
| ]; | ||
|
|
||
| $this->writeToFile( 'databases', $list ); | ||
| $timer?->stop(); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alphabetical order please.