Change #1048470 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] WIP: InjectRCRecordsJob: Use statslib (StatsFactory)

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

Note to self and others: be very careful not to use the same metric name for two different metrics (probably, each ->getCounter(), ->getGauge() and ->getTiming() call in the code should have a unique literal string argument, except perhaps in cases like T359253). I thought I knew this already (it’s mentioned here and there in the statslib docs), and yet I still accidentally messed this up (see PS2..3 of the above change), and if I hadn’t tested locally that the jobs emit the same metrics before and after statslib migration, I never would’ve noticed that part of the stats was being quietly dropped.

Change #1052737 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] DNM: testing gerrit commit message formatting

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

Change #1052737 abandoned by Lucas Werkmeister (WMDE):

[mediawiki/extensions/Wikibase@master] DNM: testing gerrit commit message formatting

Reason:

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

Oops, disregard the above DNM change – I forgot to remove the Bug line there ^^

Otherwise, assuming CI passes, I think the change should be ready for review now.

Change #1048470 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Use statslib (StatsFactory) in change dispatching

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

Looks like I messed up the time units – immediately after group1 to wmf.14 (8:13 UTC), the metric shot up and has been alerting intermittently since then:

Okay, I think this was actually a bug in the old code? The old code did:

		$this->recordDelay( $change->getAge() );
// ...
	private function recordDelay( int $delay ): void {
		if ( $this->stats ) {
			$this->stats->timing( 'wikibase.client.pageupdates.InjectRCRecords.delay', $delay );

But ChangeRow::getAge() returns seconds, while StatsdDataFactoryInterface::timing() takes milliseconds. And the Grafana dashboard was set up to show the metric as seconds rather than milliseconds.

Can we make Grafana use two different time scales, somehow? Divide the actual metric by 1000 before 8:13 UTC today? Or should we just update Grafana to use milliseconds and leave a note that this shows historical information incorrectly? (I think we could live with that – I don’t think the historical information is super interesting in this case.)

Also, a comment for @lmata or anyone else interested: IMHO this is still pretty confusing in the new system. Manual:Stats (permalink) says:

A metric: […]

• should use base units (e.g. seconds, bytes, meters, etc.)
• should have a suffix describing the unit in plural form. (i.e. _seconds, _bytes, _total, etc.)

And yet, immediately below:

$statsFactory->getTiming('foo')
	->observe( ( microtime( true ) - $startTime ) * 1000 );  # expects ms

What I did in this migration was call the metric _seconds and observe milliseconds, and I hope this is right, but it seems quite confusing. Given that TimingMetric is our own class (and not from any library or standard), could we not call the method observeMilliseconds() (and also have observeSeconds()) to make the unit more obvious?

Also updated two panels on the non-alerts dashboard.

Can we make Grafana use two different time scales, somehow? Divide the actual metric by 1000 before 8:13 UTC today?

Doesn’t sound like it (I asked in wikimedia-observability). In that case I think this can go back into tech verification.

In T359248#9994136, @Lucas_Werkmeister_WMDE wrote:

Also, a comment for @lmata or anyone else interested: IMHO this is still pretty confusing in the new system. Manual:Stats (permalink) says:

A metric: […]

• should use base units (e.g. seconds, bytes, meters, etc.)
• should have a suffix describing the unit in plural form. (i.e. _seconds, _bytes, _total, etc.)

And yet, immediately below:

$statsFactory->getTiming('foo')
	->observe( ( microtime( true ) - $startTime ) * 1000 );  # expects ms

What I did in this migration was call the metric _seconds and observe milliseconds, and I hope this is right, but it seems quite confusing. Given that TimingMetric is our own class (and not from any library or standard), could we not call the method observeMilliseconds() (and also have observeSeconds()) to make the unit more obvious?

This is fantastic feedback, thank you.

I agree - this is something StatsLib should help guide the user to correct use. We have a task and I think it's time we work on it: T364240: Investigate making TimingMetric unit-aware