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

CAD-430, CAD-429: live peer list & other TUI improvements #493

Merged
merged 3 commits into from
Jan 30, 2020
Merged

CAD-430, CAD-429: live peer list & other TUI improvements #493

merged 3 commits into from
Jan 30, 2020

Conversation

deepfire
Copy link
Contributor

@deepfire deepfire commented Jan 23, 2020 •
edited
Loading

TUI changes:

  1. give the TUI direct access to the NodeKernel, to allow easier & richer data queries
  2. added a peer list screen, accessible by the 'p' key
  3. minor string changes
  4. compressed the mempool bars in one line
  5. a bunch of minor refactoring

Screenshot from 2020-01-30 17-01-48

@deepfire deepfire self-assigned this Jan 23, 2020
@deepfire deepfire added the WIP Work In Progress (cannot be merged yet) label Jan 23, 2020
@deepfire deepfire changed the title CAD-430: live peer list CAD-430: live peer list (WIP) Jan 23, 2020
@deepfire deepfire force-pushed the cad-430-live-peer-list branch 10 times, most recently from 675a4d8 to d3ef40d Compare January 27, 2020 13:28
@deepfire deepfire removed the WIP Work In Progress (cannot be merged yet) label Jan 27, 2020
@deepfire deepfire changed the title CAD-430: live peer list (WIP) CAD-430: live peer list Jan 27, 2020
@deepfire deepfire force-pushed the cad-430-live-peer-list branch from f35eef8 to 0b71e51 Compare January 27, 2020 14:09
@CodiePP CodiePP requested a review from ksaric January 28, 2020 09:59
@deepfire deepfire added the WIP Work In Progress (cannot be merged yet) label Jan 28, 2020
@deepfire deepfire changed the title CAD-430: live peer list CAD-430, CAD-429: live peer list & other TUI improvements Jan 29, 2020
@CodiePP CodiePP requested a review from karknu January 29, 2020 09:25
@deepfire deepfire force-pushed the cad-430-live-peer-list branch from a8f444f to ec704f0 Compare January 29, 2020 09:25
@karknu
Copy link
Contributor

karknu commented Jan 29, 2020

<3
Can you add uptime, and epoch/slot to the Peer view too?

@deepfire deepfire force-pushed the cad-430-live-peer-list branch 3 times, most recently from c5c3587 to 967c68b Compare January 29, 2020 14:01
@deepfire
Copy link
Contributor Author

@karknu, done! : -)

@deepfire deepfire force-pushed the cad-430-live-peer-list branch from 967c68b to f793fbf Compare January 29, 2020 14:16
CodiePP
CodiePP suggested changes Jan 30, 2020
View reviewed changes
Copy link
Contributor

@CodiePP CodiePP left a comment

Choose a reason for hiding this comment

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

minor changes needed

cardano-node/src/Cardano/Node/TUI/LiveView.hs Outdated Show resolved Hide resolved
cardano-node/src/Cardano/Node/TUI/LiveView.hs Outdated Show resolved Hide resolved
@deepfire deepfire force-pushed the cad-430-live-peer-list branch 11 times, most recently from 4456547 to 9b68a78 Compare January 30, 2020 14:07
njd42
njd42 reviewed Jan 30, 2020
View reviewed changes
Copy link

@njd42 njd42 left a comment

Choose a reason for hiding this comment

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

Logic looks fine - but I think suggested changes would help maintainabluty

cardano-node/src/Cardano/Node/TUI/LiveView.hs Outdated
, lvsUIThread :: !LiveViewThread
, lvsMetricsThread :: !LiveViewThread
, lvsNodeThread :: !LiveViewThread

, lvsNodeKernel :: Maybe (LVNodeKernel blk)
Copy link

Choose a reason for hiding this comment

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

Shouldn't this be strict?

Copy link
Contributor

Choose a reason for hiding this comment

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

Careful that simply adding a ! on a Maybe is pretty useless.

Copy link

Choose a reason for hiding this comment

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

I would agree - except - given that this data structure is also an instance of NoUnexpectedThunks you might need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, by adding SMaybe.

