[ETCM-573] Add metrics on block imports #919

leo-bogastry · 2021-02-08T10:15:34Z

Description

Add metrics on block imports about importing new blocks, mined blocks, checkpoint blocks and other non specified blocks

Proposed Solution

The import of these block is always done the same way (on BlockImporter) but the call to make the import comes from different origins.
I have added a trait called BlockImportType and specified the four different kinds of block imports and added this information to the methods making the block import calls, and then used that information to update the appropriate metric.

Testing

I have updated the Grafana dashboard with the new metric

jvdp

LGTM but I always get an uneasy feeling when looking at actor code...

jvdp · 2021-02-11T11:22:56Z

src/it/scala/io/iohk/ethereum/sync/RegularSyncItSpec.scala

    }
  }

+  "A metric about mining a new block should be available" in customTestCaseResourceM(


Very nice that you tested the metric 👍 (Though this was perhaps also out of necessity?)

Yes, it was. I need to understand better what was happening. I started by modifying an existing test and then created this one that is a bit more specific. I also found a bit weird that there are no tests on metrics at all

jvdp · 2021-02-11T11:27:16Z

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockImporter.scala

  case class ResolvingMissingNode(blocksToRetry: NonEmptyList[Block]) extends NewBehavior
  case class ResolvingBranch(from: BigInt) extends NewBehavior

+  sealed trait BlockImportType {


Do you think this explicit distinction between the types of block imports can be used to refactor other pieces of code? Perhaps stuff that is now implicit in all the message passing? (Could be a nice follow-up ticket if so.)

I have not seen enough code yet to be able to say yes or no. I do think the code in general is not very easy to reason about and I hope we can improve it in a nearby future

jvdp · 2021-02-11T11:30:01Z

src/test/scala/io/iohk/ethereum/network/PeerManagerSpec.scala

+      new java.net.URI(
+        "enode://a59e33ccd2b3e52d578f1fbd70c6f9babda2650f0760d6ff3b37742fdcdfdb3defba5d56d315b40c46b70198c7621e63ffa3f987389c7118634b0fefbbdfa7fd@51.158.191.43:38556?discport=38556"
+      )
+    )


Random file getting formatted :P

I'm curious: are you using the IntelliJ formatter or scalafmt, or both? Because CI should already confirm that every source file is formatted, and yet your PRs contain a bunch of these changes.

I use both I think. I think that maybe different people have different settings? There should not be all this reformats indeeed

recommendation: always run sbt pp before pushing, that way you will get the code formatted.
also: not sure if CI is verifying test code

jvdp · 2021-02-11T11:30:25Z

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/RegularSyncMetrics.scala

+  final val NewBlockPropagationTimer = metrics.timer(blockPropagationTimer, "class", "NewBlockPropagation")
+  final val DefaultBlockPropagationTimer = metrics.timer(blockPropagationTimer, "class", "DefaultBlockPropagation")
+
+  def recordMinedBlockPropagationTimer(time: Long): Unit = MinedBlockPropagationTimer.record(time, NANOSECONDS)


Could call the argument something like nanos to be more explicit about the unit.

pullrequest

✅ This pull request was sent to the PullRequest network.

@LeonorLunatech you can click here to see the review status or cancel the code review job - or - cancel by adding [!pr] to the title of the pull request.

pullrequest

	1 Message
⚠️	Due to its size, this pull request will likely have a little longer turnaround time and will probably require multiple passes from our reviewers.

pullrequest

This looks good overall—I don't see any blocking issues. I did point out a couple of opportunities for adding a bit of type safety. IME using wrapper classes is a lightweight way (both in terms of code and at runtime) to get better documentation and eliminate some silly mix-ups.

Eric E

Was this helpful? Yes | No

Reviewers will be notified any time you reply to their comments or commit new changes.
You can also request a full follow-up review from your PullRequest dashboard.

pullrequest · 2021-02-11T22:21:35Z

src/it/scala/io/iohk/ethereum/sync/RegularSyncItSpec.scala

+
+    for {
+      _ <- peer1.startRegularSync()
+      _ <- peer1.mineNewBlocks(10.milliseconds, 1)(IdentityUpdate)


Note that it may be nice to create a value class for number of blocks:

case class NumBlocks(val underlying: Int) extends AnyVal

This would help make invocations like this a little clearer (at present, the only way to figure out way this 1 signifies is to track down the definition of mineNewBlocks), adds a bit of documentation and helps avoid mixups between different kinds of Ints.

Of course there is a tiny bit of extra code for this and whether it's worth paying that cost depends on how central the concept is in the codebase. Just using a regular Int probably isn't a big deal in tests like this.

(Same thinking applies to block number in waitForRegularSyncLoadLastBlock.)

Eric E

pullrequest · 2021-02-11T22:21:35Z

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockImporter.scala

+  }
+
+  case object CheckpointBlockImport extends BlockImportType {
+    override def recordMetric(time: Long): Unit = RegularSyncMetrics.recordImportCheckpointPropagationTimer(time)


Note that there's another opportunity for a value class here:

case class Time(val underlying: Long) extends AnyVal

Eric E

dzajkowski · 2021-02-12T10:25:16Z

src/it/scala/io/iohk/ethereum/sync/RegularSyncItSpec.scala

+    _: String,
+    Array("class"),
+    Array("DefaultBlockPropagation")
+  )


maybe we could start adding helpers that make it easier to read? eg.

MantisRegistries.sampleClass("DefaultBlockPropagation") object MantisRegistries { def sampleClass(className: String) = CollectorRegistry.defaultRegistry.getSampleValue( _: String, Array("class"), Array(className) ) }

I have added the helper, the code is more clean indeed. I renamed the label name from class to blocktype

dzajkowski · 2021-02-12T10:29:09Z

src/it/scala/io/iohk/ethereum/sync/RegularSyncItSpec.scala

+  "A metric about mining a new block should be available" in customTestCaseResourceM(
+    FakePeer.start2FakePeersRes()
+  ) { case (peer1, peer2) =>
+    val minedMetricBefore = getMinedBlockValue("app_regularsync_blocks_propagation_timer_seconds_count")


nitpick: the strings (eg. "app_regularsync_blocks_propagation_timer_seconds_count") are repeated - maybe a Constants helper object would make sense?

You are right, I created vals for all the strings, they all repeated

dzajkowski · 2021-02-12T10:30:21Z

src/it/scala/io/iohk/ethereum/sync/util/RegularSyncItSpecUtils.scala

        .getChainWeightByHash(block.hash)
        .getOrElse(throw new RuntimeException(s"ChainWeight by hash: ${block.hash} doesn't exist"))
-      val currentWolrd = getMptForBlock(block)
+      val currentWorld = getMptForBlock(block)


dzajkowski · 2021-02-12T11:07:47Z

src/test/scala/io/iohk/ethereum/network/PeerManagerSpec.scala

+      new java.net.URI(
+        "enode://a59e33ccd2b3e52d578f1fbd70c6f9babda2650f0760d6ff3b37742fdcdfdb3defba5d56d315b40c46b70198c7621e63ffa3f987389c7118634b0fefbbdfa7fd@51.158.191.43:38556?discport=38556"
+      )
+    )


recommendation: always run sbt pp before pushing, that way you will get the code formatted.
also: not sure if CI is verifying test code

ten15bit

👍

ten15bit · 2021-02-12T11:35:23Z

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockImporter.scala

+  }
+
+  case object DefaultBlockImport extends BlockImportType {
+    override def recordMetric(time: Long): Unit = RegularSyncMetrics.recordDefaultBlockPropagationTimer(time)


nanos would also be better here (and two more just above)

pullrequest

This looks good overall. There aren't any blocking issues that I can see outside of what's already been mentioned. Great job and welcome to PullRequest!

Livingstone W

Was this helpful? Yes | No

Reviewers will be notified any time you reply to their comments or commit new changes.
You can also request a full follow-up review from your PullRequest dashboard.

leo-bogastry requested review from AnastasiiaL, bsuieric, dzajkowski, jvdp and ten15bit February 8, 2021 10:34

leo-bogastry force-pushed the feature/ETCM-573-measure-block-propagation branch 2 times, most recently from 5dfd392 to 8e55ae8 Compare February 8, 2021 18:02

jvdp approved these changes Feb 11, 2021

View reviewed changes

AlanVerbner requested a review from PR-IOHK February 11, 2021 13:00

leo-bogastry force-pushed the feature/ETCM-573-measure-block-propagation branch from 8e55ae8 to bb70c25 Compare February 11, 2021 13:42

pullrequest bot reviewed Feb 11, 2021

View reviewed changes

pullrequest bot approved these changes Feb 11, 2021

View reviewed changes

leo-bogastry force-pushed the feature/ETCM-573-measure-block-propagation branch from bb70c25 to 504cc2b Compare February 12, 2021 08:15

dzajkowski reviewed Feb 12, 2021

View reviewed changes

leo-bogastry force-pushed the feature/ETCM-573-measure-block-propagation branch from 504cc2b to 794b52a Compare February 12, 2021 11:00

dzajkowski approved these changes Feb 12, 2021

View reviewed changes

ten15bit approved these changes Feb 12, 2021

View reviewed changes

leo-bogastry force-pushed the feature/ETCM-573-measure-block-propagation branch from 794b52a to b56e189 Compare February 12, 2021 13:31

pullrequest bot reviewed Feb 12, 2021

View reviewed changes

leo-bogastry force-pushed the feature/ETCM-573-measure-block-propagation branch from b56e189 to cadbc07 Compare February 16, 2021 15:58

[ETCM-573] Add metrics on block imports

bd60f20

leo-bogastry force-pushed the feature/ETCM-573-measure-block-propagation branch from cadbc07 to bd60f20 Compare February 17, 2021 07:13

leo-bogastry merged commit 86053a0 into develop Feb 17, 2021

leo-bogastry deleted the feature/ETCM-573-measure-block-propagation branch February 17, 2021 09:16

[ETCM-573] Add metrics on block imports #919

[ETCM-573] Add metrics on block imports #919

Uh oh!

Conversation

leo-bogastry commented Feb 8, 2021

Description

Proposed Solution

Testing

Uh oh!

jvdp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pullrequest bot left a comment

Choose a reason for hiding this comment

Uh oh!

pullrequest bot left a comment

Choose a reason for hiding this comment

Uh oh!

pullrequest bot left a comment

Choose a reason for hiding this comment

Uh oh!

pullrequest bot Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

pullrequest bot Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ten15bit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pullrequest bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants