Skip to content

[ETCM-555] Add metrics on the time taken to download blocks (headers, bodies and receipts) and also MPT nodes. Expose in Grafana other already existing metrics.#905

Merged
leo-bogastry merged 2 commits intodevelopfrom
feature/ETCM-555-add-fastsync-metrics-on-blocks
Feb 2, 2021
Merged

[ETCM-555] Add metrics on the time taken to download blocks (headers, bodies and receipts) and also MPT nodes. Expose in Grafana other already existing metrics.#905
leo-bogastry merged 2 commits intodevelopfrom
feature/ETCM-555-add-fastsync-metrics-on-blocks

Conversation

@leo-bogastry
Copy link
Copy Markdown
Contributor

@leo-bogastry leo-bogastry commented Jan 26, 2021 •
edited
Loading

Description

The goal of this PR is help measuring FastSync performance.

Proposed Solution

Some metrics were already in place but were not available in the local docker-compose with Grafana setup. Following metrics added to Grafana:

fastsync.block.pivotBlock.number.gauge
fastsync.block.bestFullBlock.number.gauge
astsync.block.bestHeader.number.gauge
fastsync.state.totalNodes.gauge
fastsync.state.downloadedNodes.gauge

Other measurement were already being done, but were not available on a metric (time elapsed during the download of headers bodies and receipts). Following metrics added:

fastsync.block.downloadBlockHeaders.timer
fastsync.block.downloadBlockBodies.timer
fastsync.block.downloadBlockReceipts.timer
fastsync.state.downloadState.timer

The elapsed time is now being calculated using System.nanoTime() instead of System.currentTimeMillis(), because System.nanoTime() is more reliable when calculated elapsed time

@leo-bogastry leo-bogastry force-pushed the feature/ETCM-555-add-fastsync-metrics-on-blocks branch from 886947b to ba96706 Compare January 26, 2021 14:02
jvdp
jvdp reviewed Jan 26, 2021
View reviewed changes
Comment thread src/main/scala/io/iohk/ethereum/blockchain/sync/fast/FastSync.scala Outdated
@leo-bogastry leo-bogastry force-pushed the feature/ETCM-555-add-fastsync-metrics-on-blocks branch 2 times, most recently from 2e4e249 to d5c848e Compare January 27, 2021 13:52
@leo-bogastry leo-bogastry changed the title [ETCM-555] Add metrics on the download of headers, bodies and receipts during FastSync. Exposed other already existing metrics [ETCM-555] [ETCM-555] Add metrics on the time taken to download blocks (headers, bodies and receipts) and also MPT nodes. Expose in Grafana other already existing metrics. Jan 27, 2021
@leo-bogastry
Copy link
Copy Markdown
Contributor Author

leo-bogastry commented Jan 27, 2021 •
edited
Loading

I have added one more metric about the total time it takes to download MPT nodes, the value was already available and being printed in logs. I have also adjusted current metrics to be histograms instead (and fixed the naming)
Screenshots of added panels in Grafana:
Screenshot 2021-01-27 at 17 07 15
Screenshot 2021-01-27 at 14 47 58
Screenshot 2021-01-27 at 14 55 57

I'm sorry that the last panel is not showing data!

@leo-bogastry leo-bogastry changed the title [ETCM-555] [ETCM-555] Add metrics on the time taken to download blocks (headers, bodies and receipts) and also MPT nodes. Expose in Grafana other already existing metrics. [ETCM-555] Add metrics on the time taken to download blocks (headers, bodies and receipts) and also MPT nodes. Expose in Grafana other already existing metrics. Jan 27, 2021
@leo-bogastry leo-bogastry force-pushed the feature/ETCM-555-add-fastsync-metrics-on-blocks branch from d5c848e to a5237ff Compare January 27, 2021 14:44
@leo-bogastry leo-bogastry mentioned this pull request Jan 27, 2021
Merged
@leo-bogastry leo-bogastry force-pushed the feature/ETCM-555-add-fastsync-metrics-on-blocks branch 2 times, most recently from e240c39 to 1165721 Compare January 27, 2021 16:08
@leo-bogastry
Copy link
Copy Markdown
Contributor Author

leo-bogastry commented Jan 27, 2021

Some last adjustments done

@leo-bogastry leo-bogastry force-pushed the feature/ETCM-555-add-fastsync-metrics-on-blocks branch 2 times, most recently from f4a0620 to 0516e16 Compare January 28, 2021 12:49
@leo-bogastry
Copy link
Copy Markdown
Contributor Author

