Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Experimental] Meter API: make instrument builders generic, extend metrics API #433

Closed
wants to merge 1 commit into from

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented Jan 15, 2024

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

The problem

There are several issues this PR tries to address:

  1. Missing builders for the following instruments: Counter[F, Double], UpDownCounter[F, Double] , Histogram[F, Long], Gauge[F, Long]
  2. Incomplete Histogram builder API (e.g. withExplicitBucketBoundaries(...) is missing)

Proposals

1) Make Meter API generic

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

2) Merge observable and sync builders

The implementation depends on the language. For example, C# offers a separate API to create Counter or ObservableCounter:

meter.CreateCounter<UInt64>("exceptions", description="number of exceptions caught");
meter.CreateObservableCounter<UInt64>("caesium_oscillates", () => clock.GetCaesiumOscillates());

On the other hand, Open Telemetry Java SDK offers separate methods within the instrument builder:

meter.counterBuilder("exceptions").build()
meter.counterBuilder("caesium_oscillates").buildWithCallback(cb => cb.record(1L))

As an experiment, I followed the Java SDK approach.

Before:

val meter: Meter[IO] = ???
val counter = meter.counter("name").create
val asyncCounter = meter.observableCounter("name").createWithCallback(cb => cb.record(1L))

After:

val meter: Meter[IO] = ???
val counter = meter.counter[Long]("name").create
val asyncCounter = meter.counter[Long]("name").createWithCallback(cb => cb.record(1L))

Benefits

1) The API is more similar to OpenTelemetry Java SDK

2) 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. #426).

The questions

  1. How do you feel about the API changes
  2. Would you prefer having unified or separate builders for sync and observable instruments? (e.g. .counter(...) vs .observableCounter(...))

@iRevive iRevive added the metrics Improvement to metrics module label Jan 15, 2024
@iRevive iRevive closed this Jan 17, 2024
@iRevive iRevive deleted the generic-metrics-instrument branch January 28, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metrics Improvement to metrics module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant