Metrics API package structure options

[jmacd]

Problem Statement

The current Metrics API has a number of defects and off-spec features, making it suitable for full-scale replacement. In today's SIG meeting we discussed doing this quickly, to lessen the pain for users. There are a couple of organizations that appear to work, and I'd like feedback from the group.

Proposed Solution

First, the assumptions I'm making: (1) the current sdkapi was created for a pre-generics world, in a post-generics world it doesn't belong in the API, it becomes an ordinary part of the SDK, (2) all batch-recording interfaces will disappear, (3) all Must*() forms will disappear, (4) generics will not be used at this time for the API surface (I think we and the users are not ready for this).

The result will be the 12 basic instruments organized somehow, plus some way to create asynchronous callbacks and bind them to instruments. I filed a specification issues to carve out support for the proposal here, see open-telemetry/opentelemetry-specification#2280

Assuming favorable outcome, though merging

open-telemetry/opentelemetry-specification#2281

then I would like to outline two potential API structures, described
as "6 packages nested", "1 package nested", and "1 package flattened".

The 6 packages nested layout

In the top-level package, types would be named named metric.MeterProvider, metric.Meter, metric.Option.

In the four specific sub-packages (syncint64, syncfloat64, asyncint64, asyncfloat64), there are four interfaces each, one to group the instrument constructors (*.Instruments) and three concisely-named instruments. For example, there are four instruments named "Counter" (i.e., syncint64.Counter, syncfloat64.Counter, asyncint64.Counter, asyncfloat64.Counter).

The final instrument package is needed to break an import cycle between the others (e.g., instrument.Option).

otel/metric/
+ MeterProvider
  - GetMeter(string, ...Option) Meter
+ Meter
  - SyncInt64() syncint64.Instruments
  - SyncFloat64() syncfloat64.Instruments
  - AsyncInt64() asyncint64.Instruments
  - AsyncFloat64() asyncfloat64.Instruments
  - NewCallback([]instrument.Asynchronous, func(ctx context.Context) error)
+ Option
+ WithInstrumentationLibraryVersion
+ WithSchemaURL

otel/metric/asyncint64
+ Instruments
  - Counter(string, ...instrument.Option) Counter
  - UpDownCounter(string, ...instrument.Option) UpDownCounter
  - Gauge(string, ...instrument.Option) Gauge
+ Counter
  - Observe(context.Context, int64, ...attribute.KeyValue)
+ UpDownCounter
  - Observe(context.Context, int64, ...attribute.KeyValue)
+ Gauge
  - Observe(context.Context, int64, ...attribute.KeyValue)

otel/metric/asyncfloat64
... same as asyncint64 w/ int64->float64

otel/metric/syncint64
+ Instruments
  - Counter(string, ...instrument.Option) Counter
  - UpDownCounter(string, ...instrument.Option) UpDownCounter
  - Histogram(string, ...instrument.Option) Histogram
+ Counter
  - Add(context.Context, int64, ...attribute.KeyValue)
+ UpDownCounter
  - Add(context.Context, int64, ...attribute.KeyValue)
+ Histogram
  - Record(context.Context, int64, ...attribute.KeyValue)

otel/metric/syncfloat64
... same as syncint64 w/ int64->float64

otel/metric/instrument
+ Option
+ WithUnit
+ WithDescription
+ Synchronous
  - Synchronous()
+ Asynchronous
  - Asynchronous()

Pros: the types all have concise names
Cons: the user has to import 2 packages at least

Synchronous example:

import (
    "go.opentelemetry.io/otel/metric"
    "go.opentelemetry.io/otel/metric/instrument"
)
    
var meter = global.MeterProvider().Get("library")
var counter = meter.SyncInt64().Counter("somecounter", instrument.WithUnit("{something}"))

func do(ctx context.Context) {
   counter.Add(ctx, 10, attribute.String("A", "B"))
}

Asynchronous example:

