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

toger5 · 2025-04-16T17:32:48Z

This fixes an issue where a log extension (a modification to the methodFactory) applied to a parent logger would ONLY be executed when logging with the parent but not on any of the children created with getChild().

This PR changes getChild to also inherit the methodFactory from the parent.

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).

…getLevel());` This is the important part that actually registers the new methods.

dbkr

Ummm... I think I understand the PR you referenced but I'm afraid I don't understand what's going on here, and the amount of casting through unknown and splicing of methods to other objects is terrifying. If we do this, it needs very heavy commenting!

…rect.

toger5 · 2025-04-17T18:32:15Z

@dbkr I found that rebuild is doing the same thing as childLogger.setLevel(childLogger.getLevel());
Additionally it was possible to make the types a lot more easy to follow by returning PrefixedLogger from getPrefixedLogger and then chinging the type once we export logger export const logger = getPrefixedLogger() as LoggerWithLogMethod;

src/logger.ts

… extension is in place.

dbkr

OK - thanks for the comments, I think it's a bit easier to see what's happening now.

BillCarsonFr

ok

richvdh · 2025-04-22T13:04:58Z

This is an experiment where I tried to get the same thing I am doing in: #4807

I assume it is no longer an experiment? Could you update the PR description to reflect the current state, please?

richvdh

looks generally good, a few clarifications though please

src/logger.ts

florianduros · 2025-04-22T13:17:12Z

@toger5 the PR breaks the https://github.com/matrix-org/complement-crypto tests. These tests are run when the PR is put in the merge queue.

toger5 · 2025-04-22T13:54:27Z

@florianduros any idea how it breaks it?
Do we rely on logging output for the tests?

florianduros · 2025-04-22T14:43:08Z

@toger5 Complement crypto runs the tests with an older version of loglevel (https://github.com/matrix-org/complement-crypto/blob/main/internal/api/js/js-sdk/yarn.lock#L226 1.8.1). rebuild method was added pimterry/loglevel#189 in January in the 1.9.0

toger5 · 2025-04-22T14:48:56Z

@florianduros I think it makes sense to not use the rebuild method in that case especailly since we do have a workaround.

florianduros · 2025-04-22T14:50:49Z

src/logger.ts

-            childLogger.rebuild();
+            // childLogger.rebuild();
+            // TODO: As a stop-gap we use the deprecated method of rebuilding the logger
+            // by setting the level. Complement Crypto uses an older version of loglevel.


Should we instead update the dependency in complement crypto?

I have done this in: matrix-org/complement-crypto#173

This fixes complement tests for: matrix-org/matrix-js-sdk#4809

toger5 · 2025-04-22T15:12:26Z

I reverted the last commit and we fix the issue with the updated complement-crypto PR.

richvdh · 2025-04-22T15:47:03Z

package.json

@@ -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",


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.

It does not hurt to include the minor patches.

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 :).

richvdh

LGTM

toger5 added 2 commits April 16, 2025 19:31

use methodFactory extensions from the rootLogger in child loggers.

4d7ab1c

use simple method factory copy AND `childLogger.setLevel(childLogger.…

3a80fc8

…getLevel());` This is the important part that actually registers the new methods.

github-actions bot deployed to PR Documentation Preview April 17, 2025 11:09 View deployment

toger5 mentioned this pull request Apr 17, 2025

Change child loggers (getChild) to LogSpans #4807

Closed

4 tasks

toger5 added the T-Enhancement label Apr 17, 2025

toger5 changed the title ~~use methodFactory extensions from the rootLogger in child loggers.~~ Inherit methodFactory extensions from the parent to the child loggers. Apr 17, 2025

toger5 marked this pull request as ready for review April 17, 2025 11:12

toger5 requested a review from a team as a code owner April 17, 2025 11:12

toger5 requested review from dbkr and florianduros April 17, 2025 11:12

dbkr reviewed Apr 17, 2025

View reviewed changes

add comments and find a way to make it clearer that the types are cor…

7a025e4

…rect.

github-actions bot deployed to PR Documentation Preview April 17, 2025 18:31 View deployment

toger5 requested a review from dbkr April 17, 2025 18:32

robintown reviewed Apr 18, 2025

View reviewed changes

src/logger.ts Outdated Show resolved Hide resolved

src/logger.ts Outdated Show resolved Hide resolved

src/logger.ts Outdated Show resolved Hide resolved

review

b9a0660

github-actions bot deployed to PR Documentation Preview April 18, 2025 15:22 View deployment

additionally fix MatrixRTCSessionManager being initialized before the…

361c636

… extension is in place.

toger5 requested a review from robintown April 19, 2025 12:30

dbkr approved these changes Apr 22, 2025

View reviewed changes

Add comment to clarify order of log extensions and creating childs.

ec385ec

toger5 requested a review from a team as a code owner April 22, 2025 10:31

toger5 requested a review from dbkr April 22, 2025 10:31

github-actions bot deployed to PR Documentation Preview April 22, 2025 10:33 View deployment

dbkr approved these changes Apr 22, 2025

View reviewed changes

toger5 removed the request for review from florianduros April 22, 2025 10:51

BillCarsonFr approved these changes Apr 22, 2025

View reviewed changes

robintown approved these changes Apr 22, 2025

View reviewed changes

toger5 added this pull request to the merge queue Apr 22, 2025

richvdh requested changes Apr 22, 2025

View reviewed changes

review

54ecd1f

github-actions bot deployed to PR Documentation Preview April 22, 2025 14:41 View deployment

github-actions bot deployed to PR Documentation Preview April 22, 2025 14:46 View deployment

toger5 requested a review from richvdh April 22, 2025 14:47

toger5 force-pushed the toger5/try-getting-logger-getChild-to-work branch from 20e178d to 8e83e09 Compare April 22, 2025 14:49

florianduros reviewed Apr 22, 2025

View reviewed changes

github-actions bot deployed to PR Documentation Preview April 22, 2025 14:50 View deployment

toger5 added a commit to matrix-org/complement-crypto that referenced this pull request Apr 22, 2025

Upgrade Loglevel to 1.9.2

c106903

This fixes complement tests for: matrix-org/matrix-js-sdk#4809

toger5 mentioned this pull request Apr 22, 2025

Upgrade Loglevel to 1.9.2 matrix-org/complement-crypto#173

Open

toger5 force-pushed the toger5/try-getting-logger-getChild-to-work branch from 8e83e09 to 54ecd1f Compare April 22, 2025 15:11

github-actions bot deployed to PR Documentation Preview April 22, 2025 15:13 View deployment

Set "loglevel" min version to guarantee access to logger.rebuild

4131970

toger5 requested a review from a team as a code owner April 22, 2025 15:41

toger5 requested a review from t3chguy April 22, 2025 15:41

github-actions bot deployed to PR Documentation Preview April 22, 2025 15:43 View deployment

richvdh reviewed Apr 22, 2025

View reviewed changes

richvdh approved these changes Apr 22, 2025

View reviewed changes

toger5 added this pull request to the merge queue Apr 22, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 22, 2025

toger5 added this pull request to the merge queue Apr 22, 2025

Merged via the queue into develop with commit 19b1b90 Apr 22, 2025
30 checks passed

toger5 deleted the toger5/try-getting-logger-getChild-to-work branch April 22, 2025 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

toger5 commented Apr 16, 2025 •

edited

Loading

dbkr left a comment

toger5 commented Apr 17, 2025

dbkr left a comment

BillCarsonFr left a comment

richvdh commented Apr 22, 2025

richvdh left a comment

florianduros commented Apr 22, 2025 •

edited

Loading

toger5 commented Apr 22, 2025 •

edited

Loading

florianduros commented Apr 22, 2025 •

edited

Loading

toger5 commented Apr 22, 2025

florianduros Apr 22, 2025 •

edited

Loading

toger5 Apr 22, 2025

florianduros Apr 22, 2025

toger5 commented Apr 22, 2025

richvdh Apr 22, 2025

toger5 Apr 22, 2025

richvdh Apr 22, 2025 •

edited

Loading

richvdh left a comment

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

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

Conversation

toger5 commented Apr 16, 2025 • edited Loading

Checklist

dbkr left a comment

Choose a reason for hiding this comment

toger5 commented Apr 17, 2025

dbkr left a comment

Choose a reason for hiding this comment

BillCarsonFr left a comment

Choose a reason for hiding this comment

richvdh commented Apr 22, 2025

richvdh left a comment

Choose a reason for hiding this comment

florianduros commented Apr 22, 2025 • edited Loading

toger5 commented Apr 22, 2025 • edited Loading

florianduros commented Apr 22, 2025 • edited Loading

toger5 commented Apr 22, 2025

florianduros Apr 22, 2025 • edited Loading

Choose a reason for hiding this comment

toger5 Apr 22, 2025

Choose a reason for hiding this comment

florianduros Apr 22, 2025

Choose a reason for hiding this comment

toger5 commented Apr 22, 2025

richvdh Apr 22, 2025

Choose a reason for hiding this comment

toger5 Apr 22, 2025

Choose a reason for hiding this comment

richvdh Apr 22, 2025 • edited Loading

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

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

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

toger5 commented Apr 16, 2025 •

edited

Loading

florianduros commented Apr 22, 2025 •

edited

Loading

toger5 commented Apr 22, 2025 •

edited

Loading

florianduros commented Apr 22, 2025 •

edited

Loading

florianduros Apr 22, 2025 •

edited

Loading

richvdh Apr 22, 2025 •

edited

Loading