cardano-node/src/Cardano/Node/TUI/LiveView.hs Outdated
return $ lvs { lvsNetworkUsageInCurr = currentNetRate
, lvsNetworkUsageInPerc = (currentNetRate / (maxNetRate / 100.0)) / 100.0
, lvsNetworkUsageInLast = inBytes
, lvsNetworkUsageInNs = currentTimeInNs
, lvsNetworkUsageInMax = maxNetRate
, lvsUpTime = diffUTCTime (tstamp meta) (lvsStartTime lvs)
, lvsPeers = Map.elems . flip Map.mapMaybeWithKey candidates $
Copy link

Choose a reason for hiding this comment

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

You might want to pull this calculation out into a supporting function and put it near the newtype above - then it could be clearly documented that this is dependent on details of the wrapped data structure - just for longer term maintenance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, good point!

cardano-node/src/Cardano/Node/TUI/LiveView.hs Outdated
withOneDecimal :: (Fractional f, Ord f, Show f) => f -> String
withOneDecimal x
| x < 0.1 = "0.0"
| otherwise = uncurry (<>) . (id *** take 2) $ span (/= '.') (show x)
Copy link

Choose a reason for hiding this comment

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

this seems to be the same basic functionality as those in Numeric ie ShowFFloat

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

@deepfire deepfire force-pushed the cad-430-live-peer-list branch from 9b68a78 to 6de835b Compare January 30, 2020 16:02
@deepfire deepfire requested a review from Jimbo4350 January 30, 2020 16:04
@deepfire
CAD-430 | LiveView: peer list & other improvements
aa8f66a 
# Conflicts:
#	cardano-node/src/Cardano/Node/TUI/LiveView.hs
@deepfire deepfire force-pushed the cad-430-live-peer-list branch from 6de835b to aa8f66a Compare January 30, 2020 16:07
@deepfire deepfire force-pushed the cad-430-live-peer-list branch from 78d0b77 to 7da74b1 Compare January 30, 2020 17:09
@deepfire
Copy link
Contributor Author

deepfire commented Jan 30, 2020 •
edited
Loading

@CodiePP, this is the plot I'm getting after 45 minutes of running shelley-testnet-live.sh:

Screenshot from 2020-01-30 20-11-10

Screenshot from 2020-01-30 20-13-05

..so the definite leaks are:

  • Brick.Main
  • Data.ByteString.Lazy (possibly legitimate)
  • Ouroboros.Consensus.Ledger.Byron.Block (likely legitimate)
  • Data.ByteArray.Methods (possibly legitimate)
  • Basement.Block.Base (possibly legitimate)
  • SYSTEM -- the worst one by size, and I'm seeing 0.48M -> 1.72M over an hour of scripts/shelley-testnet-live.sh

@deepfire
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Jan 30, 2020
@iohk-bors @deepfire
Merge #493
7421a69 
493: CAD-430, CAD-429: live peer list & other TUI improvements r=deepfire a=deepfire

TUI changes:

1. give the TUI direct access to the `NodeKernel`, to allow easier & richer data queries
1. added a peer list screen, accessible by the 'p' key
1. minor string changes
1. compressed the mempool bars in one line
1. a bunch of minor refactoring

![Screenshot from 2020-01-30 17-01-48](https://user-images.githubusercontent.com/452652/73456064-42939880-4382-11ea-912d-345162bd8158.png)


Co-authored-by: Kosyrev Serge <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 30, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 7da74b1 into master Jan 30, 2020
@iohk-bors iohk-bors bot deleted the cad-430-live-peer-list branch January 30, 2020 17:56
@mrBliss
Copy link
Contributor

mrBliss commented Jan 31, 2020

..so the definite leaks are:

[..]

bors r+

I didn't follow this PR in detail, but your response to "definite leaks" is "bors it"? 😕

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njd42 njd42 njd42 left review comments

@ksaric ksaric ksaric left review comments

@dcoutts dcoutts dcoutts left review comments

@edsko edsko edsko left review comments

@erikd erikd erikd left review comments

@CodiePP CodiePP CodiePP approved these changes

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

@denisshevchenko denisshevchenko Awaiting requested review from denisshevchenko

@karknu karknu Awaiting requested review from karknu

Assignees

@deepfire deepfire

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@deepfire @karknu @mrBliss @erikd @edsko @dcoutts @ksaric @Jimbo4350 @CodiePP @njd42