Adds withDynamicTags to metrics #6234
Conversation
| private def changeCounter(value: A)(implicit trace: ZTraceElement): UIO[Unit] = | ||
| ZIO.succeed { | ||
| val extraTags = f(value) | ||
| val allTags = self.tags ++ extraTags |
There was a problem hiding this comment.
it could also replace the tags instead of combining the static ones with the dynamic ones
| ZIO.succeed { | ||
| val extraTags = f(value) | ||
| val allTags = self.tags ++ extraTags | ||
| if (self.counter.metricKey.tags != allTags) { |
There was a problem hiding this comment.
this chunk comparison can be dropped but then each invocation allocates a new internal.metrics.Counter
There was a problem hiding this comment.
The happy path should be free of allocations, to the extent possible.
| * Converts this counter metric to one where the tags depend on the measured | ||
| * effect's result value | ||
| */ | ||
| def withDynamicTags(f: A => Chunk[MetricLabel]): ZIOMetric[A] = |
There was a problem hiding this comment.
| def withDynamicTags(f: A => Chunk[MetricLabel]): ZIOMetric[A] = | |
| def taggedWith(f: A => Chunk[MetricLabel]): ZIOMetric[A] = |
| @@ -278,15 +300,15 @@ object ZIOMetric { | |||
| * values over time. | |||
| */ | |||
| abstract class Gauge[A](final val name: String, final val tags: Chunk[MetricLabel]) extends ZIOMetric[A] { self => | |||
| private[this] val gauge = internal.metrics.Gauge(name, tags) | |||
| private[this] val gauge = ZFiberRef.unsafeMake(internal.metrics.Gauge(name, tags)) | |||
There was a problem hiding this comment.
The addition of a FiberRef wrapper here will impact performance. It may be better to make tags mutable if we can guard access sufficiently as to not violate referential transparency.
There was a problem hiding this comment.
One idea that would only make the construction non-threadsafe which I think is OK:
- keep the non-fiber ref internal metric fields as they were but change them to
var - add a fiber-ref version too which is also a
varand initialized tonull - the
taggedWithsets the direct reference tonulland initializes the fiber ref - using a helper function to get the gauge from either the fiber ref or the direct ref to work with the metric
I was thinking a lot on how else could I achieve this - to be able to use the existing (and arbitray) aspects with dynamic tags but could not come up with anything that is better or more performant so if you have a better idea please let me know
There was a problem hiding this comment.
(by the way I did not do what I described here first because I assumed that FiberRef itself does something like this - that it's cheap until its never used on multiple fibers)
|
I added the above described optimization |
| * measured effect's result value | ||
| */ | ||
| def taggedWith(f: A => Chunk[MetricLabel]): ZIOMetric[A] = { | ||
| if (self.setCountRef eq null) { |
There was a problem hiding this comment.
I think this may violate referential transparency, because it has a side-effect which is not described in the return type.
Will the behaviour of the original self differ in any way after this method is invoked?
There was a problem hiding this comment.
Thanks, this made me realize that there is such a problem: if c1 is a counter and c2 is the modified counter, applying c2 first modifies the FiberRef and does not restore it, so a next call to c1 on the same fiber will have wrong tags.
taggedWith should call self.copy
There was a problem hiding this comment.
Actually just checked and copy does not work correctly currently either (without my change):
val count = ZIOMetric
.count("test", MetricLabel("x", "1"))
.copy("test2", Chunk(MetricLabel("x", "2")))
val run =
for {
_ <- ZIO.succeed("hello") @@ count
states <- UIO(MetricClient.unsafeStates)
_ <- ZIO.debug(states.toList.mkString("\n"))
} yield ()prints
(Counter(test2,Chunk(MetricLabel(x,2))),MetricState(test2{x->2}, Counter(0.0)))
(Counter(test,Chunk(MetricLabel(x,1))),MetricState(test{x->1}, Counter(1.0)))
(because the copied aspect just calls the original self's apply which refers to the original metric).
So in addition to fix the referential transparency issue, should also:
- fix
copy - write some metric tests :)
| self.apply(zio.tap(changeSetCount)) | ||
|
|
||
| private def changeSetCount(value: A)(implicit trace: ZTraceElement): UIO[Unit] = | ||
| self.setCountRef.update { setCount => |
There was a problem hiding this comment.
You can improve performance by factoring this lambda out into a constant lambda inside the class. That way, it won't be created each time changeSetCount is invoked.
While migrating
zio-awsto the new metrics API I ran into the problem of defining a metric aspect in which the tags depend on the result value of the wrapped effect. In this particular example, each AWS call returns aDescribed[A]value where theDescribedwrapper contains metadata about the call such as AWS service and operation name, which could be used as tags of the metric.I tried to find a way in which required changes to the new the metric API are minimal while all the predefined and user defined metric aspects can be somehow used with this more dynamic tagging approach.
The result is a
def withDynamicTags(f: A => Chunk[MetricLabel]): ZIOMetric[A]on each metric type which converts them to a metric which no longer exposes any metric-type specific API (as the tags are unknown) but are still valid metric aspects.