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

use pool production tip to compute performance #1168

Merged
merged 5 commits into from
Dec 12, 2019
Merged

use pool production tip to compute performance #1168

merged 5 commits into from
Dec 12, 2019

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Dec 12, 2019

Issue Number

#1158

Overview

  • I have used pool production tip to compute performanceinstead of the node tip. The production worker might not have caught up with the network yet and therefore, yield statistics that are slightly off

Comments

@KtorZ KtorZ requested a review from Anviking December 12, 2019 10:49
@KtorZ KtorZ self-assigned this Dec 12, 2019
Anviking
Anviking requested changes Dec 12, 2019
View reviewed changes
lib/core/src/Cardano/Pool/Metrics.hs Outdated
<*> (count <$> readPoolProduction nodeEpoch)
<*> readPoolProduction nodeEpoch

let tip = maximumBy (compare `on` view #slotId)
Copy link
Member

Choose a reason for hiding this comment

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

Why not use poolProductionCursor and case on the Maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because side-effects are bad 😬 ?
We already have that information available, so there's no need to use another call for this.

cf my last commit.

lib/core/src/Cardano/Pool/Metrics.hs Outdated

when (Map.null distr || Map.null prod) $ do
computeProgress nodeTip >>= throwE . ErrMetricsIsUnsynced
computeProgress tip >>= throwE . ErrMetricsIsUnsynced
Copy link
Member

Choose a reason for hiding this comment

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

I think here is a case when we actually don't want the poolProductionTip 😅

computeProgress compares the poolProductionTip with this tip, so I think it will only display 100% now.

Copy link
Member

Choose a reason for hiding this comment

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

I see two options:

  • Keep it comparing with the node tip
  • Compare against slot 0 of the node's epoch (should be when the results are actually first displayed 🤔 )

Maybe best to keep it the same for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done 👍

Anviking
Anviking requested changes Dec 12, 2019
View reviewed changes
Copy link
Member

@Anviking Anviking left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks like maximumBy will crash when the follower is un-synced. We should also at least test the PR manually on the testnet.

(but not sure if there is a testnet we are compatible with at the very moment)

lib/core/src/Cardano/Pool/Metrics.hs
@@ -602,14 +599,13 @@ instance ToText StakePoolLayerMsg where
MsgRegistry msg -> toText msg
MsgListStakePoolsBegin -> "Listing stake pools"
MsgMetadataUnavailable -> "Stake pool metadata is unavailable"
MsgComputedProgress (Just dbTip) nodeTip -> mconcat
MsgComputedProgress prodTip nodeTip -> mconcat
[ "The node tip is:\n"
, pretty nodeTip
, ",\nbut the last pool production stored in the db"
Copy link
Member

Choose a reason for hiding this comment

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

This is not exactly true anymore. prodTip is now the greatest slot stored in the DB, where epoch == nodeTip.epoch.

If the DB were to find itself at a greater slotId than the node this distinction would matter… But that shouldn't be possible.

edit: see other comment which is more concerning

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, any epoch stored that would be greater than the nodeTip is irrelevant. That the symptoms of a rollback that hasn't yet happened on the database. In practice, the most recent production tip from the node's epoch is our current state.

lib/core/src/Cardano/Pool/Metrics.hs Outdated
<*> (count <$> readPoolProduction nodeEpoch)
<*> readPoolProduction nodeEpoch

let prodTip = maximumBy (compare `on` view #slotId)
Copy link
Member

Choose a reason for hiding this comment

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

From the docs for maximumBy:

The largest element of a non-empty structure with respect to the given comparison function.

If there are no pool productions in the epoch of the node tip, this will crash. I.e when it should have thrown ErrMetricsIsUnsynced, it will crash instead.

You cannot move this binding to after the when (Map.null distr || Map.null prod check, since computeProgress relies on the prodTip.

I would suggest using poolProductionTip. You can put it inside the atomically and there shouldn't be any downsides:

(distr, prod, dbTip) <- liftIO . atomically $ (,,)
             <$> (Map.fromList <$> readStakeDistribution nodeEpoch)
             <*> readPoolProduction nodeEpoch
             <*> (readPoolProductionCursor 1 >>= toMaybe)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm. Good point!

@Anviking
Copy link
Member

bors try

iohk-bors bot added a commit that referenced this pull request Dec 12, 2019
@iohk-bors
Try #1168:
53f84dc
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 12, 2019

try

Build failed

@KtorZ
Copy link
Member Author

KtorZ commented Dec 12, 2019

@Anviking

  • Fixed tip calculation (using the db cursor in the end 🤷‍♂️)
  • Added two basic unit tests (one to test the case where there's no production, indeed, it crashed. And one to test that the calculation works as expected).

Anviking
Anviking approved these changes Dec 12, 2019
View reviewed changes
Copy link
Member

@Anviking Anviking left a comment

Choose a reason for hiding this comment

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

lgtm

@Anviking
Copy link
Member

bors r+

1 similar comment
@KtorZ
Copy link
Member Author

KtorZ commented Dec 12, 2019

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 12, 2019

Already running a review

iohk-bors bot added a commit that referenced this pull request Dec 12, 2019
@iohk-bors @KtorZ
Merge #1168
4babeb2 
1168: use pool production tip to compute performance r=Anviking a=KtorZ

# Issue Number

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

#1158 

# Overview

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

- [ ] I have used pool production tip to compute performanceinstead of the node tip. The production worker might not have caught up with the network yet and therefore, yield statistics that are slightly off



# 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
-->


Co-authored-by: KtorZ <matthias.benkort@gmail.com>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 12, 2019

Build failed

@KtorZ KtorZ force-pushed the KtorZ/1158/use-production-tip-for-perf branch from c12421a to 31123c5 Compare December 12, 2019 16:09
@KtorZ
Copy link
Member Author

KtorZ commented Dec 12, 2019

bors r+

iohk-bors bot added a commit that referenced this pull request Dec 12, 2019
@iohk-bors @KtorZ
Merge #1168
cc988b0 
1168: use pool production tip to compute performance r=KtorZ a=KtorZ

# Issue Number

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

#1158 

# Overview

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

- [ ] I have used pool production tip to compute performanceinstead of the node tip. The production worker might not have caught up with the network yet and therefore, yield statistics that are slightly off



# 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
-->


Co-authored-by: KtorZ <matthias.benkort@gmail.com>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 12, 2019

Build failed

@KtorZ
Copy link
Member Author

KtorZ commented Dec 12, 2019

bors r+

iohk-bors bot added a commit that referenced this pull request Dec 12, 2019
@iohk-bors @KtorZ
Merge #1168
ef72522 
1168: use pool production tip to compute performance r=KtorZ a=KtorZ

# Issue Number

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

#1158 

# Overview

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

- [ ] I have used pool production tip to compute performanceinstead of the node tip. The production worker might not have caught up with the network yet and therefore, yield statistics that are slightly off



# 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
-->


Co-authored-by: KtorZ <matthias.benkort@gmail.com>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 12, 2019

Build succeeded

@iohk-bors iohk-bors bot merged commit 31123c5 into master Dec 12, 2019
@Anviking Anviking deleted the KtorZ/1158/use-production-tip-for-perf branch December 12, 2019 20:25
@Anviking Anviking mentioned this pull request Dec 13, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Anviking Anviking Anviking approved these changes

Assignees

@KtorZ KtorZ

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KtorZ @Anviking