feat: instrumenting ShardHome latency#32885
Conversation
johanandren
left a comment
johanandren
left a comment
There was a problem hiding this comment.
Looks ok, but potentially align the method names to say exactly what they are callbacks for instead of fixing the Javadoc from my feedback.
| def dependencies: immutable.Seq[String] | ||
|
|
||
| /** | ||
| * Triggered when the shard coordinator requests the home of a shard |
There was a problem hiding this comment.
It is called in the ShardRegion though, so "when a region requests the home of a shard"?
| def requestGetShardHome(): Unit | ||
|
|
||
| /** | ||
| * Triggered when the shard coordinator responds with the home of a shard |
There was a problem hiding this comment.
Not when it response but when the region receives the response (so potentially cluster roundtrip network + the time for the coordinator to reply)?
patriknw
left a comment
patriknw
left a comment
There was a problem hiding this comment.
looking good, with a few comments
|
|
||
| override def dependencies: immutable.Seq[String] = Nil | ||
|
|
||
| override def requestGetShardHome(): Unit = () |
There was a problem hiding this comment.
might be good to include typeName and maybe even shard id as parameters?
| } | ||
| } | ||
|
|
||
| private def requestShardHome(coordinator: Option[ActorRef], shard: ShardId): Unit = { |
There was a problem hiding this comment.
isn't coordinator already an instance variable in ShardRegion, so no need to pass it as parameter here?
| requestDurations.nonEmpty shouldBe true | ||
| requestDurations.foreach { duration => | ||
| duration > 0 shouldBe true | ||
| duration < 20 shouldBe true |
There was a problem hiding this comment.
this will be flaky in CI, it may take longer than 20 milliseconds, and you should measure such durations with System.nanoTime
it's not even guaranteed that it will be > 0
I don't think you need to use clock time at all in the test, but just set it to 1 in requestGetShardHome and 2 in responseShardHome. Just some deterministic values that can be asserted to verify that the methods have been called.
sebastian-alfers
force-pushed
the
sharding-telemetry
branch
from
f44207d to
0fbb370
Compare
sebastian-alfers
force-pushed
the
shard-home-latency
branch
from
d0165c4 to
301b764
Compare
...-sharding/src/multi-jvm/scala/akka/cluster/sharding/ClusterShardingInstrumentationSpec.scala
Outdated
Show resolved
Hide resolved
...-sharding/src/main/scala/akka/cluster/sharding/internal/ClusterShardingInstrumentation.scala
Outdated
Show resolved
Hide resolved
3 participants
No description provided.