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

Add more debug-level chain-sync- and follow traces #1826

Merged
merged 3 commits into from
Jul 10, 2020

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Jun 29, 2020

Issue Number

Related to / was used to debug #1794

Overview

The chain-following code is complex. We usually log when the follow function asks us to roll forward or backward, but we don't have traces for the underlying logic.

  • Trace when follow ignores (hopefully) idempotent rollbacks, and why
  • Trace low-level chain-sync commands

Comments

@Anviking Anviking self-assigned this Jun 29, 2020
@Anviking Anviking added the IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG label Jun 29, 2020
Base automatically changed from rvl/adp-302/db-reward-balance to master June 30, 2020 09:36
step delay0 hasRolledForward cursor'
(sl, _:_, False) | sl == slotId (last cps) -> do
traceWith tr $ MsgWillIgnoreRollback sl "initial rollback, \
\rollback point equals the last checkpoing"
Copy link
Member

Choose a reason for hiding this comment

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

checkpoint*

@@ -348,6 +361,7 @@ chainSyncWithBlocks fromTip queue responseBuffer =
-> P.ClientStNext n block (Tip block) m Void
collectResponses respond blocks Zero = P.ClientStNext
{ P.recvMsgRollForward = \block tip -> do
traceWith tr $ MsgChainRollForward block
Copy link
Member

Choose a reason for hiding this comment

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

Logging every rollForward sounds like something that would cause great slowness. Have you measured the syncing time against mainnet and a non-null tracer on the Byron wallets?

Copy link
Member Author

Choose a reason for hiding this comment

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

These logs should't be visible per default since they are marked Debug — but perhaps still worth a check. Haven't tried yet…

Copy link
Member

Choose a reason for hiding this comment

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

We had the case in the past, where simply having a logger would cause tremendous performance regression because the system still needs, on each block, to assess whether or not it should be logged.

@Anviking
Copy link
Member Author

Anviking commented Jul 9, 2020

This pr

restore mainnet seq: 387.6 s
--
  | restore mainnet 1% ownership: 3073 s
  | restore mainnet 2% ownership: 6333 s

Recent build from master:

restore mainnet seq: 385.9 s
--
  | restore mainnet 1% ownership: 3027 s
  | restore mainnet 2% ownership: 6241 s

Looks like no noticeable change. @KtorZ, thoughts?

@KtorZ
Copy link
Member

KtorZ commented Jul 10, 2020

Looks convincing enough :)

@KtorZ KtorZ force-pushed the anviking/more-traces branch from 4d782b3 to 3f8cdaf Compare July 10, 2020 18:11
@KtorZ
Copy link
Member

KtorZ commented Jul 10, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request Jul 10, 2020
1826: Add more debug-level chain-sync- and follow traces r=KtorZ a=Anviking

# Issue Number

Related to / was used to debug #1794 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

The chain-following code is complex. We usually log when the `follow` function asks us to roll forward or backward, but we don't have traces for the underlying logic.

- [x] Trace when `follow` ignores (hopefully) idempotent rollbacks, and why
- [x] Trace low-level chain-sync commands


# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


1887: Rename our SlotNo to SlotInEpoch r=KtorZ a=Anviking


# Issue Number

#1868
<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Renamed `SlotNo` to `SlotInEpoch` (for consistency with other components)


# Comments

- `W.SlotNo` == `Cardano.SlotInEpoch` /= `Cardano.SlotNo`

```
Epoch       0   1   2   3   4
SlotNo*     0 1 2 3 4 5 6 7 8 9
SlotInEpoch 0 1 0 1 0 1 0 1 0 1
```

*) I.e. the one defined in cardano-base and used in ourobouros-consensus.


<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


1895: hydra: Prevent caching of shelley integration test failures r=KtorZ a=rvl

### Issue Number

Not sure.

### Overview

This change causes integration tests to be re-run whenever the git revision changes, even if everything else is identical. Since these tests tend to fail a lot, we don't want to cache false failures.

Also increase the minimum severity for integration test logging, because debug level produces quite a lot of output.


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: Rodney Lorrimar <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 10, 2020

Build failed (retrying...)

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 10, 2020

@iohk-bors iohk-bors bot merged commit 5e6befb into master Jul 10, 2020
@iohk-bors iohk-bors bot deleted the anviking/more-traces branch July 10, 2020 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants