Skip to content

Inherit methodFactory extensions from the parent to the child loggers. #4809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 22, 2025
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
"bs58": "^6.0.0",
"content-type": "^1.0.4",
"jwt-decode": "^4.0.0",
"loglevel": "^1.7.1",
"loglevel": "^1.9.2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR, I think this could equally be ^1.9.0, since that's where .rebuild was introduced, but I don't imagine it makes much difference in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not hurt to include the minor patches.

Copy link
Member

@richvdh richvdh Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not hurt to include the minor patches.

In this specific situation, I agree, but in general, that's not correct.

The reason it might hurt is that a downstream application may have specifically decided that they want to use 1.9.0 or 1.9.1, for some reason and pinned their dependencies accordingly. (For example, maybe it turns out that 1.9.2 introduces a regression in their application). There is no need for matrix-js-sdk to insist on 1.9.2, and as a general principle package.json should list the earliest versions that we are compatible with.

Nevertheless that seems an unlikely scenario here. So I'm not going to insist you change it. But bear in mind for future :).

"matrix-events-sdk": "0.0.1",
"matrix-widget-api": "^1.10.0",
"oidc-client-ts": "^3.0.1",
Expand Down
21 changes: 18 additions & 3 deletions src/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ export interface Logger extends BaseLogger {
/**
* Create a child logger.
*
* This child will use the `methodFactory` of the parent, so any log extensions applied to the parent
* at the time of calling `getChild` will be applied to the child as well.
* It will NOT apply changes to the parent's `methodFactory` after the child was created.
* Those changes need to be applied to the child manually.
*
* @param namespace - name to add to the current logger to generate the child. Some implementations of `Logger`
* use this as a prefix; others use a different mechanism.
*/
Expand Down Expand Up @@ -128,14 +133,24 @@ interface PrefixedLogger extends loglevel.Logger, LoggerWithLogMethod {
*
* @param prefix Prefix to add to each logged line. If undefined, no prefix will be added.
*/
function getPrefixedLogger(prefix?: string): LoggerWithLogMethod {
function getPrefixedLogger(prefix?: string): PrefixedLogger {
const loggerName = DEFAULT_NAMESPACE + (prefix === undefined ? "" : `-${prefix}`);
const prefixLogger = loglevel.getLogger(loggerName) as PrefixedLogger;

if (prefixLogger.getChild === undefined) {
// This is a new loglevel Logger which has not been turned into a PrefixedLogger yet.
prefixLogger.prefix = prefix;
prefixLogger.getChild = (childPrefix): Logger => getPrefixedLogger((prefix ?? "") + childPrefix);
prefixLogger.getChild = (childPrefix): Logger => {
// create the new child logger
const childLogger = getPrefixedLogger((prefix ?? "") + childPrefix);
// Assign the methodFactory from the parent logger.
// This is useful if we add extensions to the parent logger that modifies
// its methodFactory. (An example extension is: storing each log to a rageshake db)
childLogger.methodFactory = prefixLogger.methodFactory;
// Rebuild the child logger with the new methodFactory.
childLogger.rebuild();
return childLogger;
};
prefixLogger.setLevel(loglevel.levels.DEBUG, false);
}

Expand All @@ -146,7 +161,7 @@ function getPrefixedLogger(prefix?: string): LoggerWithLogMethod {
* Drop-in replacement for `console` using {@link https://www.npmjs.com/package/loglevel|loglevel}.
* Can be tailored down to specific use cases if needed.
*/
export const logger = getPrefixedLogger();
export const logger = getPrefixedLogger() as LoggerWithLogMethod;

/**
* A "span" for grouping related log lines together.
Expand Down
12 changes: 6 additions & 6 deletions src/matrixrtc/MatrixRTCSessionManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { logger as rootLogger } from "../logger.ts";
import { logger as rootLogger, type Logger } from "../logger.ts";
import { type MatrixClient, ClientEvent } from "../client.ts";
import { TypedEventEmitter } from "../models/typed-event-emitter.ts";
import { type Room } from "../models/room.ts";
Expand All @@ -23,8 +23,6 @@ import { type MatrixEvent } from "../models/event.ts";
import { MatrixRTCSession } from "./MatrixRTCSession.ts";
import { EventType } from "../@types/event.ts";

const logger = rootLogger.getChild("[MatrixRTCSessionManager]");

export enum MatrixRTCSessionManagerEvents {
// A member has joined the MatrixRTC session, creating an active session in a room where there wasn't previously
SessionStarted = "session_started",
Expand All @@ -50,8 +48,10 @@ export class MatrixRTCSessionManager extends TypedEventEmitter<MatrixRTCSessionM
// longer the correct session object for the room.
private roomSessions = new Map<string, MatrixRTCSession>();

private logger: Logger;
public constructor(private client: MatrixClient) {
super();
this.logger = rootLogger.getChild("[MatrixRTCSessionManager]");
}

public start(): void {
Expand Down Expand Up @@ -105,7 +105,7 @@ export class MatrixRTCSessionManager extends TypedEventEmitter<MatrixRTCSessionM
private onRoomState = (event: MatrixEvent, _state: RoomState): void => {
const room = this.client.getRoom(event.getRoomId());
if (!room) {
logger.error(`Got room state event for unknown room ${event.getRoomId()}!`);
this.logger.error(`Got room state event for unknown room ${event.getRoomId()}!`);
return;
}

Expand All @@ -129,10 +129,10 @@ export class MatrixRTCSessionManager extends TypedEventEmitter<MatrixRTCSessionM
const nowActive = session.memberships.length > 0;

if (wasActiveAndKnown && !nowActive) {
logger.trace(`Session ended for ${room.roomId} (${session.memberships.length} members)`);
this.logger.trace(`Session ended for ${room.roomId} (${session.memberships.length} members)`);
this.emit(MatrixRTCSessionManagerEvents.SessionEnded, room.roomId, this.roomSessions.get(room.roomId)!);
} else if (!wasActiveAndKnown && nowActive) {
logger.trace(`Session started for ${room.roomId} (${session.memberships.length} members)`);
this.logger.trace(`Session started for ${room.roomId} (${session.memberships.length} members)`);
this.emit(MatrixRTCSessionManagerEvents.SessionStarted, room.roomId, this.roomSessions.get(room.roomId)!);
}
}
Expand Down
2 changes: 1 addition & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5081,7 +5081,7 @@ log-update@^6.1.0:
strip-ansi "^7.1.0"
wrap-ansi "^9.0.0"

loglevel@^1.7.1:
loglevel@^1.9.2:
version "1.9.2"
resolved "https://registry.yarnpkg.com/loglevel/-/loglevel-1.9.2.tgz#c2e028d6c757720107df4e64508530db6621ba08"
integrity sha512-HgMmCqIJSAKqo68l0rS2AanEWfkxaZ5wNiEFb5ggm08lDs9Xl2KxBlX3PTcaD2chBM1gXAYf491/M2Rv8Jwayg==
Expand Down