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

[ETCM-273] Change logging behaviour and increase handshake timeout to… #758

Merged
merged 2 commits into from
Oct 29, 2020

Conversation

mmrozek
Copy link
Contributor

@mmrozek mmrozek commented Oct 26, 2020

… investigate connection issue

Description

Temporary change handshake timeout to check if rhe testnet is able to receive any message from outside world

@mmrozek mmrozek requested a review from mirkoAlic October 26, 2020 14:56
<logger name="io.iohk.ethereum.blockchain.sync.SyncController" level="INFO" />
<logger name="io.iohk.ethereum.network.PeerActor" level="INFO" />
<logger name="io.iohk.ethereum.network.PeerActor" level="DEBUG" />
Copy link

Choose a reason for hiding this comment

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

Should we enable this permanently and remove this lines? (as we are setting the DEBUG option above)

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 think it should be a temporary change only. When we will have more peers we will be flooded by logs from there

Copy link

Choose a reason for hiding this comment

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

I'm not sure how temporal this will be 🤔 we still have this sort of logging on the other project

<logger name="io.iohk.ethereum.blockchain.sync.SyncController" level="INFO" />
<logger name="io.iohk.ethereum.network.PeerActor" level="INFO" />
<logger name="io.iohk.ethereum.network.PeerActor" level="DEBUG" />
Copy link

Choose a reason for hiding this comment

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

We also have some bothering logs of all the messages sent, should we remove them? Wdyt @mirkoAlic @mmrozek ?

Copy link
Contributor

@mirkoAlic mirkoAlic Oct 27, 2020

Choose a reason for hiding this comment

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

I think we should re-think our log polices, IMHO:

  • info: Display logs that are meaningful for someone that runs the client and more a less wants to see what is going on behind the scene.
  • debug: Log everything you could

By following those definitions, i think our issue is that PeerActor or network in general is no logging relevant information at info level.

Copy link

Choose a reason for hiding this comment

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

I agree, but I think the only thing that we can do about this for now is pay closer attention to the logs changed or added on new PRs (till maybe someone starts using Mantis and complains about something like that). We should probably better define what a client would like to see 🤔

Back to the question at hand, I don't see the logs of all the messages sent/received to be so useful (we haven't required them till now), or at least we should keep them in a single line for those that don't use the JSON formatting (I don't do so locally)

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 agree with @ntallar. I don't remember when we needed logs from PeerActor and when we will be sure after merge that node doesn't receive any message we won't need it anymore also ;)

Copy link

@ntallar ntallar left a comment

Choose a reason for hiding this comment

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

LGTM!

 .\\            //.
. \ \          / /.
.\  ,\     /` /,.-
 -.   \  /'/ /  .
 ` -   `-'  \  -
   '.       /.\`
      -    .-
      :`//.'
      .`.'
      .' BP 

We need this for our cluster, so the remainder comments will be addressed on a new PR

@mirkoAlic mirkoAlic merged commit 5d754f6 into develop Oct 29, 2020
@mirkoAlic mirkoAlic deleted the etcm-273-testnet-connection-issue branch October 29, 2020 13:51
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.

3 participants