@colewhite If I understand correctly, this problem is stemming from the fact that calling TimingMetric->start() makes the object (temporarily) stateful in a way that is incompatible with the cache that StatsFactory uses. We could avoid this state by using a closure. Would that work?

For example:

$stop = $stats->getCounter( 'foo_bar')
  ->setLabel( 'this', 'that' )
  ->copyToStatsdAt( 'foo.bar' )
  ->start();

…

$stop();

It could work like this:

	public function start(): void {
		$this->startTime = hrtime( true );
	}

	public function stop(): void {
		if ( $this->startTime === null ) {
			trigger_error( 'Called stop without start' );
			return;
		}
		$value = ( hrtime( true ) - $this->startTime ) * 1e-6;
		$this->observe( $value );
		$this->startTime = null;
	}

	public function start(): Closure {
		$startTime = hrtime( true );
		return function () use ( &$startTime ) {
			if ( $startTime === null ) {
				trigger_error( 'Called stop more than once' );
				return;
			}
			$value = ( hrtime( true ) - $startTime ) * 1e-6;
			$this->observe( $value );
			$startTime = null;
		};
	}

The transition will be a bit tricky, but its not used in many places yet. We could (for 1 week) support both by storing it as $this->startTime like before and supporting both ways of stopping, and then once we've updated callers, remove that part. What do you think?

I've run into this again at https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1139144/1/includes/ResourceLoader/Module.php, since there is one module (the StartupModule) which builds multiple other modules, so the metric is nested. This is fortunately detected and CI fails:

PHP Warning: Stats: stop() called before start() for metric 'resourceloader_build_seconds'

But it'd be great we can make this work somehow.

Change #1146690 had a related patch set uploaded (by Cwhite; author: Cwhite):

[mediawiki/core@master] Stats: Add recursion support to TimingMetric start/stop

https://gerrit.wikimedia.org/r/1146690

Change #1146690 merged by jenkins-bot:

[mediawiki/core@master] Stats: Add recursion support to TimingMetric start/stop

https://gerrit.wikimedia.org/r/1146690

Change #1159546 had a related patch set uploaded (by Cwhite; author: Cwhite):

[mediawiki/core@master] File: reintroduce TimingMetric start/stop pattern

https://gerrit.wikimedia.org/r/1159546

Change #1159547 had a related patch set uploaded (by Cwhite; author: Cwhite):

[mediawiki/core@master] ResourceLoader: reintroduce TimingMetric start/stop pattern

https://gerrit.wikimedia.org/r/1159547

Change #1159546 merged by jenkins-bot:

[mediawiki/core@master] File: reintroduce TimingMetric start/stop pattern

https://gerrit.wikimedia.org/r/1159546

Change #1159547 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: reintroduce TimingMetric start/stop pattern

https://gerrit.wikimedia.org/r/1159547