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

Rudimentary diffusion socket logging #2746

Merged
merged 2 commits into from
Nov 29, 2020
Merged

Rudimentary diffusion socket logging #2746

merged 2 commits into from
Nov 29, 2020

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Nov 16, 2020 •
edited
Loading

No description provided.

@mrBliss
Copy link
Contributor

mrBliss commented Nov 16, 2020 •
edited
Loading

FYI, if this branch is just for yourself, you can just push the branch without opening a PR. Pushing to a branch with a PR creates notifications for the people subscribed to the repo, pushing to a branch without a PR doesn't 🙂.

@newhoggy newhoggy force-pushed the rudimentary-diffusion-logging branch 4 times, most recently from 366c9da to 52a2916 Compare November 17, 2020 05:21
@newhoggy newhoggy changed the title [For Test Only] Rudimentary diffusion logging Rudimentary diffusion socket logging Nov 17, 2020
@newhoggy
Copy link
Contributor Author

newhoggy commented Nov 17, 2020 •
edited
Loading

I made some changes and am upgrading to a proper PR.

@newhoggy newhoggy marked this pull request as ready for review November 17, 2020 06:03
coot
coot previously requested changes Nov 17, 2020
View reviewed changes
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
@newhoggy
Copy link
Contributor Author

Oops, sorry, I had stylish-haskell turned on. Will revert the stylistic changes.

@newhoggy newhoggy requested a review from coot November 17, 2020 22:05
@newhoggy newhoggy dismissed coot’s stale review November 18, 2020 02:05

Comments addressed

coot
coot previously requested changes Nov 18, 2020
View reviewed changes
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

A bit more nitpicking ;).

ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
@newhoggy newhoggy force-pushed the rudimentary-diffusion-logging branch 2 times, most recently from 7485ad0 to a2b56f7 Compare November 19, 2020 00:17
@newhoggy newhoggy requested a review from coot November 19, 2020 00:19
@newhoggy newhoggy dismissed coot’s stale review November 19, 2020 00:19

Comments addressed.

@newhoggy newhoggy force-pushed the rudimentary-diffusion-logging branch 4 times, most recently from fa6d505 to b1b7deb Compare November 19, 2020 12:34
@newhoggy newhoggy force-pushed the rudimentary-diffusion-logging branch 2 times, most recently from b487f75 to 73b933f Compare November 19, 2020 22:49
@newhoggy newhoggy requested review from coot and karknu November 19, 2020 23:21
@karknu
Copy link
Contributor

karknu commented Nov 20, 2020

I think you should add the windows' UnsupportedReadySocket error too.

karknu
karknu approved these changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@karknu karknu left a comment

Choose a reason for hiding this comment

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

LGTM. Just some minor things.

ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
@newhoggy newhoggy force-pushed the rudimentary-diffusion-logging branch 2 times, most recently from ece0ddd to 0e33a53 Compare November 23, 2020 06:54
@newhoggy newhoggy requested a review from coot November 24, 2020 09:07
@newhoggy newhoggy force-pushed the rudimentary-diffusion-logging branch from 0e33a53 to de06585 Compare November 24, 2020 09:08
coot
coot previously requested changes Nov 24, 2020
View reviewed changes
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
@newhoggy newhoggy force-pushed the rudimentary-diffusion-logging branch 2 times, most recently from 1241fb8 to 4fc0390 Compare November 24, 2020 21:15
@newhoggy newhoggy requested a review from coot November 24, 2020 23:19
@newhoggy newhoggy dismissed coot’s stale review November 24, 2020 23:19

Comments addressed

@newhoggy newhoggy force-pushed the rudimentary-diffusion-logging branch from 4fc0390 to 39955e7 Compare November 25, 2020 00:18
@newhoggy
Copy link
Contributor Author 

[macbook-:cardano.node.DiffusionInitializationTracer:Info:43] [2020-11-25 01:52:25.98 UTC] [String "test-059f56f3348ccd9e/socket/node-0",String "CreateSystemdSocketForSnocketPath"]
[macbook-:cardano.node.DiffusionInitializationTracer:Info:43] [2020-11-25 01:52:25.98 UTC] [String "test-059f56f3348ccd9e/socket/node-0",String "CreatedSystemdSocketForSnocketPath"]
[macbook-:cardano.node.DiffusionInitializationTracer:Info:47] [2020-11-25 01:52:25.98 UTC] [String "[::]:59250",String "CreatingServerSocket"]
[macbook-:cardano.node.DiffusionInitializationTracer:Info:47] [2020-11-25 01:52:25.98 UTC] [String "[::]:59250",String "ConfiguringServerSocket"]
[macbook-:cardano.node.DiffusionInitializationTracer:Info:43] [2020-11-25 01:52:25.98 UTC] [String "test-059f56f3348ccd9e/socket/node-0",String "<file-descriptor: 29>",String "ConfiguringLocalSocket"]
[macbook-:cardano.node.DiffusionInitializationTracer:Info:46] [2020-11-25 01:52:25.98 UTC] [String "0.0.0.0:59250",String "CreatingServerSocket"]
[macbook-:cardano.node.DiffusionInitializationTracer:Info:46] [2020-11-25 01:52:25.98 UTC] [String "0.0.0.0:59250",String "ConfiguringServerSocket"]
[macbook-:cardano.node.DiffusionInitializationTracer:Info:47] [2020-11-25 01:52:25.98 UTC] [String "RunServer"]
[macbook-:cardano.node.DiffusionInitializationTracer:Info:46] [2020-11-25 01:52:25.98 UTC] [String "RunServer"]

@newhoggy
Copy link
Contributor Author

This is how it looks now:

[macbook-:cardano.node.DiffusionInitializationTracer:Info:43] [2020-11-25 01:52:25.98 UTC] [String "test-059f56f3348ccd9e/socket/node-0",String "CreateSystemdSocketForSnocketPath"]
[macbook-:cardano.node.DiffusionInitializationTracer:Info:43] [2020-11-25 01:52:25.98 UTC] [String "test-059f56f3348ccd9e/socket/node-0",String "CreatedSystemdSocketForSnocketPath"]
[macbook-:cardano.node.DiffusionInitializationTracer:Info:47] [2020-11-25 01:52:25.98 UTC] [String "[::]:59250",String "CreatingServerSocket"]
[macbook-:cardano.node.DiffusionInitializationTracer:Info:47] [2020-11-25 01:52:25.98 UTC] [String "[::]:59250",String "ConfiguringServerSocket"]
[macbook-:cardano.node.DiffusionInitializationTracer:Info:43] [2020-11-25 01:52:25.98 UTC] [String "test-059f56f3348ccd9e/socket/node-0",String "<file-descriptor: 29>",String "ConfiguringLocalSocket"]
[macbook-:cardano.node.DiffusionInitializationTracer:Info:46] [2020-11-25 01:52:25.98 UTC] [String "0.0.0.0:59250",String "CreatingServerSocket"]
[macbook-:cardano.node.DiffusionInitializationTracer:Info:46] [2020-11-25 01:52:25.98 UTC] [String "0.0.0.0:59250",String "ConfiguringServerSocket"]
[macbook-:cardano.node.DiffusionInitializationTracer:Info:47] [2020-11-25 01:52:25.98 UTC] [String "RunServer"]
[macbook-:cardano.node.DiffusionInitializationTracer:Info:46] [2020-11-25 01:52:25.98 UTC] [String "RunServer"]

coot
coot reviewed Nov 25, 2020
View reviewed changes
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

A few more comments

ouroboros-network-framework/src/Ouroboros/Network/Snocket.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion.hs Outdated

data DiffusionInitializationTracer
= RunServer
| RunLocalServer
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add SockAddr and LocalAddress on both RunServer and RunLocalServer. These are probably the only two messages that should be logged with Notice severity, all others are either Info or Debug messages so it makes sense to show the addresses on both of them.

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 wasn't able to get a SockAddr or LocalAddress for these points of logging, so I used FileDescriptor instead. Please advise.

Copy link
Contributor

@coot coot Nov 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

For runServer you can do this:

          addr <- case address of
               -- If a socket was provided it should be ready to accept
               Left sd -> getLocalName sn sd
               Right addr -> do
                 traceWith dtDiffusionInitializationTracer $ ConfiguringServerSocket addr
                 Snocket.bind sn sd addr
                 return addr
                 Snocket.listen sn sd

          traceWith dtDiffusionInitializationTracer (RunServer addr)

Similar thing will work in runLocalServer

Copy link
Contributor

Choose a reason for hiding this comment

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

Snocket haddoc documentation could help you find the solution (though it could be improved).

ouroboros-network/src/Ouroboros/Network/Diffusion.hs
@@ -350,17 +384,21 @@ runDataDiffusion tracers
(
case address of
Left sd -> return sd
Right a -> Snocket.open sn (Snocket.addrFamily sn a)
Right addr -> do
traceWith dtDiffusionInitializationTracer $ CreatingServerSocket addr
Copy link
Contributor

Choose a reason for hiding this comment

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

The same note as for the other Creating... message applies, e.g. if things go wrong the diffusion will error.

ouroboros-network/src/Ouroboros/Network/Diffusion.hs
)
(Snocket.close sn) -- We close the socket here, even if it was provided for us.
(\sd -> do

case address of
Left _ -> pure () -- If a socket was provided it should be ready to accept
Right addr -> do
traceWith dtDiffusionInitializationTracer $ ConfiguringServerSocket addr
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly for this message, if bind errors diffusion stops => node stops. The error should be logged by the node.

@newhoggy newhoggy force-pushed the rudimentary-diffusion-logging branch 8 times, most recently from cadd37b to 776d1bd Compare November 27, 2020 10:04
@newhoggy newhoggy force-pushed the rudimentary-diffusion-logging branch from 776d1bd to 00f1463 Compare November 27, 2020 11:08
coot
coot approved these changes Nov 27, 2020
View reviewed changes
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

Thanks, I left one layout comment, otherwise it can be merged.

ouroboros-network/src/Ouroboros/Network/Diffusion.hs Show resolved Hide resolved
@newhoggy newhoggy force-pushed the rudimentary-diffusion-logging branch from 00f1463 to 6719c87 Compare November 29, 2020 21:48
@newhoggy
Copy link
Contributor Author

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 29, 2020

Build succeeded:

@iohk-bors iohk-bors bot merged commit 680b172 into master Nov 29, 2020
@iohk-bors iohk-bors bot deleted the rudimentary-diffusion-logging branch November 29, 2020 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coot coot coot approved these changes

@karknu karknu karknu approved these changes

@mrBliss mrBliss Awaiting requested review from mrBliss

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@newhoggy @mrBliss @karknu @coot