import (
    "go.opentelemetry.io/otel/metric"
    "go.opentelemetry.io/otel/metric/instrument"
)

func start() {
   var meter = global.MeterProvider().Get("library")
   var counter = meter.AsyncInt64().Counter("somecounter", instrument.WithUnit("{something}"))
   var gauge = meter.AsyncInt64().Gauge("somegauge", instrument.WithUnit("millisomethings"))

   meter.NewCallback([]instrument.Asynchronous{counter, gauge}, func(ctx context.Context) {
      counter.Observe(ctx, counterValue)
      gauge.Observe(ctx, gaugeValue)
   })
}

The 1-package nested layout

The structure is similar to the 6-package nested layout but reduced to one large package:

otel/metric/
+ MeterProvider
  - GetMeter(string, ...Option) Meter
+ Meter
  - SyncInt64() SyncInt64Instruments
  - SyncFloat64() SyncFloat64Instruments
  - AsyncInt64() AsyncInt64Instruments
  - AsyncFloat64() AsyncFloat64Instruments
  - NewCallback([]Asynchronous, func(ctx context.Context) error)
+ MeterOption
+ WithInstrumentationLibraryVersion
+ WithSchemaURL
+ InstrumentOption
+ WithDescription
+ WithUnit
+ AsyncInt64Instruments
  - Counter(string, ...instrument.Option) AsyncInt64Counter
  - UpDownCounter(string, ...instrument.Option) AsyncInt64UpDownCounter
  - Gauge(string, ...instrument.Option) AsyncInt64Histogram
+ AsyncFloat64Instruments
  - Counter(string, ...instrument.Option) AsyncFloat64Counter
  - UpDownCounter(string, ...instrument.Option) AsyncFloat64UpDownCounter
  - Gauge(string, ...instrument.Option) AsyncFloat64Histogram
+ SyncInt64Instruments
  - Counter(string, ...instrument.Option) SyncInt64Counter
  - UpDownCounter(string, ...instrument.Option) SyncInt64UpDownCounter
  - Histogram(string, ...instrument.Option) SyncInt64Histogram
+ SyncFloat64Instruments
  - Counter(string, ...instrument.Option) SyncFloat64Counter
  - UpDownCounter(string, ...instrument.Option) SyncFloat64UpDownCounter
  - Histogram(string, ...instrument.Option) SyncFloat64Histogram
+ AsyncInt64Counter
  - Observe(context.Context, int64, ...attribute.KeyValue)
+ AsyncInt64UpDownCounter
  - Observe(context.Context, int64, ...attribute.KeyValue)
+ AsyncInt64Gauge
  - Observe(context.Context, int64, ...attribute.KeyValue)
+ AsyncFloat64Counter
  - Observe(context.Context, float64, ...attribute.KeyValue)
+ AsyncFloat64UpDownCounter
  - Observe(context.Context, float64, ...attribute.KeyValue)
+ AsyncFloat64Gauge
  - Observe(context.Context, float64, ...attribute.KeyValue)
+ SyncInt64Counter
  - Add(context.Context, int64, ...attribute.KeyValue)
+ SyncInt64UpDownCounter
  - Add(context.Context, int64, ...attribute.KeyValue)
+ SyncInt64Histogram
  - Record(context.Context, int64, ...attribute.KeyValue)
+ SyncFloat64Counter
  - Add(context.Context, float64, ...attribute.KeyValue)
+ SyncFloat64UpDownCounter
  - Add(context.Context, float64, ...attribute.KeyValue)
+ SyncFloat64Histogram
  - Record(context.Context, float64, ...attribute.KeyValue)
+ Synchronous
  - Synchronous()
+ Asynchronous
  - Asynchronous()

Pros: the user has only one import statement
Cons: the types have longer names; patterns in the API structure are not obvious

Synchronous example:

import (
    "go.opentelemetry.io/otel/metric"
)
    
