Metric listeners #7474
Metric listeners #7474
Conversation
| @@ -43,4 +44,13 @@ private[zio] object MetricClient { | |||
| */ | |||
| final def snapshot()(implicit unsafe: Unsafe): Set[MetricPair.Untyped] = | |||
| metricRegistry.snapshot() | |||
|
|
|||
| trait Listener { | |||
There was a problem hiding this comment.
For performance reasons, I would create Listener.Unsafe, which is an unsafe version of this trait. Then Listener.Unsafe can extend Listener. This will allow us to create an even lower-level interface to listening to changes.
There was a problem hiding this comment.
Yeah, that makes sense. I changed the Listener methods to unsafe (returning Unit) for performance reasons, but I dropped the high-level interface as I don't think the use case for Datadog distribution would cover that.
| def updateHistogram(key: MetricKey[MetricKeyType.Histogram], value: Double): UIO[Unit] | ||
| def updateGauge(key: MetricKey[MetricKeyType.Gauge], value: Double): UIO[Unit] | ||
| def updateFrequency(key: MetricKey[MetricKeyType.Frequency], value: String): UIO[Unit] | ||
| def updateSummary(key: MetricKey[MetricKeyType.Summary], value: (Double, java.time.Instant)): UIO[Unit] |
There was a problem hiding this comment.
Might be better to avoid the tuple (Double, java.time.Instant) and instead pass two parameters.
| @@ -310,7 +325,7 @@ trait Metric[+Type, -In, +Out] extends ZIOAspect[Nothing, Any, Nothing, Any, Not | |||
| * provided amount. | |||
| */ | |||
| final def update(in: => In)(implicit trace: Trace): UIO[Unit] = | |||
| ZIO.succeed(unsafe.update(in, Set.empty)(Unsafe.unsafe)) | |||
| ZIO.succeed(unsafe.update(in, Set.empty)(Unsafe.unsafe)) *> notifyListeners(in) | |||
There was a problem hiding this comment.
This is much more expensive than the old version. I think what you should do is inside of ZIO.succeed, impurely perform the notification of listeners, so we can avoid the overhead.
Of course, this assumes all listeners are unsafe.
There was a problem hiding this comment.
Sure. What I ended up doing is that Unsafe.update handles the impure notification of listeners.
| @@ -43,4 +44,13 @@ private[zio] object MetricClient { | |||
| */ | |||
| final def snapshot()(implicit unsafe: Unsafe): Set[MetricPair.Untyped] = | |||
| metricRegistry.snapshot() | |||
|
|
|||
| trait Listener { | |||
There was a problem hiding this comment.
It's not recommended to put traits inside package objects.
| @@ -24,6 +24,10 @@ sealed trait MetricKeyType { | |||
| type Out | |||
| } | |||
| object MetricKeyType { | |||
| type Aux[T] = MetricKeyType { | |||
There was a problem hiding this comment.
| type Aux[T] = MetricKeyType { | |
| type WithIn[T] = MetricKeyType { |
|
@petoalbert Let me know when this is ready for another review, or ping me if you want to pair on it for a bit! |
| package zio.metrics | ||
|
|
||
| private[zio] trait MetricListener { | ||
| def updateHistogram(key: MetricKey[MetricKeyType.Histogram], value: Double): Unit |
There was a problem hiding this comment.
These should all take implicit unsafe: Unsafe.
| listeners.computeIfPresent(metricKeyType, { case (a, b) => b - listener }) | ||
| } | ||
|
|
||
| def update[T](key: MetricKey[MetricKeyType.WithIn[T]], value: T)(implicit trace: Trace): Unit = { |
There was a problem hiding this comment.
This is not actually updating, but rather, notifying listeners, so I would rename this to notifyListenerUpdate or something.
|
|
||
| def update[T](key: MetricKey[MetricKeyType.WithIn[T]], value: T)(implicit trace: Trace): Unit = { | ||
| val listenersForKey = listeners.get(key.keyType) | ||
| val iterator = listenersForKey.iterator |
There was a problem hiding this comment.
Here you are always allocating an Iterator, together with a Function0. At the very least, you should fast-path this by checking to see if listeners.nonEmpty before executing all of this (heavyweight) logic.
| iterator.next().updateCounter(key.asInstanceOf[MetricKey.Counter], value) | ||
| } | ||
| while (iterator.hasNext) { | ||
| updateNext() |
There was a problem hiding this comment.
Better to inline updateNext() so that you avoid allocating the Function0.
| ): Unit = | ||
| hook(extraTags).update(in) | ||
| ): Unit = { | ||
| val fullKey = key.tagged(extraTags).asInstanceOf[MetricKey[key.keyType.type]] |
There was a problem hiding this comment.
Computing the fullKey was not necessary before, and will involve additional allocation on every update of every metric.
Can you explain why this is necessary? At the very least, it should be fast-pathed, so that if extraTags.isEmpty, then no fullKey is computed.
|
This PR is ready for review now. |
Implementation for: #7393
Note that this needs obviously needs more work (at least tests and benchmarks), at the current stage I'd just like to get some feedback before proceeding.
I just learned that
zio.metricshad listeners which were using an unsafe API without zio effects. I used effects so that from thezio-metrics-connectorside it would be easier to use ZIO functionality, likeZStream. (see draft PR for zio-metrics-connectors: zio/zio-metrics-connectors#11) I am happy to recreate the originalMetricListenerwith theunsafeUpdatemethod if that is much better in terms of performance.