From 68204d792be18ce554bc53404d9914d3e79e05dd Mon Sep 17 00:00:00 2001 From: Cole White Date: Fri, 16 Feb 2024 22:32:32 +0000 Subject: [PATCH] editpage: track edit conflicts in edit failures stat Begins tracking edit conflicts in edit_failure_total. Begins tracking edit conflicts resolved in edit_failure_resolved_total. Enables TextConflictHelper to use both StatsFactory and IBufferingStatsdDataFactory instances for backwards compatibility with TwoColConflict extension. Bug: T356812 Change-Id: I1eaeb4a62ea6e07d24f224456e6abb10fcd6e591 --- RELEASE-NOTES-1.42 | 3 + includes/editpage/EditPage.php | 4 +- includes/editpage/TextConflictHelper.php | 98 ++++++++++++++++++------ 3 files changed, 80 insertions(+), 25 deletions(-) diff --git a/RELEASE-NOTES-1.42 b/RELEASE-NOTES-1.42 index a746dfdad00..6966d1e96cb 100644 --- a/RELEASE-NOTES-1.42 +++ b/RELEASE-NOTES-1.42 @@ -662,6 +662,9 @@ because of Phabricator reports. has been hard deprecated * IMaintainableDatabase::truncate() has been deprecated. Use truncateTable() instead. +* TextConflictHelper->incrementStatsByUserEdits() is now deprecated. The + action this function previously handled should be moved into + incrementConflictStats() and incrementResolvedStats(). * … === Other changes in 1.42 === diff --git a/includes/editpage/EditPage.php b/includes/editpage/EditPage.php index 8bdaab05840..cb8f78985c3 100644 --- a/includes/editpage/EditPage.php +++ b/includes/editpage/EditPage.php @@ -4115,6 +4115,8 @@ class EditPage implements IEditObject { MediaWikiServices::getInstance()->getStatsFactory() ->getCounter( 'edit_failure_total' ) ->setLabel( 'cause', $failureType ) + ->setLabel( 'namespace', 'n/a' ) + ->setLabel( 'user_bucket', 'n/a' ) ->copyToStatsdAt( 'edit.failures.' . $failureType ) ->increment(); } @@ -4599,7 +4601,7 @@ class EditPage implements IEditObject { $this->editConflictHelper = new TextConflictHelper( $this->getTitle(), $this->getContext()->getOutput(), - MediaWikiServices::getInstance()->getStatsdDataFactory(), + MediaWikiServices::getInstance()->getStatsFactory(), $label, MediaWikiServices::getInstance()->getContentHandlerFactory() ); diff --git a/includes/editpage/TextConflictHelper.php b/includes/editpage/TextConflictHelper.php index 3862b305502..90ffdd127ae 100644 --- a/includes/editpage/TextConflictHelper.php +++ b/includes/editpage/TextConflictHelper.php @@ -29,6 +29,7 @@ use MediaWiki\Output\OutputPage; use MediaWiki\Title\Title; use MediaWiki\User\User; use MWUnknownContentModelException; +use Wikimedia\Stats\StatsFactory; /** * Helper for displaying edit conflicts in text content models to users @@ -59,7 +60,7 @@ class TextConflictHelper { protected $out; /** - * @var IBufferingStatsdDataFactory + * @var IBufferingStatsdDataFactory|StatsFactory */ protected $stats; @@ -86,14 +87,14 @@ class TextConflictHelper { /** * @param Title $title * @param OutputPage $out - * @param IBufferingStatsdDataFactory $stats + * @param IBufferingStatsdDataFactory|StatsFactory $stats * @param string $submitLabel * @param IContentHandlerFactory $contentHandlerFactory Required param with legacy support * * @throws MWUnknownContentModelException */ public function __construct( - Title $title, OutputPage $out, IBufferingStatsdDataFactory $stats, $submitLabel, + Title $title, OutputPage $out, $stats, $submitLabel, IContentHandlerFactory $contentHandlerFactory ) { $this->title = $title; @@ -136,18 +137,36 @@ class TextConflictHelper { * @param User|null $user */ public function incrementConflictStats( User $user = null ) { - $this->stats->increment( 'edit.failures.conflict' ); + $namespace = 'n/a'; + $userBucket = 'n/a'; + $statsdMetrics = [ 'edit.failures.conflict' ]; + // Only include 'standard' namespaces to avoid creating unknown numbers of statsd metrics if ( $this->title->getNamespace() >= NS_MAIN && $this->title->getNamespace() <= NS_CATEGORY_TALK ) { - $this->stats->increment( - 'edit.failures.conflict.byNamespaceId.' . $this->title->getNamespace() - ); + // getNsText() returns empty string if getNamespace() === NS_MAIN + $namespace = $this->title->getNsText() ?: 'Main'; + $statsdMetrics[] = 'edit.failures.conflict.byNamespaceId.' . $this->title->getNamespace(); } if ( $user ) { - $this->incrementStatsByUserEdits( $user->getEditCount(), 'edit.failures.conflict' ); + $userBucket = $this->getUserBucket( $user->getEditCount() ); + $statsdMetrics[] = 'edit.failures.conflict.byUserEdits.' . $userBucket; + } + if ( $this->stats instanceof StatsFactory ) { + $this->stats->getCounter( 'edit_failure_total' ) + ->setLabel( 'cause', 'conflict' ) + ->setLabel( 'namespace', $namespace ) + ->setLabel( 'user_bucket', $userBucket ) + ->copyToStatsdAt( $statsdMetrics ) + ->increment(); + } + + if ( $this->stats instanceof IBufferingStatsdDataFactory ) { + foreach ( $statsdMetrics as $metric ) { + $this->stats->increment( $metric ); + } } } @@ -156,41 +175,72 @@ class TextConflictHelper { * @param User|null $user */ public function incrementResolvedStats( User $user = null ) { - $this->stats->increment( 'edit.failures.conflict.resolved' ); + $namespace = 'n/a'; + $userBucket = 'n/a'; + $statsdMetrics = [ 'edit.failures.conflict.resolved' ]; + // Only include 'standard' namespaces to avoid creating unknown numbers of statsd metrics if ( $this->title->getNamespace() >= NS_MAIN && $this->title->getNamespace() <= NS_CATEGORY_TALK ) { - $this->stats->increment( - 'edit.failures.conflict.resolved.byNamespaceId.' . $this->title->getNamespace() - ); + // getNsText() returns empty string if getNamespace() === NS_MAIN + $namespace = $this->title->getNsText() ?: 'Main'; + $statsdMetrics[] = 'edit.failures.conflict.resolved.byNamespaceId.' . $this->title->getNamespace(); } + if ( $user ) { - $this->incrementStatsByUserEdits( - $user->getEditCount(), - 'edit.failures.conflict.resolved' - ); + $userBucket = $this->getUserBucket( $user->getEditCount() ); + $statsdMetrics[] = 'edit.failures.conflict.resolved.byUserEdits.' . $userBucket; + } + + if ( $this->stats instanceof StatsFactory ) { + $this->stats->getCounter( 'edit_failure_resolved_total' ) + ->setLabel( 'cause', 'conflict' ) + ->setLabel( 'namespace', $namespace ) + ->setLabel( 'user_bucket', $userBucket ) + ->copyToStatsdAt( $statsdMetrics ) + ->increment(); + } + + if ( $this->stats instanceof IBufferingStatsdDataFactory ) { + foreach ( $statsdMetrics as $metric ) { + $this->stats->increment( $metric ); + } + } + } + + /** + * Retained temporarily for backwards-compatibility. + * + * This action should be moved into incrementConflictStats, incrementResolvedStats. + * + * @deprecated since 1.42, do not use + * @param int|null $userEdits + * @param string $keyPrefixBase + */ + protected function incrementStatsByUserEdits( $userEdits, $keyPrefixBase ) { + if ( $this->stats instanceof IBufferingStatsdDataFactory ) { + $this->stats->increment( $keyPrefixBase . '.byUserEdits.' . $this->getUserBucket( $userEdits ) ); } } /** * @param int|null $userEdits - * @param string $keyPrefixBase + * @return string */ - protected function incrementStatsByUserEdits( $userEdits, $keyPrefixBase ) { + protected function getUserBucket( ?int $userEdits ): string { if ( $userEdits === null ) { - $userBucket = 'anon'; + return 'anon'; } elseif ( $userEdits > 200 ) { - $userBucket = 'over200'; + return 'over200'; } elseif ( $userEdits > 100 ) { - $userBucket = 'over100'; + return 'over100'; } elseif ( $userEdits > 10 ) { - $userBucket = 'over10'; + return 'over10'; } else { - $userBucket = 'under11'; + return 'under11'; } - $this->stats->increment( $keyPrefixBase . '.byUserEdits.' . $userBucket ); } /**