Make `Meter` API generic

[iRevive]

Reference	Link
Specification	https://opentelemetry.io/docs/specs/otel/metrics/api/#counter-creation

The motivation

There are missing builders for the following instruments: Counter[F, Double], UpDownCounter[F, Double] , Histogram[F, Long], Gauge[F, Long].

Instead of extending Meter with doubleCounter and longCounter, the API can be generic.

The proposal

While it's not mentioned explicitly in the spec, according to the proto models, the measurement can be either Long or Double.

Since the set of allowed types is finite and known, we can improve the ergonomics of the Meter API:

trait Meter[F[_]] {
- def counter(name: String): Counter.Builder[F, Long]
+ def counter[A: MeasurementValue](name: String): Counter.Builder[F, A]

And the MeasurementValue can be defined as:

sealed trait MeasurementValue[A]
object MeasurementValue {
  implicit object LongMeasurementValue extends MeasurementValue[Long]
  implicit object DoubleMeasurementValue extends MeasurementValue[Double]
}

Before:

val meter: Meter[IO] = ???
meter.counter("long-counter").create 
// double counter cannot be created
meter.histogram("double-counter").create 
// long histogram cannot be created

After:

val meter: Meter[IO] = ???
meter.counter[Long]("long-counter").create
meter.counter[Double]("double-counter").create
meter.histogram[Long]("long-histogram").create
meter.histogram[Double]("double-histogram").create

Benefits

1) The API feels more intuitive (well, it's subjective)

OpenTelemetry Java SDK:

val meter: Meter = ???
val longCounter: LongCounter = meter.counterBuilder("long-counter").build
val doubleCounter: DoubleCounter = meter.counterBuilder("long-counter").ofDoubles().build

Otel4s:

val meter: Meter[IO] = ???
val longCounter: IO[Counter[IO, Long]] = meter.counter[Long]("long-counter").create
val doubleCounter: IO[Counter[IO, Double]] = meter.counter[Double]("double-counter").create

Drawbacks

1) The instrument type must be provided explicitly

Calling an instrument builder without an explicit param type will lead to the 'ambiguous implicits' error. The error itself can be frustrating for newcomers. However, it can be somewhat mitigated with the @implicitAmbiguous annotation.

For example, the following error message will be raised when the type param is missing:

val meter: Meter[IO] = ???
meter.counter("test").create

[info] compiling 1 Scala source to ./.../otel4s/examples/target/scala-2.13/classes ...
[error] /..../otel4s/examples/src/main/scala/Example.scala:132:16: 
[error] 
[error] Choose the type of an instrument explicitly, for example:
[error] 1) `.counter[Long](...)` or `.counter[Double](...)`
[error] 2) `.upDownCounter[Long](...)` or `.upDownCounter[Double](...)`
[error] 3) `.histogram[Long](...)` or `.histogram[Double](...)`
[error] 4) `.gauge[Long](...)` or `.gauge[Double](...)`
[error]     
[error]   meter.counter("test").create
[error]                ^

2) That's a lot of breaking changes

Even though we are still in the 'active development' zone, the changes may frustrate users. This issue can be partially mitigated by the scalafix migration rules (e.g. https://github.com/typelevel/otel4s/pull/426).

The questions

1) What do you think about these API changes?

[iRevive]

@NthPortal @bpholt @mpilquist @rossabaker @arosien @milessabin @christiankjaer @armanbilge

Sorry for pinging you, but If I'm not mistaken, you are using otel4s at $work on a daily basis. It would be interesting to hear your opinion.

[iRevive]

Currently, I ended up with (at least) three possible solutions:

1) Generic API

trait Meter[F[_]] {
  def counter[A: MeasurementValue](name: String): Counter.Builder[F, A]
  def histogram[A: MeasurementValue](name: String): Histogram.Builder[F, A]
  ...
}

2) Separate API

trait Meter[F[_]] {
  def longCounter(name: String): Counter.Builder[F, Long]
  def doubleCounter(name: String): Counter.Builder[F, Double]
  def longHistogram(name: String): Histogram.Builder[F, Long]
  def doubleHistogram(name: String): Histogram.Builder[F, Double]
  ...
}

3) A builder's of method to change the instrument's type

trait Meter[F[_]] {
  def counter(name: String): Counter.Builder[F, Long]
  def histogram(name: String): Histogram.Builder[F, Double]
  ...
}

val longCounter = Meter[F].counter("name").build
val doubleCounter = Meter[F].counter("name").of[Double].build

[rossabaker]

We are not using this at work, so I'm not opposed to breaking changes.

I think the Generic API reads nicer to people who know what they're doing (i.e., won't be scared of that context bound), and the Separate API reads nicer to people new to the library or particularly to Scala. The first one needs examples in the Scaladoc if we go that route.

The spec kind of supports either of those approaches in its language-agnostic way:

If strong type is desired, OpenTelemetry API authors MAY decide the language idiomatic name(s), for example CreateUInt64Histogram, CreateDoubleHistogram, CreateHistogram, CreateHistogram

I guess I lean toward Option 1, but not strongly.

[bpholt]

We're not using it at work yet either (but I do hope to start switching some things over from Natchez soon), so breaking changes wouldn't hurt me yet. That said, I like option 3, then option 1, with option 2 a distant third.

If option 1 is selected, is there a specific reason to seal MeasurementValue? e.g. maybe you'd want to have an instance for a refined or newtyped Long? Idk.

[iRevive]

e.g. maybe you'd want to have an instance for a refined or newtyped Long? Idk.

Thanks for the feedback! That's a valid point.

The trait is sealed due to the limitations of my naive implementation:

trait Meter[F[_]] {
  def counter[A: MeasurementValue](name: String): Counter.Builder[F, A] = 
    MeasurementValue[A] match {
      case MeasurementValue.LongMeasurementValue => longBuilder(name)
      case MeasurementValue.DoubleMeasurementValue => doubleBuilder(name)
    }
}

I guess with with a smarter alternative, we can allow arbitrary types too.

[iRevive]

I think the Generic API reads nicer to people who know what they're doing (i.e., won't be scared of that context bound), and the Separate API reads nicer to people new to the library or particularly to Scala. The first one needs examples in the Scaladoc if we go that route.

Thanks for the feedback!

I agree that generic API requires detailed Scaladoc and site documentation.

[bpholt]

The trait is sealed due to the limitations of my naive implementation … I guess with with a smarter alternative, we can allow arbitrary types too.

Maybe providing a contramap method would be enough, so that you can generate instances for things that can be converted to the listed types.

[iRevive]

I made preview PRs.

1) Generic API - https://github.com/typelevel/otel4s/pull/449 2) Builder with of - https://github.com/typelevel/otel4s/pull/448

MeasurementValue has contramap now, so a MeasurementValue for arbitrary types can be derived.

---

The implementations are nearly identical under the hood. So it's a matter of taste and preference which API to keep.

[armanbilge]

(1) feels more natural to me, but I do see the practical benefits of (2). What determines the "blessed" default type for an instrument?

[iRevive]

What determines the "blessed" default type for an instrument?

Hard to say. I choose the same default types as OpenTelemetry Java: 1) counter - Long 2) upDownCounter - Long 3) histogram - Double 4) gauge - Double

This selection also matches my use cases at $work.

---

Prometheus, for example, does not care and offers overloaded alternatives for Long and Double:

import io.prometheus.metrics.core.metrics.Counter

val failures: Counter = Counter
    .builder()
    .name("akka_dispatcher_failures")
    .help("The number of failed tasks")
    .labelNames("dispatcher")
    .build()

failures.inc(1L)
failures.inc(1.0)

Technically, we can do the same in the SDK module. But it would be problematic to integrate with OpenTelemetry Java.

[CJSmith-0141]

I very much like styles 3 and 1, both feel typelevel-y. I think that 3 is probably easier to understand for a wider audience (and therefore would require less documentation maybe). Can't really go wrong with either.

Please not 2!

[iRevive]

Thanks everyone for the feedback! There is a generic API (option 1) now.