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

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

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

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

Fixed

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

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

case class Time(val underlying: Long) extends AnyVal

Eric E

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

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

🎉

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

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

Fixed!