leo-bogastry commented Jan 28, 2021 •
edited
Loading

I have undone the modification of using System.nanoTime instead of System.currentTimeMillis(), given the discussion that took place in #907 (comment)

robinraju
robinraju approved these changes Jan 28, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@robinraju robinraju left a comment

Choose a reason for hiding this comment

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

🚀

@leo-bogastry leo-bogastry force-pushed the feature/ETCM-555-add-fastsync-metrics-on-blocks branch 2 times, most recently from a721eb1 to bf9de95 Compare January 28, 2021 13:56
@leo-bogastry
Copy link
Copy Markdown
Contributor Author

leo-bogastry commented Jan 28, 2021

Rebased with develop

AnastasiiaL
AnastasiiaL reviewed Jan 28, 2021
View reviewed changes
Comment thread src/main/scala/io/iohk/ethereum/blockchain/sync/fast/SyncMetrics.scala
object SyncMetrics extends MetricsContainer {

private[this] final val PivotBlockNumberGauge =
private final val PivotBlockNumberGauge =
Copy link
Copy Markdown
Contributor

@AnastasiiaL AnastasiiaL Jan 28, 2021

Choose a reason for hiding this comment

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

shall we keep private[this]? it imposes a bigger restriction

Copy link
Copy Markdown
Contributor Author

@leo-bogastry leo-bogastry Jan 28, 2021

Choose a reason for hiding this comment

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

As far as I understand private[this] is not more restrictive. No place outside this object can access that val already, or tries to

Copy link
Copy Markdown
Contributor

@AnastasiiaL AnastasiiaL Jan 28, 2021

Choose a reason for hiding this comment

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

[private] makes it available to the current instance and other instances of the class it’s declared in. Since we are already using a singleton - there are no other instances that could access it. So there's no difference in this case. Some consider it a good practice to default to the highest restriction, I guess this was the reason private[this] was used here on the first place. We could make a decision as a team on which guideline to follow

dzajkowski
dzajkowski reviewed Jan 29, 2021
View reviewed changes
Comment thread src/main/scala/io/iohk/ethereum/blockchain/sync/fast/FastSync.scala
updatePivotBlock(ImportedLastBlock)
case ValidationFailed(header, peerToBlackList) =>
log.warning(s"validation of header ${header.idTag} failed")
log.warning("validation of header {} failed", header.idTag)
Copy link
Copy Markdown
Contributor

@dzajkowski dzajkowski Jan 29, 2021

Choose a reason for hiding this comment

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

oh no.... we need to move to a proper logging wrapper. making ticket.

@leo-bogastry leo-bogastry force-pushed the feature/ETCM-555-add-fastsync-metrics-on-blocks branch from bf9de95 to a203ed3 Compare February 1, 2021 08:31
@leo-bogastry
Copy link
Copy Markdown
Contributor Author

leo-bogastry commented Feb 1, 2021

Rebased

ten15bit
ten15bit reviewed Feb 1, 2021
View reviewed changes
Comment thread docker/monitoring/grafana/provisioning/dashboards/mantis-dashboard.json Outdated
[ETCM-555] Add metrics on the time taken to download blocks (headers,…
f742fb4 
… bodies and receipts) and also MPT nodes. Expose in Grafana other already existing metrics.
@leo-bogastry leo-bogastry force-pushed the feature/ETCM-555-add-fastsync-metrics-on-blocks branch from a203ed3 to f742fb4 Compare February 1, 2021 09:22
ten15bit
ten15bit approved these changes Feb 1, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@ten15bit ten15bit left a comment

Choose a reason for hiding this comment

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

👍

@leo-bogastry leo-bogastry merged commit e581c7b into develop Feb 2, 2021
@leo-bogastry leo-bogastry deleted the feature/ETCM-555-add-fastsync-metrics-on-blocks branch February 2, 2021 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bsuieric bsuieric Awaiting requested review from bsuieric

5 more reviewers

@jvdp jvdp jvdp left review comments

@dzajkowski dzajkowski dzajkowski left review comments

@AnastasiiaL AnastasiiaL AnastasiiaL left review comments

@ten15bit ten15bit ten15bit approved these changes

@robinraju robinraju robinraju approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@leo-bogastry @jvdp @ten15bit @dzajkowski @robinraju @AnastasiiaL