Use generics for metric instrument API #3973
Conversation
MrAlias
requested review from
jmacd,
Aneurysm9,
evantorrie,
XSAM,
dashpole,
MadVikingGod,
pellared,
hanyuancheung and
dmathieu
as code owners
MrAlias
force-pushed
the
generic-metric-api
branch
2 times, most recently
from
740a152
to
b5808d1
Compare
MrAlias
force-pushed
the
generic-metric-api
branch
from
b5808d1
to
0674a76
Compare
MrAlias
force-pushed
the
generic-metric-api
branch
from
cd2a4f2
to
42d808b
Compare
MrAlias
changed the title
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3973 +/- ##
======================================
Coverage 82.1% 82.1%
======================================
Files 174 172 -2
Lines 12975 12758 -217
======================================
- Hits 10655 10482 -173
+ Misses 2100 2067 -33
+ Partials 220 209 -11
|
MrAlias
force-pushed
the
generic-metric-api
branch
from
812c667
to
7f23272
Compare
MrAlias
force-pushed
the
generic-metric-api
branch
from
6967f6a
to
d22fc12
Compare
| type CounterConfig[N int64 | float64] struct { | ||
| description string | ||
| unit string | ||
| } |
There was a problem hiding this comment.
Do you need config per record type? A.k.a what about CounterConfig not having the generic type.
There was a problem hiding this comment.
The Observable configs need to be split on number type given the callback from WithCallback is defined by number. It was done for all the configurations to provide consistency, but also to ensure the forward compatibility with OTel if they decide in the future to add options that are defined by number.
There was a problem hiding this comment.
Not sure I understand the "but also to ensure the forward compatibility with OTel if they decide in the future to add options that are defined by number." unless the capability is exactly the same but the implementation differs by the number type (which I believe you can solve at the construction time) I am failing to understand this benefit, maybe an example would be good.
For WithCallback is only for async, and you can do a runtime error during construction if you want by keeping the WithCallback generic and return a non generic Option and validate the type during construction (meter.NewFoo).
| ObserveFloat64(obsrv instrument.ObservableT[float64], value float64, attributes ...attribute.KeyValue) | ||
| // ObserveInt64 records the int64 value with attributes for obsrv. | ||
| ObserveInt64(obsrv instrument.Int64Observable, value int64, attributes ...attribute.KeyValue) | ||
| ObserveInt64(obsrv instrument.ObservableT[int64], value int64, attributes ...attribute.KeyValue) |
There was a problem hiding this comment.
Not clear what advantage has this design where you ask users to call this vs asking users to call "Observe" on the ObservableT directly, since they have to have the ObservableT instances when registering the callback.
If we do that, we don't need ObservableT to be public anymore.
There was a problem hiding this comment.
The types implementing ObservableT do not have and Observe method anymore. This was a follow up to a decision to refactor the RegisterCallback API so it can provide the guarantees the specification recommends and requires.
The other way of allowing users to directly call Observe did not clearly restrict when they can call it and therefore provided potential for unintentional bugs from the user. It also wasn't a design we could make compliant with the specification given we couldn't completely restrict that method to only be called in a callback registered with the instrument.
There was a problem hiding this comment.
I missed the Observe removing part, was familiar with Java where they have that. Makes sense then the overall design, just make sure you are allowed to extend the Callback interface to allow recording more "types" in the future.
|
I could be helpful to users if we deprecate the old types (as type aliases) for a release. |
|
If #3971 does get merged, will there be a follow-up to genericitize the options? |
Yeah that was my plan 👍 (I'm already buckled in and ready for the merge conflict pain 😉). |
|
Two things from conversations IRL about this PR:
|
Initially, I was leaning towards having generics in both API and SDK. However, right now I start to hesitate. I got a feeling that when I see generics in SDK the generics just make the things "more concrete" and remove noise in docs and API. While in the API it makes another layer of abstraction which makes it harder for me to reason about the API (e.g. #3973 (comment)). I am sorry that my comment is so unprecise. I decided to leave some comment at this point rather than not leaving any feedback at all. |
|
I'm leaning towards closing this PR.
Base on this, using generics only seems like a half-way "good fit" for the configuration. It has the benefit of unifying As mentioned above we could keep the instrument options/configuration non-generic and only make the changes for the instruments. However, I'm not sure the benefit of generic instruments outweighs future compatibility issue we will need to document and address for SDKs. |
|
Closing based on consensus from SIG meeting. |
This refactors the
go.opentelemetry.io/otel/metric/instrumentpackage to use generics for instruments and their configuration. Additionally, this adds benchmarks for the global metric implementation to ensure no performance regression.This reverses the decision made in #2494 because of the following reasons:
go modfor any Go < 1.18With #3971 planned to be merged, it will add 18 new configuration declarations if we do not generalize on the number type. It will only add 9 if we do.
Benchmarks
otel/sdk/metricotel/internal/global