var meter = global.MeterProvider().Get("library")
var counter = meter.SyncInt64().Counter("somecounter", metric.WithUnit("{something}"))

func do(ctx context.Context) {
   counter.Add(ctx, 10, attribute.String("A", "B"))
}

Asynchronous example:

import (
    "go.opentelemetry.io/otel/metric"
)

func start() {
   var meter = global.MeterProvider().Get("library")
   var counter = meter.AsyncInt64().Counter("somecounter", metric.WithUnit("{something}"))
   var gauge = meter.AsyncInt64().Gauge("somegauge", metric.WithUnit("millisomethings"))

   meter.NewCallback([]metric.Asynchronous{counter, gauge}, func(ctx context.Context) {
      counter.Observe(ctx, counterValue)
      gauge.Observe(ctx, gaugeValue)
   })
}

The 1-package flattened layout

The Meter gains long-name methods, but there are fewer types.

otel/metric/
+ MeterProvider
  - GetMeter(string, ...Option) Meter
+ Meter
  - AsyncInt64Counter(string, ...instrument.Option) AsyncInt64Counter
  - AsyncInt64UpDownCounter(string, ...instrument.Option) AsyncInt64UpDownCounter
  - AsyncInt64Gauge(string, ...instrument.Option) AsyncInt64Histogram
  - AsyncFloat64Counter(string, ...instrument.Option) AsyncFloat64Counter
  - AsyncFloat64UpDownCounter(string, ...instrument.Option) AsyncFloat64UpDownCounter
  - AsyncFloat64Gauge(string, ...instrument.Option) AsyncFloat64Histogram
  - AsyncFloat64Counter(string, ...instrument.Option) SyncInt64Counter
  - SyncInt64Counter(string, ...instrument.Option) SyncIntt64Counter
  - SyncInt64UpDownCounter(string, ...instrument.Option) SyncInt64UpDownCounter
  - SyncInt64Histogram(string, ...instrument.Option) SyncInt64Histogram
  - SyncFloat64Counter(string, ...instrument.Option) SyncFloat64Counter
  - SyncFloat64UpDownCounter(string, ...instrument.Option) SyncFloat64UpDownCounter
  - SyncFloat64Histogram(string, ...instrument.Option) SyncFloat64Histogram
  - NewCallback([]Asynchronous, func(ctx context.Context) error)
+ MeterOption
+ WithInstrumentationLibraryVersion
+ WithSchemaURL
+ InstrumentOption
+ WithDescription
+ WithUnit
+ AsyncInt64Counter
  - Observe(context.Context, int64, ...attribute.KeyValue)
+ AsyncInt64UpDownCounter
  - Observe(context.Context, int64, ...attribute.KeyValue)
+ AsyncInt64Gauge
  - Observe(context.Context, int64, ...attribute.KeyValue)
+ AsyncFloat64Counter
  - Observe(context.Context, float64, ...attribute.KeyValue)
+ AsyncFloat64UpDownCounter
  - Observe(context.Context, float64, ...attribute.KeyValue)
+ AsyncFloat64Gauge
  - Observe(context.Context, float64, ...attribute.KeyValue)
+ SyncInt64Counter
  - Add(context.Context, int64, ...attribute.KeyValue)
+ SyncInt64UpDownCounter
  - Add(context.Context, int64, ...attribute.KeyValue)
+ SyncInt64Histogram
  - Record(context.Context, int64, ...attribute.KeyValue)
+ SyncFloat64Counter
  - Add(context.Context, float64, ...attribute.KeyValue)
+ SyncFloat64UpDownCounter
  - Add(context.Context, float64, ...attribute.KeyValue)
+ SyncFloat64Histogram
  - Record(context.Context, float64, ...attribute.KeyValue)
+ Synchronous
  - Synchronous()
+ Asynchronous
  - Asynchronous()

Pros: the user has only one import statement
Cons: the types have longer names; patterns in the API structure are not obvious

Synchronous example:

import (
    "go.opentelemetry.io/otel/metric"
)
    
var meter = global.MeterProvider().Get("library")
var counter = meter.SyncInt64Counter("somecounter", metric.WithUnit("{something}"))

func do(ctx context.Context) {
   counter.Add(ctx, 10, attribute.String("A", "B"))
}

Asynchronous example:

import (
    "go.opentelemetry.io/otel/metric"
)

func start() {
   var meter = global.MeterProvider().Get("library")
   var counter = meter.AsyncInt64Counter("somecounter", metric.WithUnit("{something}"))
   var gauge = meter.AsyncInt64Gauge("somegauge", metric.WithUnit("millisomethings"))

   meter.NewCallback([]metric.Asynchronous{counter, gauge}, func(ctx context.Context) {
      counter.Observe(ctx, counterValue)
      gauge.Observe(ctx, gaugeValue)
   })
}

Summary

While other designs are possible for asynchronous callbacks (and there are open questions there), these three at least summarize the package layout question we have to answer.

[jmacd]

The 6-package nested layout is prototyped in #2525

[Aneurysm9]

Thanks for laying this out, this is very helpful.

First, the assumptions I'm making: (1) the current sdkapi was created for a pre-generics world, in a post-generics world it doesn't belong in the API, it becomes an ordinary part of the SDK, (2) all batch-recording interfaces will disappear, (3) all Must*() forms will disappear, (4) generics will not be used at this time for the API surface (I think we and the users are not ready for this).

These assumptions seem right to me. Is open-telemetry/opentelemetry-specification#2270 required to support (3)?

As for the layout options, the 6-package nested layout seems like a clear winner to me. It is simple and straightforward and the only "downside" looks to be importing two packages instead of one. I expect that many/most developers have tooling such as goimports handle that for them anyways. Having the package structure make clear the symmetry between the instrument interfaces and allowing developers to focus on a single type in a direct manner seems like a clear win.

[MadVikingGod]

I don't think we need the Must*() forms. They make sense in a world where you can instantiate a static form of something, like regex's and Prometheus' Observers. All of our instruments are attached to the Meter that created them, so it doesn't make sense to have a Must form. We can explore adding it if there is some kind of demand from the community, which I would expect someone to write a helper package to add them back.

My only suggestion for the 6 package layout is to nest the syncInt64 et al under instrument/. This shouldn't change how many imports are needed, and it should be a decent breadcrumb to users of what is and is not an instrument.

[MadVikingGod]

Do we want to take some precautions around equivalent interfaces? Right now asyncInt64.Counter, asyncInt64.UpDownCounter, and asyncInt64.Gauge are all the same interface (meaning they all define the exact same methods), and thus the user could accidentally convert one to another.

I don't think this is a problem, but I want to see what others have to think about it.

[jmacd]

This question's been on the back of my mind. Same question for synchronous Counter and UpDownCounter.
I would say no, not a problem, allow these conversions.

[bogdandrutu]

For the flatten pattern:

Pros: the user has only one import statement
Cons: the types have longer names; patterns in the API structure are not obvious

The "pros" is more than just one import statement, is also one godoc package to check, right now it feels like resolving a maze to find the right godoc for the instrument. Think that users who do not know your "magic" structure, will always start from the top package that contains Meter and from there you jump 3 packages in order to construct a simple counter instrument.

[pellared]

I strongly prefer the 1 package approach. I find it more "idiomatic". The Go standard library also tries to minimize the number of packages for sake of ease of use and discovery. E.g. http package has both server and client functionality. ioutil package has been deprecated and merged into io and os packages.

[MrAlias]

Closing as we have produced a revised API based on the discussions found here. We plan to release the new design as an experimental release and continue the evaluation of that design. We can reopen this issue if we find the need to regress back to discussing structure options due to the new design warranting it.