Added core logging #5727

papafe · 2023-04-13T08:47:33Z

This PR adds support for the new core logging feature.

This closes #2546

NOTE: The test could need to be revisited after realm/realm-core#6524 is merged

☑️ ToDos

📝 Changelog entry
🚦 Tests
Deprecate previous sync logging feature and create ticket to remove it in v13
Add default logger callback (similar to https://github.com/realm/realm-js/pull/5698/files)

* main: (41 commits) allow useQuery to filter or sort a collection by using a callback (#5513) Fix `realm.create` types and 'requiredProperties' on Realm.Object constructor (#5697) Fix return type of `App.allUsers` (#5708) Removed renamed file Remove console.log Fix README instructions and bump version Bump template versions Update Templates (#5702) Moved "submodules" wireit task to "postinstall" Set up typedoc for realm-react, realm-web (#5709) Prepare for vNext (#5711) Prepare for 12.0.0-alpha.2 (#5707) Build iOS prebuilds in release by default (#5710) Use the event.sender as assignee when preparing release Updated prepare release workflow to print PR url in summary Fixing lint error fix the template app links (#5701) Expose `Sync` as named export (#5705) Enable all tests after bad rebase (#5595) Clear test state flag (#5689) ... # Conflicts: # packages/realm/src/Realm.ts

packages/realm/src/Logger.ts

Co-authored-by: LJ <[email protected]>

* main: Update core to v13.9.1 (#5739) Implement `User.state` (#5712) Rename `local-mongodb` to enable app replacements (#5734) Update return type of 'assert.never'. (#5718) Run tests after builds are done, regardless of they failed or succeeded (#5729)

elle-j

Great addition! I have some suggestions 👍

integration-tests/tests/src/tests/shared-realms.ts

elle-j · 2023-04-18T15:45:25Z

integration-tests/tests/src/tests/shared-realms.ts

+      Realm.setLogLevel("warn");
+      realm.write(() => realm.create("Person", { name: "Alice" }));
+      expect(logs).to.be.empty;
+
+      Realm.setLogLevel("error");
+      realm.write(() => realm.create("Person", { name: "Alice" }));
+      expect(logs).to.be.empty;


I think it could also be useful to have tests for when we'd expect log output for "warn" and "error" (and "fatal").

I'm a little torn on whether we should split up some of the tests into dedicated it tests as well. If we add some more tests for "warn" etc, having the it separation may be favorable? What do you think?

I don't think we need separate tests, otherwise we are probably verifying that the whole logger works, even at the core level. We just need to be sure the callback is called, and get a confirmation that we can change the log level. I think we can assume that core verified that all the levels work.

Regarding "warn", "error" and so on, I was thinking about it, but it seems that those are actually raised only when using sync (like with a compensating write), and I thought it would have been too much to create a sync test just to verify the logger (also connected to what I've said before). I'm open to change my mind though 😁

To me, the main reason for extending "warn" etc. would be if the tests are intended to test that the logger not only doesn't get fired when it shouldn't, but also that it does when it should. But if that's not within this scope then it might just be a PR for another time 👍

packages/realm/src/Logger.ts

elle-j · 2023-04-18T16:15:15Z

packages/realm/src/Logger.ts

+const consoleErrorLevels: LogLevel[] = ["error", "fatal"];
+const consoleWarnLevels: LogLevel[] = ["warn"];


This suggestion is combined with my previous one if we provide the additional log level helpers.

Suggested change

const consoleErrorLevels: LogLevel[] = ["error", "fatal"];

const consoleWarnLevels: LogLevel[] = ["warn"];

const consoleErrorLevels: ErrorLogLevel[] = ["error", "fatal"];

const consoleWarnLevels: WarnLogLevel[] = ["warn"];

In this case, you'd have to change e.g. consoleErrorLevels.includes(logLevel) to consoleErrorLevels.includes(logLevel as ErrorLogLevel), so not sure how beneficial this would be for us internally.

Or perhaps satisfies WarnLogLevel[].

Suggested change

const consoleErrorLevels: LogLevel[] = ["error", "fatal"];

const consoleWarnLevels: LogLevel[] = ["warn"];

const consoleErrorLevels = ["error", "fatal"] satisfies LogLevel[];

const consoleWarnLevels = ["warn"] satisfies LogLevel[];

@kraenhansen I wrote this as a comment to another related suggestion, but I'm reluctant to go this route (defining WarnLogLevel and ErrorLogLevel), because it will make the code more difficult to read, and we won't get much out of it.

packages/realm/src/Logger.ts

packages/realm/src/app-services/Sync.ts

Co-authored-by: LJ <[email protected]>

kraenhansen · 2023-04-19T10:12:14Z

packages/realm/src/Logger.ts

+
+const inverseTranslationTable: Record<LogLevel, binding.LoggerLevel> = Object.fromEntries(
+  Object.entries(translationTable).map(([key, val]) => [val, Number(key)]),
+) as unknown as Record<LogLevel, binding.LoggerLevel>;


I'd like to iterate on this with you. I think it'd be possible to use the type parameters on the fromEntries, entries and map to achieve this and avoid as unknown.

I actually discovered that we don't need the unknown 😁
Before I didn't have the conversion to number, and that was the issue

papafe · 2023-04-19T15:46:28Z

packages/bindgen/src/realm_js_helpers.h

@@ -194,6 +194,15 @@ struct Helpers {
    using LoggerFactory = std::function<std::shared_ptr<util::Logger>(util::Logger::Level)>;
    using LogCallback = std::function<void(util::Logger::Level, const std::string& message)>;
    static LoggerFactory make_logger_factory(LogCallback&& logger)
+    {


@RedBeard0531 I followed your suggestion of creating a make_logger method that is being used by the older make_logger_factory method.
I just wanted to ask if it makes sense the way that I've done it.

packages/realm/src/Logger.ts

Co-authored-by: Kræn Hansen <[email protected]>

* main: Added note in docs for change listeners (#5749) Update CHANGELOG entry (#5751) Use `SchemaParseError` for invalid schemas (#5722) Improving Electron tests (#5738) Support SSL Sync Configuration (#5507) # Conflicts: # CHANGELOG.md

* main: Update yaml to version 2.2.2 (#5763) Upgraded RN to 0.71.7 for "realm" and "@realm/react" React Native 0.71.4 → 0.71.7 (#5761) Split changelog and compability matrix. Update readme and support. (#5748) fix build scripts for ios and android Removed simple test/example used to bootstrap bindgen project Make bindgen-lib's .eslintrc be freestanding, and copy to js bindgen dir move js_spec.yml out of bindgen-lib Move js-specific TS files out of bindgen-lib change --template arg to be path to source file move packages/bindgen to packages/realm/bindgen/vendor/bindgen-lib rename realm_js_helpers.h to just realm_helpers.h Iterating Android tests (#5746)

papafe added 4 commits March 31, 2023 15:00

Initial stub

f9fb5ab

Small note

163be76

Corrections

8024720

Fixed test

ef68b27

cla-bot bot added the cla: yes label Apr 13, 2023

papafe added 2 commits April 13, 2023 10:50

Small correction [skip ci]

ca15a98

elle-j reviewed Apr 14, 2023

View reviewed changes

packages/realm/src/Logger.ts Outdated Show resolved Hide resolved

papafe and others added 7 commits April 17, 2023 15:15

Update packages/realm/src/Logger.ts

b6441e6

Co-authored-by: LJ <[email protected]>

Created inverted table

baf6486

Various fixes

8924bb3

Updated core

4d1e602

Final touches

f1356c4

Small fix

ba23794

papafe marked this pull request as ready for review April 18, 2023 14:22

elle-j approved these changes Apr 18, 2023

View reviewed changes

Apply suggestions from code review

ea1daa7

Co-authored-by: LJ <[email protected]>

papafe requested review from kneth and kraenhansen April 19, 2023 09:05

Small corrections

2ff0f8b

kraenhansen reviewed Apr 19, 2023

View reviewed changes

kraenhansen approved these changes Apr 19, 2023

View reviewed changes

Small fixes

7d9d292

papafe mentioned this pull request Apr 19, 2023

Remove deprecated sync logger in v13 #5750

Open

papafe requested a review from RedBeard0531 April 19, 2023 15:44

papafe commented Apr 19, 2023

View reviewed changes

kraenhansen reviewed Apr 19, 2023

View reviewed changes

packages/realm/src/Logger.ts Outdated Show resolved Hide resolved

kneth approved these changes Apr 20, 2023

View reviewed changes

packages/realm/src/Logger.ts Show resolved Hide resolved

Update packages/realm/src/Logger.ts

41f9d26

Co-authored-by: Kræn Hansen <[email protected]>

papafe added 4 commits April 20, 2023 16:59

Removed unused [skip ci]

af0e164

Merge branch 'main' into fp/core-logging

0044a20

* main: Added note in docs for change listeners (#5749) Update CHANGELOG entry (#5751) Use `SchemaParseError` for invalid schemas (#5722) Improving Electron tests (#5738) Support SSL Sync Configuration (#5507) # Conflicts: # CHANGELOG.md

Removed

50c64f7

papafe merged commit b3a19af into main Apr 25, 2023

papafe deleted the fp/core-logging branch April 25, 2023 10:42

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added core logging #5727

Added core logging #5727

papafe commented Apr 13, 2023 •

edited

Loading

elle-j left a comment

elle-j Apr 18, 2023 •

edited

Loading

papafe Apr 19, 2023

elle-j Apr 19, 2023 •

edited

Loading

elle-j Apr 18, 2023 •

edited

Loading

kraenhansen Apr 19, 2023

papafe Apr 19, 2023

kraenhansen Apr 19, 2023

papafe Apr 19, 2023

papafe Apr 19, 2023

		const consoleErrorLevels: LogLevel[] = ["error", "fatal"];
		const consoleWarnLevels: LogLevel[] = ["warn"];

Added core logging #5727

Added core logging #5727

Conversation

papafe commented Apr 13, 2023 • edited Loading

☑️ ToDos

elle-j left a comment

Choose a reason for hiding this comment

elle-j Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

papafe Apr 19, 2023

Choose a reason for hiding this comment

elle-j Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

elle-j Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

kraenhansen Apr 19, 2023

Choose a reason for hiding this comment

papafe Apr 19, 2023

Choose a reason for hiding this comment

kraenhansen Apr 19, 2023

Choose a reason for hiding this comment

papafe Apr 19, 2023

Choose a reason for hiding this comment

papafe Apr 19, 2023

Choose a reason for hiding this comment

papafe commented Apr 13, 2023 •

edited

Loading

elle-j Apr 18, 2023 •

edited

Loading

elle-j Apr 19, 2023 •

edited

Loading

elle-j Apr 18, 2023 •

edited

Loading