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

Core upgrade 13.8.0 - Core Logging #1226

Merged
merged 127 commits into from
May 24, 2023
Merged

Core upgrade 13.8.0 - Core Logging #1226

merged 127 commits into from
May 24, 2023

Conversation

desistefanova
Copy link
Contributor

@desistefanova desistefanova commented Mar 24, 2023

Support Core Logging
Applied changes from realm/realm-core#6339
The default logger is wrapped into _RealmLogger. To manage the log level at runtime use Realm.logger.level.
The default log level in 'Info'. Default print is available only in the first isolate working with realm.
Setting an instance of Logger is still supported, but it will be wrapped into an instance of _RealmLogger and the default printing is no more available.
The default printing could be stopped also with Realm.logger.clearListeners().

Fixes #859
Fixes #1234

@coveralls
Copy link

coveralls commented Mar 24, 2023

Pull Request Test Coverage Report for Build 5070324663

  • 45 of 75 (60.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.2%) to 87.818%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/src/native/realm_core.dart 14 15 93.33%
lib/src/scheduler.dart 10 11 90.91%
lib/src/realm_class.dart 21 49 42.86%
Totals Coverage Status
Change from base Build 4991856685: -1.2%
Covered Lines: 2660
Relevant Lines: 3029

💛 - Coveralls

@desistefanova desistefanova marked this pull request as ready for review May 23, 2023 07:57
Copy link
Contributor

@nielsenko nielsenko left a comment

Choose a reason for hiding this comment

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

I few questions and suggestions.

One thing I would like you to think about is:

Realm.logger = Realm.logger; // double wrap

lib/src/realm_class.dart Outdated Show resolved Hide resolved
lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
lib/src/realm_class.dart Outdated Show resolved Hide resolved
src/realm_dart_scheduler.cpp Show resolved Hide resolved
test/realm_logger_test.dart Outdated Show resolved Hide resolved
test/realm_logger_test.dart Outdated Show resolved Hide resolved
test/realm_logger_test.dart Outdated Show resolved Hide resolved
test/realm_logger_test.dart Outdated Show resolved Hide resolved
test/realm_logger_test.dart Outdated Show resolved Hide resolved
test/realm_logger_test.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@blagoev blagoev left a comment

Choose a reason for hiding this comment

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

Our tests rely on Apps which are setup in another Isolate. I think we should not be doing this and rely only on local and perhaps inMemory realms clearing them explicitly in these tests from the separate Isolates

src/realm_dart.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@blagoev blagoev left a comment

Choose a reason for hiding this comment

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

Its very hard to read and understand what the tests are trying to test, especially the common private functions.
I have added some suggestions but in general the tests seems not easy to maintain or write new tests

lib/src/realm_class.dart Outdated Show resolved Hide resolved
lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
lib/src/scheduler.dart Outdated Show resolved Hide resolved
lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
src/realm_dart.cpp Outdated Show resolved Hide resolved
test/realm_logger_test.dart Outdated Show resolved Hide resolved
test/realm_logger_test.dart Outdated Show resolved Hide resolved
test/realm_logger_test.dart Outdated Show resolved Hide resolved
test/realm_logger_test.dart Outdated Show resolved Hide resolved
test/realm_logger_test.dart Outdated Show resolved Hide resolved
@desistefanova desistefanova marked this pull request as draft May 23, 2023 20:36
* log message for testing fixes

* fix tests

* update logging library

* bump logging dep
@blagoev blagoev marked this pull request as ready for review May 24, 2023 15:06
@nielsenko
Copy link
Contributor

Suggested edit:

diff --git a/src/realm-core b/src/realm-core
index 3c950f43..096d9ba1 160000
--- a/src/realm-core
+++ b/src/realm-core
@@ -1 +1 @@
-Subproject commit 3c950f43bafc4fe39ce605e913cc7f30426e1747
+Subproject commit 096d9ba1ef0465fc4e6991a3b99cb99bb75014d7
diff --git a/src/realm_dart_logger.cpp b/src/realm_dart_logger.cpp
index 8ce5e24b..a5f5124b 100644
--- a/src/realm_dart_logger.cpp
+++ b/src/realm_dart_logger.cpp
@@ -30,7 +30,7 @@ bool is_core_logger_callback_set = false;
 std::map<Dart_Port, realm_log_level_e> dart_send_ports;
 realm_log_level_e current_core_log_level;
 
-realm_log_level_e calucale_minimum_log_level() {
+realm_log_level_e calculate_minimum_log_level() {
     std::lock_guard<std::recursive_mutex> lock(dart_logger_mutex);
     auto min_element = std::min_element(dart_send_ports.begin(), dart_send_ports.end(),
         [](std::pair<Dart_Port, realm_log_level_e> const& prev, std::pair<Dart_Port, realm_log_level_e> const& next) {
@@ -49,7 +49,7 @@ RLM_API void realm_dart_release_logger(Dart_Port port) {
     if (dart_send_ports.find(port) != dart_send_ports.end())
     {
         dart_send_ports.erase(port);
-        auto minimum_level = calucale_minimum_log_level();
+        auto minimum_level = calculate_minimum_log_level();
         realm_set_log_level(minimum_level);
     }
 }
@@ -101,7 +101,7 @@ RLM_API void realm_dart_set_log_level(realm_log_level_e level, Dart_Port port) {
     auto port_item = dart_send_ports.find(port);
     if (port_item == dart_send_ports.end() || port_item->second != level) {
         dart_send_ports[port] = level;
-        auto minimum_level = calucale_minimum_log_level();
+        auto minimum_level = calculate_minimum_log_level();
         realm_set_log_level(minimum_level);
     }
 }

@nielsenko
Copy link
Contributor

Suggested edit:

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 21f338dd..24a41e29 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,7 +9,7 @@
   `Realm.logger.level` allows changing the log level per isolate.
 * Add logging at the Storage level (Core upgrade).
 * Performance improvement for the following queries (Core upgrade):
-    * Significant (~75%) improvement when counting (query count) the number of exact matches (with no other query conditions) on a String/int/Uuid/ObjectId property that has an index. This improvement will be especially noticiable if there are a large number of results returned (duplicate values).
+    * Significant (~75%) improvement when counting (query count) the number of exact matches (with no other query conditions) on a String/int/Uuid/ObjectId property that has an index. This improvement will be especially noticeable if there are a large number of results returned (duplicate values).
     * Significant (~99%) improvement when querying for an exact match on a Timestamp property that has an index.
     * Significant (~99%) improvement when querying for a case insensitive match on a Mixed property that has an index.
     * Moderate (~25%) improvement when querying for an exact match on a Boolean property that has an index.

Copy link
Contributor

@blagoev blagoev left a comment

Choose a reason for hiding this comment

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

The tests were rewritten and are passing so this is ready

@blagoev blagoev merged commit a282aee into main May 24, 2023
@blagoev blagoev deleted the ds/core_upgrade_13.8.0 branch May 24, 2023 15:20
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log messages with timestamps by default Support Core Logging
4 participants