Prototype OTel-Go metrics APIv2 proposal #2044
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2044 +/- ##
=======================================
- Coverage 72.8% 72.5% -0.4%
=======================================
Files 166 171 +5
Lines 8168 8199 +31
=======================================
- Hits 5953 5949 -4
- Misses 1987 2020 +33
- Partials 228 230 +2
|
| @@ -0,0 +1,53 @@ | |||
| package asyncfloat64metric | |||
There was a problem hiding this comment.
What is the reason for such fine-grained packages separation? What problem does it solve? Would it not make the API less discoverable?
There was a problem hiding this comment.
I was experimenting, here. @rakyll gave feedback a while back, here, which suggested focusing on the package directory structure and ensuring that the top-level package is a good entry point.
The problem I am trying to address is that we have 2 number types, and 2 calling styles. The number types create parallel interfaces that are essentially identical, and the calling styles create parallel interfaces with certain differences. If you put all four of these interfaces into one package, the godoc becomes difficult to read. My expectation is that users will not have to import these sub-packages, they would declare instruments like:
myCounter = metricglobal.Meter("library").Int64().Counter("name1")
myGauge = metricglobal.Meter("library").Asynchronous().Float64().Gauge("name2")
There was a problem hiding this comment.
Just giving a few cents (not sure how much help it would be). e.g. github.com/prometheus/client_golang/prometheus has the following description to address the overwhelming godoc:
The number of exported identifiers in this package might appear a bit overwhelming. However, in addition to the basic plumbing shown in the example above, you only need to understand the different metric types and their vector versions for basic usage.
Not an easy problem to address...
There was a problem hiding this comment.
Would it be prudent to remove some of the repetition?
Like:
metric2/asyncmetric/float64/asyncfloat64metric.gometric2/syncmetric/float64/asyncfloat64metric.go
There was a problem hiding this comment.
I like this, will try it out.
|
Please note this is an incomplete proposal, a work-in-progress. I put it into a DRAFT PR because it was mentioned in today's Spec SIG meeting. I'm definitely interested in feedback, but it's not at all documented at this time. |
There was a problem hiding this comment.
@jmacd thanks for taking a stab at this. My main concern is that much of our instrumentation code will need to be updated to support this new API. I know it isn't stable yet, and that is part of the deal with relying on unstable packages.
Ideally the end user would only need to use the metric, and your changes seem to accomplish that goal.
metric2/asyncmetric/asyncmetric.go
Outdated
| type Meter struct { | ||
| } | ||
|
|
||
| type Callback struct { |
There was a problem hiding this comment.
Are you replacing the term Observer with Callback? Is this some sort of Handler? I think handler is a term most Go programmers are familiar with.
There was a problem hiding this comment.
Thanks for pointing this out. The Specification is moving to use "Asynchronous" to describe the instruments that use a callback. Yes, this object is a Handler, but the Spec also uses "Callback" to describe the active piece of code that makes observations. We can call it "Handler" in Go if everyone agrees.
I was trying to steer this API toward a lower-level interface than the former API. This Callback is the function-object passed to the BatchObserver instrument in the former API, here:
opentelemetry-go/metric/metric.go
Line 54 in 484258e
and the idea is to only support this batch-oriented API for async instruments. Helpers could be provided for singleton observers, and of course this would be way easier with generics.
metric2/instrument/instrument.go
Outdated
| @@ -0,0 +1,4 @@ | |||
| package instrument | |||
There was a problem hiding this comment.
Could this live in the metric package instead?
There was a problem hiding this comment.
There's a cycle if I do this. I'll try and move things around, thanks for pointing this out.
There was a problem hiding this comment.
I have restructured the draft API so that the top-level metric package contains the Instrument and Measurement types, with sub-package meter, meter/asyncint64, meter/asyncfloat64, meter/syncint64, and meter/syncfloat64. I'm using a type named Builder for the instrument constructors, to make it clear that you only need to know sync/async and int64/float64 when building instruments, not when recording measurements.
metric2/metric.go
Outdated
| type Meter struct { | ||
| } | ||
|
|
||
| func (m Meter) Integer() int64metric.Meter { |
|
I will apply updates from all the standing recommendations and return with some progress in next week's SIG. This week the U.S. July 4 holiday stood in the way of more development on this PR. |
| return metric.Measurement{} | ||
| } | ||
|
|
||
| func (u UpDownCounter) Measure(x int64) metric.Measurement { |
There was a problem hiding this comment.
What does Measure do? Can it be removed? Maybe consider to be added later?
There was a problem hiding this comment.
Yes. Measure() creates a measurement for batch recording in both synchronous and asynchronous instruments, and it can be added later. My goal in sharing this prototype was to show how it would evolve to support batch measurements since that is an important way to optimize metrics costs. The other way that we know of to optimize at the API level involves binding instruments, which I left out of this proposal (and I consider less important than batch support).
This is a DRAFT. The goal of this API proposal is to implement open-telemetry/oteps#146 using "modern" metrics terminology in the project. The hypothesis is that the new metrics API can produced without significantly modifying or breaking the existing API, to assist with a smooth transition.