Change child loggers (getChild) to LogSpans

[toger5]

With the child loggers, we ended up not seeing the logs in the rageshake because it requires a custom configuration in element call to write the child loggers (with getChild) into the rageshake log.
LogSpans are a lightweight alternative that still use the same logger but only extend the prefix and hence end up in the rageshake without registering new loggers.
It prohibits the footgun of not getting logs in the rageshake.

Checklist

• Tests written for new code (and old code if feasible).
• New or updated public/exported symbols have accurate TSDoc documentation.
• Linter and other CI checks pass.
• Sign-off given on the changes (see CONTRIBUTING.md).

[BillCarsonFr]

?

[richvdh]

With the child loggers, we ended up not seeing the logs in the rageshake because it requires a custom configuration in element call to write the child loggers (with getChild) into the rageshake log.

This doesn't sound like the right solution to this problem. If getChild doesn't produce a functional logger in your environment, I think you should fix that, rather than avoiding its use.

(Aside: https://github.com/element-hq/element-call/blob/livekit/src/settings/rageshake.ts#L9-L13 claims that element-call patches console.log, but that appears no longer to be the case as of element-hq/element-call#1592)

[richvdh]

(The problem seems to be that we don't set the methodfactory on newly-created child loggers?)

[toger5]

@richvdh

reading this:

matrix-js-sdk/src/logger.ts

Lines 152 to 160 in a52c64b

	* A "span" for grouping related log lines together.
	*
	* The current implementation just adds the name at the start of each log line.
	*
	* This offers a lighter-weight alternative to 'child' loggers returned by {@link Logger#getChild}. In particular,
	* it's not possible to apply individual filters to the LogSpan such as setting the verbosity level. On the other hand,
	* no reference to the LogSpan is retained in the logging framework, so it is safe to make lots of them over the course
	* of an application's life and just drop references to them when the job is done.
	*/

It sounded to me like it fits our usecase fairly well where all the logs are related to the matrix RTCsessions and we just want to prefix, if its from: "EncryptionManager",
"MatrixRTCSessionManager",
"NewMembershipManager",
"NewMembershipActionScheduler",
"RoomAndToDeviceTransport",
"RoomKeyTransport",
"ToDeviceKeyTransport" ...

[richvdh]

want prefixing if its form

Not quite understanding this. Do you mean "if it's from"?

"LogSpan" is intended for when you might create thousands of the things, and it makes no sense to filter on them -- for example, you might create one per event ID. Doesn't it make sense that you might want to be able to change the log level for, say, MatrixRTCSessionManager without changing the log level for the whole application, so that you can get debug logs for one without the other?

Also: if you're basically saying that getChild doesn't work, then you'd better rip it out everywhere, not just in a couple of places.

[toger5]

getChild only does not work in EC.

(The problem seems to be that we don't set the methodfactory on newly-created child loggers?)

How would that look like?

[richvdh]

How would that look like?

Have a look at logger.js in matrix-js-sdk, where getChild is patched into the Loggers.

[toger5]

I tried very hard to get it to work with getChild (see: #4809 which has the js-sdk changes but missing the EC changes)

I tried with:

const childLogger = getPrefixedLogger((prefix ?? "") + childPrefix) as unknown as loglevel.Logger;
            childLogger.methodFactory = logger.methodFactory;
            return childLogger as unknown as Logger;

first.

But this is not working. Then I tried all kinds of other more involved ways to get the correct (extended) method factory to the later initialized child loggers.

I eventually got sth somewhat working: 4d7ab1c

And then noticed that I never tried the initial approach with:
childLogger.setLevel(childLogger.getLevel());
Which now (having worked with it for a bit) seems like its obviously needed.
That seems to work. So there is an alternative solution for this here: #4809

@richvdh I think you like this other solution more? I still think the LogSpan is not compleatly off to what we are trying to achive. But in the end both work.

[toger5]

Closed in favor of: #4809