In ServiceWiring.php, there's

$factory->setLogger( LoggerFactory::getInstance( 'exec' ) );
$factory->logStderr();

so in theory it should be in the exec.log?

tailing /srv/mw-log/exec.log shows new entries coming in, so I think logging is working properly.

wmf-config is configured with:

6657:		'exec' => 'debug',

So that's looking good as well. That means it's all on mwlog1001, and anything with INFO or higher should also be in Logstash.

If successful commands are in debug-only, that means those are not in Logstash. But perhaps that's a good balance overall in terms of load, sensitivity, and ease of access.

In some classe we use

$this->setLogger( new NullLogger() );

and then set the real logger in ServiceWiring. (If a class has a logger, it is almost certainly going to be set somewhere.) In others we use

$this->setLogger( LoggerFactory::getInstance( 'foo' ) );

and then override it for unit tests. We should probably standardize on one of those patterns to reduce confusion.

That seems a better way forward, indeed

In T257070#6279639, @Tgr wrote:

In some classe we use

$this->setLogger( new NullLogger() );

and then set the real logger in ServiceWiring. (If a class has a logger, it is almost certainly going to be set somewhere.) In others we use

$this->setLogger( LoggerFactory::getInstance( 'foo' ) );

and then override it for unit tests. We should probably standardize on one of those patterns to reduce confusion.

A third pattern is that the stable constructor method takes an optional logger object:

$this->setLogger( $logger ?? new NullLogger() );

A fourth patttern is that the class is a service class or factory class where the constructor is considered internal and takes a mandatory Logger object:

$this->setLogger( $logger );

I currently understand these four as follows:

1. $this->setLogger( new NullLogger() ); - This is the standard for anything where we want to separate construction of objects from the public handing out of objects. For example, ResourceLoaderModule has a NullLogger by default. The ResourceLoader class calls setLogger afterwards later in the chain. This adds the constraint that nothing significant must happen during the constructor logic as this could not be logged. This pattern is I imho fragile and should be phased out of in favour of pattern 3. But there's not been a push to do that yet.
2. $this->setLogger( LoggerFactory::getInstance( 'foo' ) ); - This was the standard for everything written prior to our use of PSR-3 and dependency injection and is always legacy. These are generally slowly moved higher up to the stack over the years until such as time where there is an owner to decide and figure out how apply service wiring for that component at which point it becomes pattern 3 or 4, depending on whether the constructor is considered public/stable.
3. $this->setLogger( $logger ?? new NullLogger() ); - This is the inevitable pattern you come up with if you want to support a public constructor for your class but can't make a breaking change and don't want to require callers to create a null logger by default.
4. $this->setLogger( $logger ); - This i s the standard for all newer service-wired and dependency injected code.

The annoying thing about 1/3/4 is that it's hard to figure out which channel a class logs to. I wonder if we are doing dependency injection wrong here and we should be injecting a LoggerFactory instance instead, and leave it to the class to decide on the channel and create the logger.

Of course, Logger is a PSR standard and LoggerFactory is not, so that would make code harder to reuse.

Does this task need to be private?

I think in general we are using too many channels and would favour injection as we do today. This means the service is in control and individual components don't have to concern themselves with how they are used or composed together. In some rare cases that means two channels are injected for legacy reasons.

On the whole, though, it seems neat to be able to think of a service as one unit in terms of its message output and in terms of how that is monitored and triaged.