Page MenuHomePhabricator

Proposal: Introduce IStatsFactory and NullStatsFactory in StatsLib
Closed, ResolvedPublic

Description

Like IBufferingStatsdDataFactory, we need an interface that can be used to type-hint and provide the contract to various StatsFactory objects that can be passed around in live code and tests.

IBufferingStatsdDataFactory is implemented by NullStatsdDataFactory (value object) and BufferingStatsdDataFactory (via MW services) which can be used depending on the scenario at hand. When we're dealing with real metrics that we want to buffer and send over the wire, we get an instance of BufferingStatsdDataFactory (via MW services mostly injected). But sometimes consumer code just needs something to deal with (not a hard requirement - like a NULL value object) and we don't want to pass in a full stats data factory (in places we want a default or don't have access to MW services), we just provide a NULL value object (NullStatsdDataFactory) especially in tests etc.

This task is to provide an interface (proposed name: IStatsFactory) that can be implemented by various concrete use cases for the afore mentioned purpose. BagOStuff for example needs a StatsFactory object injected:

public function __construct( array $params = [] ) {
	$this->keyspace = $params['keyspace'] ?? 'local';
	$this->stats = $params['stats'] ?? new NullStatsFactory();
	$this->setLogger( $params['logger'] ?? new NullLogger() );

	$asyncHandler = $params['asyncHandler'] ?? null;
	if ( is_callable( $asyncHandler ) ) {
		$this->asyncHandler = $asyncHandler;
	}
}

when creating the various caching backends but if not specified in $params, fallback back to a NULL object but we don't yet have this in StatsLib and the NullEmitter won't do the trick or will it? So I propose we introduce an interface for this and have various concrete cases implement it including the null case.

Event Timeline

Change 993701 had a related patch set uploaded (by D3r1ck01; author: Derick Alangi):

[mediawiki/core@master] Stats: Introduce NullStatsFactory and IStatsFactory

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

DAlangi_WMF renamed this task from Introduce IStatsFactory and NullStatsFactory in StatsLib to Proposal: Introduce IStatsFactory and NullStatsFactory in StatsLib.Jan 31 2024, 10:13 AM
DAlangi_WMF updated the task description. (Show Details)

Let me know what you think about this @colewhite.

I'm not sure there's a need for a separate interface per-se, other than than that if we take the approach of a Null class, and if said class is complete and standalone (i.e. not a subclass), then you would indeed want a common interface (or abstract base class).

I'd like to offer a simpler approach for consideration, given that StatsdFactory has internally really good abstraction already. If I understand correclty, it can already be instantiated as a null object, through dependency injection. This would allow you to take an approach similar to what we do with WANObjectCache, which offers WANObjectCache::newEmpty() that creates that for you as a shortcut.

Looking at a few test cases, it seems like the following idiom has emerged as a potential equivalent of this:

new StatsFactory( new StatsCache, new NullEmitter, new NullLogger );

Would this work?

Change 999005 had a related patch set uploaded (by D3r1ck01; author: Derick Alangi):

[mediawiki/core@master] Stats: Introduce factory method to create NULL stats factory

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

Change 993701 abandoned by D3r1ck01:

[mediawiki/core@master] Stats: Introduce NullStatsFactory and IStatsFactory

Reason:

Alternative approach: I9efcce04c127af8633eca792e63b1f0dad6696a0

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

new StatsFactory( new StatsCache, new NullEmitter, new NullLogger );

Would this work?

Yes, I think this is going to work and it's a simpler alternatives. I've also updated existing code to use this method directly. Nice, thank you!

Adding a helper static function looks like a great proposal to me. Will follow up on patch.

Change 999005 merged by jenkins-bot:

[mediawiki/core@master] Stats: Introduce factory method to create NULL stats factory

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

DAlangi_WMF claimed this task.