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.

Sign up for GitHub

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

Jump to bottom

Overlay logging improvement #2276

Merged
merged 4 commits into from
Sep 19, 2019
Merged

Overlay logging improvement #2276

merged 4 commits into from
Sep 19, 2019

Conversation

MonsieurNicolas
Copy link
Contributor

Description

This PR:

  • fixes a few logging issues in overlay
    • logging of uninitialized member variables during Hello
    • missing context, useful when running tests that spin up multiple applications
  • bump overlay version (allows to boot old core versions later on that can't even catchup to the network post protocol upgrade)
  • adds a new property to connections to report how long a connection has been active (only used for reporting in this PR)

@MonsieurNicolas MonsieurNicolas added this to In progress in v12.0.0 via automation Sep 17, 2019
jonjove
jonjove reviewed Sep 17, 2019
View reviewed changes
src/overlay/OverlayManagerImpl.cpp Outdated
@@ -201,10 +209,33 @@ OverlayManagerImpl::PeersList::acceptAuthenticatedPeer(Peer::pointer peer)

CLOG(INFO, "Overlay") << "Non preferred " << mDirectionString
<< " authenticated peer " << peer->toString()
<< " rejected because all available slots are taken.";
<< " rejected because all available slots are taken."
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these lines are out of order. Adding a function for this also would have avoided this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same thing: I do want this "@..." thing at the end of the line

src/overlay/OverlayManagerImpl.cpp Outdated
@@ -113,7 +114,8 @@ OverlayManagerImpl::PeersList::removePeer(Peer* peer)
if (pendingIt != std::end(mPending))
{
CLOG(DEBUG, "Overlay") << "Dropping pending " << mDirectionString
<< " peer: " << peer->toString();
<< " peer: " << peer->toString() << " @"
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intend for no space between "@" and the port? This is the case in every log line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, makes it simpler to grep

src/overlay/OverlayManagerImpl.cpp Outdated
@@ -142,7 +146,8 @@ OverlayManagerImpl::PeersList::moveToAuthenticated(Peer::pointer peer)
{
CLOG(WARNING, "Overlay")
<< "Trying to move non-pending " << mDirectionString << " peer "
<< peer->toString() << " to authenticated list";
<< peer->toString() << " to authenticated list"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you meant to have "to authenticated list" after the port. In order to avoid issues like this, I think it would be better to use a function like std::string Peer::toStringWithPort() const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, the port is not related to peer but to the application, and I'd rather keep this at the end of the line to limit the confusion

src/overlay/OverlayManagerImpl.cpp
std::stringstream pending, authenticated;
for (auto p : mPending)
{
pending << p->toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to add the port here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no: port is unrelated to p and there is already context from the INFO entry above

@MonsieurNicolas MonsieurNicolas force-pushed the overlayLoggingBumpVersion branch 2 times, most recently from da59330 to 45cd965 Compare September 18, 2019 22:18
@jonjove
Copy link
Contributor

jonjove commented Sep 18, 2019

r+ 45cd965

@MonsieurNicolas
Copy link
Contributor Author

@latobarita: retry

@MonsieurNicolas
Copy link
Contributor Author

@latobarita: retry

@MonsieurNicolas
Copy link
Contributor Author

r+ 04dcaf5530e5a0cb5a3704b24f14bc9520cfcc61

@MonsieurNicolas
Copy link
Contributor Author

Issue was that I forgot to update one of the tests when I removed some of the constraints on PeerRecord.

@MonsieurNicolas
Copy link
Contributor Author

r+ 6556353

latobarita added a commit that referenced this pull request Sep 19, 2019
@latobarita
Merge pull request #2276 from MonsieurNicolas/overlayLoggingBumpVersion
ec62e52 
Overlay logging improvement

Reviewed-by: MonsieurNicolas
@latobarita latobarita merged commit 6556353 into stellar:master Sep 19, 2019
v12.0.0 automation moved this from In progress to Done Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonjove jonjove jonjove left review comments

Assignees
No one assigned
Labels
None yet
Projects
No open projects
v12.0.0
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@MonsieurNicolas @jonjove @latobarita