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

CBL-6155: Improve LiteCore Log #2164

Open
wants to merge 7 commits into
base: release/3.2
Choose a base branch
from
Open

CBL-6155: Improve LiteCore Log #2164

wants to merge 7 commits into from

Conversation

velicuvlad
Copy link
Contributor

@cbl-bot
Copy link

cbl-bot commented Oct 15, 2024

Code Coverage Results:

Type Percentage
branches 67.9
functions 79.28
instantiations 35.26
lines 79.22
regions 75.15

@callumbirks
Copy link
Contributor

Ignoring Blackduck failure. It complains because we have changed mbedtls without bumping the version

@@ -104,7 +104,6 @@ namespace litecore::repl {
DatabaseImpl* impl = asInternal(db);
return impl->dataFile()->loggingName();
});
logInfo("DB=%s Instantiated %s", dbLogName.c_str(), string(*options).c_str());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this info not important or is it logged the same info somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the same exact info is logged on Starting Replicator ..., which is the next logged line after is Instantiated in every log I've seen pretty much

logInfo("Starting Replicator %s with config: {%s}\n", _replicator->loggingName().c_str(),
std::string(*_options).c_str());
logInfo("Starting Replicator %s with config: {%s} with endpoing: %.*s", _replicator->loggingName().c_str(),
std::string(*_options).c_str(), SPLAT(_replicator->remoteURL()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

What will happen if the replicator is for db-to-db or p2p sync?

Copy link
Contributor

Choose a reason for hiding this comment

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

Whenever Replicator is instantiated it sets _remoteURL = webSocket->url(). So I believe this would be something like ws://loopback. I will run test and verify.

Copy link
Contributor

Choose a reason for hiding this comment

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

DB to DB endpoint looks like file:////var/folders/dq/4702h1qn7wdclczftpkmv1dr0000gp/T/LiteCore_Tests_1729177323.cblite2/cbl_core_test2.cblite2.cblite2/

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah this is ReplicatorImpl so for P2P the implementation is by platform - so this exact log isn't in the P2P replicators.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what we should do about this - @pasin any ideas? Should LiteCore be logging the Replicator options from P2P replicator like it does for remote replicator?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I understand this. The c4ReplicatorImpl's start() should be used by any types of the replicator. I think we can focus on what is being logged here with the remoteURL() with the replicator with message endpoint is used. Maybe the easiest way to find out is to look at the log when running the message endpoint replicator tests from the platform (e.g. iOS) that uses LiteCore in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

endpoint: x-msg-endpt:////_blipsync
endpoint: wss://localhost:55369/otherdb/_blipsync
endpoint: file:////var/folders/q7/sjmz4q8n073f1lynlw5fw70w0000gp/T/CouchbaseLite_EE/otherdb.cblite2/

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've also seen this for the passive replicator
endpoint: x-msg-conn:///0x104397e20

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is good enough.

Replicator/c4ReplicatorImpl.hh Outdated Show resolved Hide resolved
logInfo("Starting Replicator %s with config: {%s}\n", _replicator->loggingName().c_str(),
std::string(*_options).c_str());
logInfo("Starting Replicator %s with config: {%s} with endpoing: %.*s", _replicator->loggingName().c_str(),
std::string(*_options).c_str(), SPLAT(_replicator->remoteURL()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what we should do about this - @pasin any ideas? Should LiteCore be logging the Replicator options from P2P replicator like it does for remote replicator?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants