Adding a Description field to ZIO MetricKey #8159

Grryum · 2023-05-30T19:09:01Z

The MetricKey describes metric with its name and tags.
With this MR, the MetricKey is extended to include a "description" field, allowing developers to provide meaningful descriptions for each metric.

Problem

While working on new metric connector (zio/zio-metrics-connectors#42) I was faced with the need to extract the description of the metric.
Similar issue (zio/zio-metrics-connectors#23) was solved via throwing description through metrics tags set (https://github.com/zio/zio-metrics-connectors/pull/32/files#diff-fcbe5916f5b460e7cca68e385fbb00d736fcb533e5b2867ee09a7a3e38be17b9R35). IMO this is not very clean approach because tags is not a storage for metadata at all.
We need to have a separate typed field (optional) in MetricKey to be able to operate with metric description without any non clear workarounds.

Additional motivation

enhances the self-documentation aspect of ZIO metric development. By adding descriptive metadata to metrics, developers can clearly communicate the purpose, usage, and context of individual metrics.
aligns ZIO more closely with vendor-specific metrics systems like Prometheus. These systems often rely on metric metadata to generate insightful visualizations, dashboards, and analytics. With the "description" field supported by MetricKey, ZIO facilitates seamless integration with Prometheus and other similar vendors, ensuring a smooth flow of metric data for monitoring and analysis

If MR is accepted I would be able to fix description extraction in zio-metrics-connector module.

CLAassistant · 2023-05-30T19:09:12Z

All committers have signed the CLA.

adamgfraser · 2023-05-30T22:29:35Z

This change is not binary compatible.

vladimirkl · 2023-05-31T15:37:02Z

@Grryum There was already discussion - this is why description implemented currently as one of tags
#7781

Grryum · 2023-05-31T15:59:14Z

@vladimirkl thank you for mentioning you issue!

@adamgfraser do I have a chance to bring this MR to its logical end or you are categorically against adding a description to the metric? The solution with passing description via tags is really looks like workaround... Now we are very close to the long-awaited release of Micrometer integration (zio/zio-metrics-connectors#42). And if we stay with the solution when description is passed through tags, then we will be forced to make the Micrometer layer depend on the MetricsConfig (like here https://github.com/zio/zio-metrics-connectors/blob/zio/series2.x/core/src/main/scala/zio/metrics/connectors/prometheus/package.scala#L10) in which the description tag key will be located. It seems to be very strange for user without context of our discussion.

adamgfraser · 2023-05-31T22:08:44Z

@Grryum I'm not opposed to adding the description but it needs to be done in a binary compatible way. What I proposed before plus using overloads instead of variable arguments should do the trick:

sealed case class MetricKey[+Type] private (
  name: String,
  keyType: Type,
  tags: Set[MetricLabel] = Set.empty
) { self =>

  def description: Option[String] =
    None

  def copy[Type2](name: String = name, keyType: Type2 = keyType, tags: Set[MetricLabel] = tags): MetricKey[Type2] =
    new MetricKey(name, keyType, tags) {
      override def description: Option[String] =
        self.description
    }
}

We also need to think about whether the description should be part of the identity of the key.

Grryum · 2023-05-31T23:33:26Z

@adamgfraser thank you very much! I will rewrite

Grryum · 2023-06-05T10:20:51Z

@adamgfraser I rewrote the MR. Now it adds description in binary compatible way. The main focus of this update is to redefine the semantic behavior of the "description" field, making it "unmodifiable".
Developers can set an initial description for a Metric instance using the withDescription method. Once set, the description becomes an immutable attribute of the Metric and cannot be altered or overridden in subsequent calls to withDescription. This behavior ensures that the descriptions associated with metrics remain static and reliable, similar to how Micrometer (I checked it for myself) and other metric frameworks handles descriptions.

Please have a look

adamgfraser · 2023-06-05T13:30:59Z

@Grryum withDescription is not a great API because it makes the fact that the description is immutable a matter of convention instead of correct by construction.

Instead we should make it part of the constructor of a metric as you had previously done, using overloading instead of default arguments to avoid binary compatibility issues:

object Metric {
  def gauge(name: String, description: String): Gauge[Double] =
    fromMetricKey(MetricKey.gauge(name))
}

Grryum · 2023-06-05T14:58:24Z

@Grryum withDescription is not a great API because it makes the fact that the description is immutable a matter of convention instead of correct by construction.

Instead we should make it part of the constructor of a metric as you had previously done, using overloading instead of default arguments to avoid binary compatibility issues:
object Metric {
  def gauge(name: String, description: String): Gauge[Double] =
    fromMetricKey(MetricKey.gauge(name))
}

@adamgfraser sorry, seems that I misunderstood you.
I reversed the approach. To avoid issues with binary compatibility I had to remove one version of method timer because adding new argument description: Option[String] = None with default value to both overloaded versions of method timer:

def timer(name: String, chronoUnit: ChronoUnit): Metric[MetricKeyType.Histogram, Duration, MetricState.Histogram]
def timer(name: String, chronoUnit: ChronoUnit, boundaries: Chunk[Double]): Metric[MetricKeyType.Histogram, Duration, MetricState.Histogram]

leaded to a compile error in object Metric, multiple overloaded alternatives of method timer define default arguments..
Hope it is ok now.

Both version of timer are supports now.

adamgfraser · 2023-06-05T15:01:40Z

@Grryum This is not binary compatible. You need to use overloading, not default arguments.

Do this:

object Metric {

  def gauge(name: String): Gauge[Double] =
    ???

  def gauge(name: String, description: String): Gauge[Double] =
    ???
}

Not this:

object Metric {

  def gauge(name: String, description: Option[String] = None): Gauge[Double] =
    ???
}

Grryum · 2023-06-05T15:59:12Z

@adamgfraser finally got it :)
I am also reduced repeatable code in timer methods in separate commit e9fedd5
Could you have a look please

adamgfraser · 2023-06-05T20:02:32Z

core/shared/src/main/scala/zio/metrics/MetricKey.scala

+  /**
+   * Returns a new `MetricKey` with the specified description.
+   */
+  def described(description0: String): MetricKey[Type] =


We should delete this and instead provide variants of the constructors allowing specifying a description just like we did for the metrics themselves since there should be no way to update the description once a metric is created.

@adamgfraser sure, done!

…overloaded alternatives.

adamgfraser · 2023-06-05T23:40:47Z

@Grryum What is your thought regarding whether the description should be part of the identity of the key?

Grryum · 2023-06-06T11:53:00Z

@Grryum What is your thought regarding whether the description should be part of the identity of the key?

@adamgfraser In my opinion, I believe that adding a description to the identity of Metric key is not advisable. This is because metrics with different descriptions would be treated as separate metrics with separate counters. In my view, the semantic of the description field should be considered as some kind of label that can be optionally set. Furthermore, incorporating a description into the metric key does not align with the integration concepts of Prometheus, Micrometer, and potentially other systems.

According to Micrometer's documentation (https://micrometer.io/docs/concepts#_meters):
A meter is uniquely identified by its name and dimensions. We use the terms, “dimensions” and “tags,” interchangeably, and the Micrometer interface is Tag simply because it is shorter. As a general rule, it should be possible to use the name as a pivot. Dimensions let a particular named metric be sliced to drill down and reason about the data. This means that, if only the name is selected, you can drill down by using other dimensions and reason about the value being shown.

Similarly, the Prometheus documentation (https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels) highlights that each time series is uniquely identified by its metric name and optional tags:
Prometheus fundamentally stores all data as [time series](https://en.wikipedia.org/wiki/Time_series): streams of timestamped values belonging to the same metric and the same set of labeled dimensions. .... Every time series is uniquely identified by its metric name and optional key-value pairs called labels.

Additionally, as I mentioned before I had verified that Micrometer follows the approach of not allowing updates to the metric description once it is initially set. This aligns with the philosophy of maintaining metrics identity.

Thank you for considering this perspective, and I am open to further discussions or clarification on this matter.

adamgfraser · 2023-06-06T16:21:55Z

@Grryum I think everything you said makes sense but I'm not sure it addresses the issue.

If you look at MetricKey now it is a case class so its identity is based on all its fields, which include the name, the key type, and the tags. So for example:

import zio.metrics._

object Example extends App {

  val metric1 = MetricKey.counter("metric1")
  val metric2 = MetricKey.counter("metric1").tagged("key1", "value1")

  println(metric1 == metric2) // false
}

This obviously works okay with all of the metric backends we have implemented.

Under the current implementation on the other hand we would have the following:

import zio.metrics._

object Example extends App {

  val metric1 = MetricKey.counter("metric1")
  val metric2 = MetricKey.counter("metric1", "description1")

  println(metric1 == metric2) // true
}

This arguably isn't crazy, perhaps you say the description is an "implementation detail", but it is at the very least a little weird. These two metric keys have obviously different properties (calling description on one will return None and on the other will return Some("description1"). Having definitions of equality like this can also create issues when metrics are used in data structures such as hash maps.

Note also that in your original pull request the description would have been part of the identity of the key since it was a field of a case class. And I think that was indeed the correct way to do that absent these binary compatibility issues.

So I tend to think we should try to mirror as much as possible what we would have done if we were implementing this today and didn't have to work around the binary compatibility issues, which would mean overriding equals and hashcode so they behave as if description was a field of the case class.

Grryum · 2023-06-06T21:51:56Z

@adamgfraser Thank you for your detailed response.

If we consider implementing a metric key with a description from scratch, I think there are globally three possible options:

Option 1: Implement a case class MetricKey with all necessary fields (name, type, tags, description) and explicitly overload equals and hashCode methods. This approach ensures that the MetricKey can be used as a key for hash maps and other hash-related data structures without the description field affecting equality comparisons. An example of this approach can be seen in Micrometer's Meter.Id implementation https://github.com/micrometer-metrics/micrometer/blob/main/micrometer-core/src/main/java/io/micrometer/core/instrument/Meter.java#L352-L366.

Option 2: Implement a case class MetricKey with fields (name, type, tags) that uniquely identify the metric and do some trick (thank you for idea) with description inside this class. This is the approach taken in the current merge request. By keeping the equals and hashCode methods not modified (autogenerated from case class) we focus on identifying the metric while allowing for a description property and achieve the desired functionality.

Option 3: Implement a case class MetricKey with fields (name, type, tags) that solely identify the metric and introduce a new structure to handle the description. This approach involves creating a new structure, such as DescribedMetricKey, which combines the MetricKey with the description field.
final case class DescribedMetricKey[+Type](metricKey: MetricKey[Type], description: String)

We would also need to modify the MetricListener methods to accept an optional description parameter.

private[zio] trait MetricListener {
  def modifyGauge(key: MetricKey[MetricKeyType.Gauge], description: Option[String], value: Double)(implicit unsafe: Unsafe): Unit
  ...  
}

This way, the description can be propagated through the listener callbacks at internal method notifyListeners

  private[zio] def notifyListeners[T](
    key: MetricKey[MetricKeyType.WithIn[T]],
    description: Option[String],
    value: T,
    eventType: MetricEventType
  ) = {
    ...
    listeners(i).modifyGauge(key.asInstanceOf[MetricKey.Gauge], description, value.asInstanceOf[Double])
    ...
  }

Additionally, a public constructor for Metric can be created, taking a DescribedMetricKey as a parameter and utilizing the description via notifyListeners.

  def fromMetricKey[Type <: MetricKeyType](
    describedKey: DescribedMetricKey[Type]
  ): Metric[Type, describedKey.key.keyType.In, describedKey.key.keyType.Out] =
    new Metric[Type, describedKey.key.keyType.In, describedKey.key.keyType.Out] {
      val keyType = describedKey.key.keyType

      override private[zio] val unsafe: UnsafeAPI =
        new UnsafeAPI {
          def update(in: describedKey.key.keyType.In, extraTags: Set[MetricLabel] = Set.empty)(implicit
            unsafe: Unsafe
          ): Unit = {
            val fullKey = describedKey.key.tagged(extraTags).asInstanceOf[MetricKey[describedKey.key.keyType.type]]
            hook(fullKey).update(in)
            metricRegistry.notifyListeners(fullKey, Some(describedKey.description), in, MetricEventType.Update)
          }

          def value(extraTags: Set[MetricLabel] = Set.empty)(implicit unsafe: Unsafe): describedKey.key.keyType.Out = {
            val fullKey = describedKey.key.tagged(extraTags).asInstanceOf[MetricKey[describedKey.key.keyType.type]]
            hook(fullKey).get()
          }

          def modify(in: describedKey.key.keyType.In, extraTags: Set[MetricLabel] = Set.empty)(implicit
            unsafe: Unsafe
          ): Unit = {
            val fullKey = describedKey.key.tagged(extraTags).asInstanceOf[MetricKey[describedKey.key.keyType.type]]
            hook(fullKey).modify(in)
            metricRegistry.notifyListeners(fullKey, Some(describedKey.description), in, MetricEventType.Modify)
          }

        }

      def hook(fullKey: MetricKey[describedKey.key.keyType.type]): MetricHook[describedKey.key.keyType.In, describedKey.key.keyType.Out] =
        metricRegistry.get(fullKey)(Unsafe.unsafe)
    }

This approach maintains transparency for the user and avoids binary compatibility issues (only internal modifications).
Of course the implementation details of last option may vary.

First option is not possible for us because of binary compatibility issues. Second we discussed above.
I would appreciate your thoughts and feedback on alternative approach.

adamgfraser · 2023-06-06T21:55:13Z

I would just override equals and hashCode in MetricKey.

Grryum · 2023-06-07T13:40:57Z

I would just override equals and hashCode in MetricKey.

@adamgfraser but if we override equals and hashCode in MetricKey our metrics starts to be unique by the description. For example such test would fail:

    test("Test metrics with description") {
      val name = "counterName"

      val counter1 = Metric.counter(name)

      val counter2 = Metric.counter(name, "description")

      for {
        _  <- counter1.update(1L)
        _  <- counter2.update(1L)
        r1 <- counter1.value
        r2 <- counter2.value
      } yield assertTrue(
        r1 == MetricState.Counter(2d),
        r2 == MetricState.Counter(2d)
      )
    }

would it be ok for users?

adamgfraser · 2023-06-07T13:53:38Z

@Grryum We already have:

test("Test metrics with tags") {
  val name = "counterName"
  val key = "key"
  val value = "value"

  val counter1 = Metric.counter(name)

  val counter2 = Metric.counter(name).tagged(key, value)

  for {
    _  <- counter1.update(1L)
    _  <- counter2.update(1L)
    r1 <- counter1.value
    r2 <- counter2.value
  } yield assertTrue(
    r1 == MetricState.Counter(1d),
    r2 == MetricState.Counter(1d)
  )
}

So I think your test should fail.

…y the description.

Grryum · 2023-06-07T16:53:48Z

@adamgfraser I overridden equals and hashCode methods in MetricKey and added corresponding test.
Please take a look.

adamgfraser

Looks great! Shall we override toString as well so that the MetricKey shows the description as if it was a field of the case class. Then I think this will be ready to go!

Grryum · 2023-06-07T17:32:22Z

Looks great! Shall we override toString as well so that the MetricKey shows the description as if it was a field of the case class. Then I think this will be ready to go!

@adamgfraser sure! I have pushed a new commit

adamgfraser

Thank you for your work on this! 🙏

Grryum · 2023-06-07T20:39:58Z

@adamgfraser glad to work together to make the world a little better :)
It seems like a small change, but a great step towards the future ZIO Metric integrations!
Thank you!

Grryum force-pushed the zio-metric-description branch from 0f618c5 to 9f5118b Compare May 30, 2023 20:20

Grryum mentioned this pull request May 30, 2023

Integration with Micrometer for greater ZIO monitoring capabilities zio/zio-metrics-connectors#42

Merged

Grryum force-pushed the zio-metric-description branch from 9f5118b to 1304c93 Compare June 5, 2023 10:03

Grryum force-pushed the zio-metric-description branch from 1304c93 to b857b94 Compare June 5, 2023 14:50

Grryum force-pushed the zio-metric-description branch from b857b94 to 4935718 Compare June 5, 2023 15:53

Grryum added 2 commits June 5, 2023 18:55

Added description for ZIO Metric and tests.

2bc3662

Cut down on repetitive code.

e9fedd5

Grryum force-pushed the zio-metric-description branch from 4935718 to e9fedd5 Compare June 5, 2023 15:55

adamgfraser reviewed Jun 5, 2023

View reviewed changes

Removed descripted method from MetricKey, key constructors now are …

e660097

…overloaded alternatives.

Overridden equals and hashCode for MetricKey to make it unique also b…

99b96b6

…y the description.

Grryum force-pushed the zio-metric-description branch from aedd49f to 99b96b6 Compare June 7, 2023 16:06

adamgfraser suggested changes Jun 7, 2023

View reviewed changes

Also overridden the MetricKey#toString method.

341e242

Grryum requested a review from adamgfraser June 7, 2023 17:24

adamgfraser approved these changes Jun 7, 2023

View reviewed changes

adamgfraser merged commit eb3a921 into zio:series/2.x Jun 7, 2023

Grryum mentioned this pull request Sep 2, 2023

Updated zio version with metric key description and added its support to Prometheus and Micrometer metric connectors. zio/zio-metrics-connectors#54

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Adding a Description field to ZIO MetricKey #8159

Adding a Description field to ZIO MetricKey #8159

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Adding a Description field to ZIO MetricKey #8159

Adding a Description field to ZIO MetricKey #8159

Uh oh!

Conversation

Problem

Additional motivation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!