Skip to content
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

Added core logging #5727

Merged
merged 21 commits into from
Apr 25, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

### Enhancements
* Enable multiple processes to operate on an encrypted Realm simultaneously. ([realm/realm-core#1845](https://github.com/realm/realm-core/issues/1845))
* Added `Realm.setLogger`, that allows to setup a single static logger for the duration of the app lifetime. Differently from the now deprecated sync logger (that was setup with `Sync.setLogger`), this new one will emit messages coming also from the local database, and not only from sync. It is also possible to change the log level during the whole duration of the app lifetime with `Realm.setLogLevel`. ([#2546](https://github.com/realm/realm-js/issues/2546))

### Fixed
* Fix a stack overflow crash when using the query parser with long chains of AND/OR conditions. ([realm/realm-core#6428](https://github.com/realm/realm-core/pull/6428), since v10.11.0)
Expand Down
5 changes: 5 additions & 0 deletions integration-tests/tests/src/setup-globals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,8 @@ describe("Test Harness", function (this: Mocha.Suite) {
Suite.prototype.longTimeout = longTimeout;
Context.prototype.longTimeout = longTimeout;
});

import { Realm } from "realm";

//Disable the logger to avoid console flooding
Realm.setLogLevel("off");
58 changes: 58 additions & 0 deletions integration-tests/tests/src/tests/shared-realms.ts
elle-j marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,64 @@ import { openRealmBefore, openRealmBeforeEach } from "../hooks";
import { createLocalConfig } from "../utils/open-realm";

describe("SharedRealm operations", () => {
describe("logger", () => {
it("logger callback gets called", async function () {
type Log = {
message: string;
level: string;
};
let logs: Log[] = [];

Realm.setLogger((level, message) => {
logs.push({ level, message });
});

Realm.setLogLevel("all");

const realm = await Realm.open({
schema: [{ name: "Person", properties: { name: "string" } }],
});
realm.write(() => realm.create("Person", { name: "Alice" }));

expect(logs).to.not.be.empty;
expect(logs.map((l) => l.level)).to.contain.members(["trace", "debug"]);
logs = [];

Realm.setLogLevel("trace");
realm.write(() => realm.create("Person", { name: "Alice" }));
expect(logs.map((l) => l.level)).to.contain.members(["trace", "debug"]);
logs = [];

Realm.setLogLevel("debug");
realm.write(() => realm.create("Person", { name: "Alice" }));
expect(logs.map((l) => l.level))
.to.contain("debug")
.and.to.not.contain("trace");
logs = [];

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

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;
Comment on lines +65 to +71
Copy link
Contributor

@elle-j elle-j Apr 18, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 😁

Copy link
Contributor

@elle-j elle-j Apr 19, 2023

Choose a reason for hiding this comment

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

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 👍


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

//This will also disable the logger again after the test
Realm.setLogLevel("off");
realm.write(() => realm.create("Person", { name: "Alice" }));
expect(logs).to.be.empty;
});
});

describe("object deletion", () => {
openRealmBefore({ schema: [{ name: "Person", properties: { name: "string" } }] });

Expand Down
1 change: 1 addition & 0 deletions packages/bindgen/spec.yml
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,7 @@ classes:
is_empty_realm: '(realm: SharedRealm) -> bool'
base64_decode: '(input: StringData) -> BinaryData'
make_logger_factory: '(log: (level: LoggerLevel, message: const std::string&) off_thread) -> LoggerFactory'
make_logger: '(log: (level: LoggerLevel, message: const std::string&) off_thread) -> SharedLogger'
simulate_sync_error: '(session: SyncSession&, code: const int&, message: const std::string&, type: const std::string&, is_fatal: bool)'
consume_thread_safe_reference_to_shared_realm: '(tsr: ThreadSafeReference) -> SharedRealm'
file_exists: '(path: StringData) -> bool'
Expand Down
15 changes: 10 additions & 5 deletions packages/bindgen/src/realm_js_helpers.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return [logger = std::move(logger)](util::Logger::Level level) mutable {
auto out = make_logger(std::move(logger));
out->set_level_threshold(level);
return out;
};
}

static std::shared_ptr<util::Logger> make_logger(LogCallback&& logger)
{
class MyLogger final : public util::Logger {
public:
Expand All @@ -210,11 +219,7 @@ struct Helpers {
LogCallback m_log;
};

return [logger = std::move(logger)](util::Logger::Level level) {
auto out = std::make_shared<MyLogger>(logger);
out->set_level_threshold(level);
return out;
};
return std::make_shared<MyLogger>(logger);
}

static void simulate_sync_error(SyncSession& session, const int& code, const std::string& message,
Expand Down
91 changes: 91 additions & 0 deletions packages/realm/src/Logger.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
////////////////////////////////////////////////////////////////////////////
//
// Copyright 2023 Realm Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//
////////////////////////////////////////////////////////////////////////////

import { assert, binding } from "./internal";

export type LogLevel = "all" | "trace" | "debug" | "detail" | "info" | "warn" | "error" | "fatal" | "off";
papafe marked this conversation as resolved.
Show resolved Hide resolved

export enum NumericLogLevel {
All = 0,
Trace = 1,
Debug = 2,
Detail = 3,
Info = 4,
Warn = 5,
Error = 6,
Fatal = 7,
Off = 8,
}

export type Logger = (level: NumericLogLevel, message: string) => void;

export type LoggerCallback = (level: LogLevel, message: string) => void;
papafe marked this conversation as resolved.
Show resolved Hide resolved

/** @internal */
export function toBindingLoggerLevel(arg: LogLevel): binding.LoggerLevel {
const bindingLogLevel = inverseTranslationTable[arg];
assert(bindingLogLevel !== undefined, `Unexpected log level: ${arg}`);
return bindingLogLevel;
}

/** @internal */
export function fromBindingLoggerLevelToNumericLogLevel(arg: binding.LoggerLevel): NumericLogLevel {
// For now, these map 1-to-1
return arg as unknown as NumericLogLevel;
}

const translationTable: Record<binding.LoggerLevel, LogLevel> = {
[binding.LoggerLevel.All]: "all",
[binding.LoggerLevel.Trace]: "trace",
[binding.LoggerLevel.Debug]: "debug",
[binding.LoggerLevel.Detail]: "detail",
[binding.LoggerLevel.Info]: "info",
[binding.LoggerLevel.Warn]: "warn",
[binding.LoggerLevel.Error]: "error",
[binding.LoggerLevel.Fatal]: "fatal",
[binding.LoggerLevel.Off]: "off",
};

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

/** @internal */
export function fromBindingLoggerLevelToLogLevel(arg: binding.LoggerLevel): LogLevel {
return translationTable[arg];
}

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

@elle-j elle-j Apr 18, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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[];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


/** @internal */
export const defaultLogger: LoggerCallback = function (logLevel: LogLevel, message: string) {
papafe marked this conversation as resolved.
Show resolved Hide resolved
const formattedLogMessage = `[${logLevel}] ${message}`;
/* eslint-disable no-console */
if (consoleErrorLevels.includes(logLevel)) {
console.error(formattedLogMessage);
} else if (consoleWarnLevels.includes(logLevel)) {
console.error(formattedLogMessage);
} else {
console.log(formattedLogMessage);
}
papafe marked this conversation as resolved.
Show resolved Hide resolved
/* eslint-enable no-console */
};

/** @internal */
export const defaultLoggerLevel: LogLevel = "warn";
33 changes: 33 additions & 0 deletions packages/realm/src/Realm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ import {
List,
LocalAppConfiguration,
LogLevel,
LoggerCallback,
MapToDecorator,
MigrationCallback,
MongoDB,
Expand Down Expand Up @@ -152,8 +153,11 @@ import {
UserState,
assert,
binding,
defaultLogger,
defaultLoggerLevel,
extendDebug,
flags,
fromBindingLoggerLevelToLogLevel,
fromBindingRealmSchema,
fs,
index,
Expand All @@ -162,6 +166,7 @@ import {
normalizeRealmSchema,
safeGlobalThis,
toArrayBuffer,
toBindingLoggerLevel,
toBindingSchema,
toBindingSyncConfig,
validateConfiguration,
Expand Down Expand Up @@ -261,6 +266,30 @@ export class Realm {

private static internals = new Set<binding.WeakRef<binding.Realm>>();

/**
* Sets the log level.
* @param level The log level to be used by the logger. The default value is `info`.
* @note The log level can be changed during the lifetime of the application.
* @since v12.0.0
*/
static setLogLevel(level: LogLevel) {
const bindingLoggerLevel = toBindingLoggerLevel(level);
binding.Logger.setDefaultLevelThreshold(bindingLoggerLevel);
}

/**
* Sets the logger callback.
* @param loggerCallback The callback invoked by the logger. The default callback uses `console.log`, `console.warn` and `console.error`, depending on the level of the message.
* @note The logger callback needs to be setup before opening the first realm.
* @since v12.0.0
*/
static setLogger(loggerCallback: LoggerCallback) {
const logger = binding.Helpers.makeLogger((level, message) => {
loggerCallback(fromBindingLoggerLevelToLogLevel(level), message);
});
binding.Logger.setDefaultLogger(logger);
}

/**
* Clears the state by closing and deleting any Realm in the default directory and logout all users.
* @private Not a part of the public API: It's primarily used from the library's tests.
Expand Down Expand Up @@ -1971,6 +2000,10 @@ declare global {
}
}

//Set default logger and log level.
Realm.setLogger(defaultLogger);
Realm.setLogLevel(defaultLoggerLevel);

// Patch the global at runtime
let warnedAboutGlobalRealmUse = false;
Object.defineProperty(safeGlobalThis, "Realm", {
Expand Down
41 changes: 9 additions & 32 deletions packages/realm/src/app-services/Sync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@
import {
App,
ConnectionState,
LogLevel,
Logger,
MutableSubscriptionSet,
NumericLogLevel,
OpenRealmBehaviorConfiguration,
OpenRealmBehaviorType,
OpenRealmTimeOutBehavior,
Expand All @@ -31,41 +34,12 @@ import {
User,
assert,
binding,
fromBindingLoggerLevelToNumericLogLevel,
toBindingLoggerLevel,
toBindingSyncConfig,
validateSyncConfiguration,
} from "../internal";

export type LogLevel = "all" | "trace" | "debug" | "detail" | "info" | "warn" | "error" | "fatal" | "off";

export enum NumericLogLevel {
All = 0,
Trace = 1,
Debug = 2,
Detail = 3,
Info = 4,
Warn = 5,
Error = 6,
Fatal = 7,
Off = 8,
}

export type Logger = (level: NumericLogLevel, message: string) => void;

function toBindingLoggerLevel(arg: LogLevel): binding.LoggerLevel {
const result = Object.entries(NumericLogLevel).find(([name]) => {
return name.toLowerCase() === arg;
});
assert(result, `Unexpected log level: ${arg}`);
const [, level] = result;
assert.number(level, "Expected a numeric level");
return level as number as binding.LoggerLevel;
}

function fromBindingLoggerLevel(arg: binding.LoggerLevel): NumericLogLevel {
// For now, these map 1-to-1
return arg as unknown as NumericLogLevel;
}

export class Sync {
/** @deprecated Please use named imports */
static Session = SyncSession;
Expand All @@ -82,13 +56,16 @@ export class Sync {
/** @deprecated Please use named imports */
static NumericLogLevel = NumericLogLevel;

/** @deprecated Will be removed in v13.0.0. Please use {@link Realm.setLogLevel}. */
static setLogLevel(app: App, level: LogLevel) {
const numericLevel = toBindingLoggerLevel(level);
app.internal.syncManager.setLogLevel(numericLevel);
}

/** @deprecated Will be removed in v13.0.0. Please use {@link Realm.setLogger}. */
static setLogger(app: App, logger: Logger) {
const factory = binding.Helpers.makeLoggerFactory((level, message) => {
logger(fromBindingLoggerLevel(level), message);
logger(fromBindingLoggerLevelToNumericLogLevel(level), message);
});
app.internal.syncManager.setLoggerFactory(factory);
}
Expand Down
5 changes: 1 addition & 4 deletions packages/realm/src/binding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
//
////////////////////////////////////////////////////////////////////////////

import { IndexSet, Int64, Logger, LoggerLevel, ObjKey, Timestamp } from "../generated/ts/native.mjs";
import { IndexSet, Int64, ObjKey, Timestamp } from "../generated/ts/native.mjs";

/** @internal */
export * from "../generated/ts/native.mjs"; // core types are transitively exported.
Expand Down Expand Up @@ -73,6 +73,3 @@ export function isEmptyObjKey(objKey: ObjKey) {
// This relies on the JS representation of an ObjKey being a bigint
return Int64.equals(objKey as unknown as Int64, -1);
}

// Silence logs from core by default
Logger.setDefaultLevelThreshold(LoggerLevel.Off);
1 change: 1 addition & 0 deletions packages/realm/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ export {
InvalidateEvent,
List,
LocalAppConfiguration,
LoggerCallback,
LogLevel,
mapTo,
MapToDecorator,
Expand Down
1 change: 1 addition & 0 deletions packages/realm/src/internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ export * from "./Set";
export * from "./Dictionary";

export * from "./Types";
export * from "./Logger";

export * from "./app-services/utils";
export * from "./app-services/SyncConfiguration";
Expand Down