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

[#250] tracing chain density and display in LiveView #261

Merged
merged 2 commits into from Nov 1, 2019
Merged

[#250] tracing chain density and display in LiveView #261

merged 2 commits into from Nov 1, 2019

Conversation

CodiePP
Copy link
Contributor

@CodiePP CodiePP commented Oct 22, 2019

trace and display chain density (see issue #250)
and also the number of connected peers.

@CodiePP CodiePP self-assigned this Oct 22, 2019
@CodiePP
Copy link
Contributor Author

CodiePP commented Oct 23, 2019

one block still missing, if restarted on an existing db:

[iohk.cardano.node.CoreId 0.density.ChainDB:Notice:113] [2019-10-23 07:03:12.92 UTC] density = 0.999537251272559
[iohk.cardano.node.CoreId 0.density.ChainDB:Notice:113] [2019-10-23 07:03:12.92 UTC] density based on blocks:2160 slots:2161

One surplus after restarting from scratch:

[iohk.cardano.node.CoreId 0.density.ChainDB:Notice:108] [2019-10-23 07:27:20.94 UTC] density = 1.1428571428571428
[iohk.cardano.node.CoreId 0.density.ChainDB:Notice:108] [2019-10-23 07:27:20.94 UTC] density based on blocks:8 slots:7

@CodiePP CodiePP requested a review from dcoutts October 25, 2019 11:28
@mrBliss
Copy link
Contributor

mrBliss commented Oct 25, 2019

Does the latest version report the correct chain density?

@CodiePP
Copy link
Contributor Author

CodiePP commented Oct 25, 2019 •
edited

Does the latest version report the correct chain density?

yes, I think so. There are times when the density is below 100%. Might be due to a time lag between updating slot number vs. block number.
(this has been while syncing; not observed though when the chain is fully synced and just updates to the latest block every 20 secs)
node-synced3

@CodiePP CodiePP mentioned this pull request Oct 25, 2019
Closed
dcoutts
dcoutts requested changes Oct 25, 2019
View reviewed changes
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

Spotted why we're getting off by 1.

cardano-node/src/Cardano/Tracing/Tracers.hs Outdated Show resolved Hide resolved
cardano-node/src/Cardano/Tracing/Tracers.hs Outdated Show resolved Hide resolved
@mrBliss
Copy link
Contributor

mrBliss commented Oct 28, 2019

I suggest you rebase this PR so that there's a separate commit for each logical change. Currently, the commits contain a mix of unrelated changes and there are two incorrect versions of the chain density in the history 🙂.

@CodiePP CodiePP added this to Review in progress in ActiveBoard via automation Oct 29, 2019
@CodiePP CodiePP force-pushed the 250-chain-density branch 2 times, most recently from 17a1c6c to 3c32826 Compare October 29, 2019 12:09
@CodiePP
Copy link
Contributor Author

CodiePP commented Oct 29, 2019 •
edited

the exported metrics:

# TYPE cardano_node_metrics_ChainDB_blockNum_int gauge
cardano_node_metrics_ChainDB_blockNum_int  3305544.0
# TYPE cardano_node_metrics_ChainDB_density_real gauge
cardano_node_metrics_ChainDB_density_real  1.0
# TYPE cardano_node_metrics_ChainDB_epoch_int gauge
cardano_node_metrics_ChainDB_epoch_int  153.0
# TYPE cardano_node_metrics_ChainDB_slotInEpoch_int gauge
cardano_node_metrics_ChainDB_slotInEpoch_int  2283.0
# TYPE cardano_node_metrics_ChainDB_slotNum_int gauge
cardano_node_metrics_ChainDB_slotNum_int  3307083.0
...

ActiveBoard automation moved this from Review in progress to Reviewer approved Oct 29, 2019
dcoutts
dcoutts approved these changes Oct 29, 2019
View reviewed changes
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

Looks ok to me.

Both the first and last patch have nix things. Probably should be rebased so all the nix changes are in the one or all in the other, but not both. It's ok to squash it into one since that's easier.

I don't understand the purpose of the second patch. How is it related?

Jimbo4350
Jimbo4350 approved these changes Oct 30, 2019
View reviewed changes
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM! I would put the result of chainInformation into a record type like you suggested @CodiePP

dcoutts
dcoutts approved these changes Oct 30, 2019
View reviewed changes
cardano-node/src/Cardano/Tracing/Tracers.hs Outdated Show resolved Hide resolved
cardano-node/src/Cardano/Tracing/Tracers.hs Outdated Show resolved Hide resolved
cardano-node/src/Cardano/Tracing/Tracers.hs Show resolved Hide resolved
@CodiePP
Copy link
Contributor Author

CodiePP commented Oct 31, 2019

added better description, NamedFieldPuns, field names in constructor use, opened issue

@dcoutts
Copy link
Contributor

dcoutts commented Oct 31, 2019

bors r+

iohk-bors bot added a commit that referenced this pull request Oct 31, 2019
@iohk-bors @CodiePP @deepfire
Merge #261
d6afad9 
261: [#250] tracing chain density and display in LiveView r=dcoutts a=CodiePP

trace and display chain density (see issue #250)
and also the number of connected peers.

Co-authored-by: Alexander Diemand <codieplusplus@apax.net>
Co-authored-by: Kosyrev Serge <serge.kosyrev@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 31, 2019

Build failed

@CodiePP
Copy link
Contributor Author

CodiePP commented Nov 1, 2019

bors r+

iohk-bors bot added a commit that referenced this pull request Nov 1, 2019
@iohk-bors @CodiePP @deepfire
Merge #261
756aec6 
261: [#250] tracing chain density and display in LiveView r=CodiePP a=CodiePP

trace and display chain density (see issue #250)
and also the number of connected peers.

Co-authored-by: Alexander Diemand <codieplusplus@apax.net>
Co-authored-by: Kosyrev Serge <serge.kosyrev@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 1, 2019

Build succeeded

@iohk-bors iohk-bors bot merged commit ea74215 into master Nov 1, 2019
ActiveBoard automation moved this from Reviewer approved to Done Nov 1, 2019
@iohk-bors iohk-bors bot deleted the 250-chain-density branch November 1, 2019 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrBliss mrBliss mrBliss left review comments

@dcoutts dcoutts dcoutts approved these changes

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

Assignees

@CodiePP CodiePP

Labels
None yet
Projects
No open projects
ActiveBoard
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@CodiePP @mrBliss @dcoutts @Jimbo4350 @deepfire