Stats: clarify TimingMetric::observe() units and discourage in new code

In the previous implementation with statsd/Graphite, the actual unit of
what was value did, defacto, not matter. Seconds and even bytes worked
just fine. However, with Prometheus it is essential to store timings
with the correct units, because the buckets for the histogram are
based on absolute thresholds.

While there exist `observeSeconds` and `observeNanoseconds` helper
methods on this class, the main method promoted today is `observe()`
which was missing essential information in its name or doc about what
unit to provide. This is made worse by the fact that all documentation
and migration instructions emphasize that the metric should be named
with a `_seconds` suffix, and reference Prometheus documentation which
explicitly states that SI units like seconds should be stored.

This disconnect caused T383208 and will keep being a source of
confusion and bugs as long as this method is promoted.

Ideally, this would have been part of Ieb24a5f723 (commit 37d29e8),
but second best time to fix this is now.

Bug: T364240
Change-Id: Ie92fa7897085bef55b37b02ee4ddba774dc2ed9c

1 files changed, 60 insertions, 31 deletions

diff --git a/includes/libs/Stats/Metrics/TimingMetric.php b/includes/libs/Stats/Metrics/TimingMetric.php
index 7deaa377d78f..5dd1a7d5ed8e 100644
--- a/includes/libs/Stats/Metrics/TimingMetric.php
+++ b/includes/libs/Stats/Metrics/TimingMetric.php
@@ -47,6 +47,15 @@ class TimingMetric implements MetricInterface {
 
 	/**
 	 * Starts a timer.
+	 *
+	 * Example:
+	 *
+	 * ```php
+	 * $timer = StatsFactory->getTiming(…);
+	 * $timer->start();
+	 * # work to be measured...
+	 * $timer->stop();
+	 * ```
 	 */
 	public function start(): void {
 		$this->startTime = hrtime( true );
@@ -65,57 +74,77 @@ class TimingMetric implements MetricInterface {
 	}
 
 	/**
-	 * Records a previously calculated observation in milliseconds.
+	 * Record a previously calculated observation in nanoseconds.
 	 *
-	 * @param float $milliseconds
+	 * Example:
+	 *
+	 * ```php
+	 * $startTime = hrtime( true )
+	 * # work to be measured...
+	 * $metric->observeNanoseconds( hrtime( true ) - $startTime )
+	 * ```
+	 *
+	 * @param float $nanoseconds
 	 * @return void
+	 * @since 1.43
 	 */
-	public function observe( float $milliseconds ): void {
-		foreach ( $this->baseMetric->getStatsdNamespaces() as $namespace ) {
-			$this->baseMetric->getStatsdDataFactory()->timing( $namespace, $milliseconds );
-		}
-
-		try {
-			$this->baseMetric->addSample( new Sample( $this->baseMetric->getLabelValues(), $milliseconds ) );
-		} catch ( IllegalOperationException $ex ) {
-			// Log the condition and give the caller something that will absorb calls.
-			trigger_error( $ex->getMessage(), E_USER_WARNING );
-		}
+	public function observeNanoseconds( float $nanoseconds ): void {
+		$this->addSample( $nanoseconds * 1e-6 );
 	}
 
 	/**
 	 * Record a previously calculated observation in seconds.
 	 *
-	 * Common usage:
-	 *  ```php
-	 *  $startTime = microtime( true )
-	 *  # work to be measured...
-	 *  $metric->observeSeconds( microtime( true ) - $startTime )
-	 *  ```
+	 * This method is provided for tracking externally-generated values, timestamp deltas, and
+	 * situations where the expected input value is the expected Prometheus graphed value.
+	 *
+	 * Performance measurements in process should be done with hrtime() and observeNanoseconds()
+	 * to ensure monotonic time is used and not wall-clock time.
+	 *
+	 * Example:
+	 *
+	 * ```php
+	 * $startTime = microtime( true )
+	 * # work to be measured...
+	 * $metric->observeSeconds( microtime( true ) - $startTime )
+	 * ```
 	 *
 	 * @param float $seconds
 	 * @return void
 	 * @since 1.43
 	 */
 	public function observeSeconds( float $seconds ): void {
-		$this->observe( $seconds * 1000 );
+		$this->addSample( $seconds * 1000 );
 	}
 
 	/**
-	 * Record a previously calculated observation in nanoseconds.
+	 * Record a previously calculated observation in milliseconds.
 	 *
-	 *  Common usage:
-	 *  ```php
-	 *  $startTime = hrtime( true )
-	 *  # work to be measured...
-	 *  $metric->observeNanoseconds( hrtime( true ) - $startTime )
-	 *  ```
-	 * @param float $nanoseconds
+	 * NOTE: You MUST pass values converted to milliseconds.
+	 *
+	 * This method is discouraged in new code, because PHP does not measure
+	 * time in milliseconds. It will be less error-prone if you use start()
+	 * and stop(), or pass values from hrtime() directly to observeNanoseconds()
+	 * without manual multiplication to another unit.
+	 *
+	 * @param float $milliseconds
 	 * @return void
-	 * @since 1.43
 	 */
-	public function observeNanoseconds( float $nanoseconds ): void {
-		$this->observe( $nanoseconds * 1e-6 );
+	public function observe( float $milliseconds ): void {
+		$this->addSample( $milliseconds );
+	}
+
+	private function addSample( float $milliseconds ): void {
+		foreach ( $this->baseMetric->getStatsdNamespaces() as $namespace ) {
+			$this->baseMetric->getStatsdDataFactory()->timing( $namespace, $milliseconds );
+		}
+
+		try {
+			$this->baseMetric->addSample( new Sample( $this->baseMetric->getLabelValues(), $milliseconds ) );
+		} catch ( IllegalOperationException $ex ) {
+			// Log the condition and give the caller something that will absorb calls.
+			trigger_error( $ex->getMessage(), E_USER_WARNING );
+		}
 	}
 
 	/** @inheritDoc */