feat!(metrics): remove batch observer

[dyladan]

Fixes #2565

This removes the batch observer which was removed from the API in order to simplify the 1.0 release

[codecov]

Codecov Report

Merging #2566 (7f11892) into main (5bb8322) will increase coverage by 0.28%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2566      +/-   ##
==========================================
+ Coverage   92.71%   93.00%   +0.28%     
==========================================
  Files         130      138       +8     
  Lines        4478     5092     +614     
  Branches      966     1095     +129     
==========================================
+ Hits         4152     4736     +584     
- Misses        326      356      +30     

Impacted Files	Coverage Δ
...metry-sdk-metrics-base/src/ObservableBaseMetric.ts	90.90% <0.00%> (-9.10%)	⬇️
...ckages/opentelemetry-sdk-metrics-base/src/Meter.ts	87.50% <0.00%> (-1.10%)	⬇️
...ages/opentelemetry-api-metrics/src/types/Metric.ts	100.00% <0.00%> (ø)
...emetry-sdk-metrics-base/src/BatchObserverResult.ts
...pentelemetry-sdk-metrics-base/src/BatchObserver.ts
...exporter-metrics-otlp-http/src/transformMetrics.ts	95.65% <0.00%> (ø)
...opentelemetry-exporter-trace-otlp-http/src/util.ts	100.00% <0.00%> (ø)
...ation-xml-http-request/src/enums/AttributeNames.ts	100.00% <0.00%> (ø)
...s/opentelemetry-instrumentation-fetch/src/fetch.ts	96.98% <0.00%> (ø)
...-instrumentation-fetch/src/enums/AttributeNames.ts	100.00% <0.00%> (ø)
... and 6 more

[vmarchaud]

lgtm

[aabmass]

LGTM

[obecny]

The thing is it wasn't either removed or specified yet and it is quite useful when you have to update many observable metrics at the same time. If we remove it we will have to implement some caching mechanism to avoid cpu time consuming calculations or IO operations. Currently we can read async stats from certain library and update as much observables as we want. Without this it would be more complicated. Anyway I'm not blocking that if that is the way to go, you have been made aware of this :).

[legendecas]

I would also +1 for BatchObserver to be part of v1 of metrics API, it is very useful when a single observation resulted in a series of observable updates. But I'm not against removing it if the spec team is determined to remove it from v1.

[legendecas]

I think https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-api-metrics/src/types/Observation.ts and https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-api-metrics/src/types/Metric.ts#L161 should also be removed as part of this PR as they are only been used in BatchObservers.

[vmarchaud]

Well if its not in the v1 of the metrics spec, we can't have it if we want to release 1.0 right ? That would means we declare stable a implementation that isn't frozen at the spec level i believe

[weyert]

I think we could package it up in a separate extras/Utils package? But I agree if it’s not part of v1 it shouldn’t be part of it. Similarly there are also no sync updown counters either; requiring me to write a sync wrapper for the the asynchronous one.

[dyladan]

Brought this up in the maintainers SIG this morning. They suggested we can either remove the feature or keep it by somehow marking the feature as experimental in some way so that it is not stable but doesn't affect metrics api 1.0 stability.

Unfortunately there is no jsdoc tag for this, but we could probably find another way.

[vmarchaud]

Could we just let it inside this package and not move it to the api repo afterwards ?

[legendecas]

The problem with the batch observer API is that the current API spec for creating an async instrument requires a callback since every async instrument has to know how to collect stats. However, the callback is optional in the current SDK, so that we can collect stats in the batch observer's callback instead of the callbacks of each async instrument. So things might get confusing for users if we are going forward without the batch observer being removed:

const observable = meter.createObservableGauge('test', () => { /* a noop callback is required */ });
const observer = meter.createBatchObserver(result => {
  // a second callback!
  result.observe({}, [ observable.observation(0) ]);
});

Notably, BatchObserverResult has to be working with observable.observation, and it is part of the Observable API so if we're going to keep the BatchObserver then all SDKs have to implement the method. I'd believe this is not promising.

I'm still leaning on hard compliance with API spec and marking the callback parameter as required (#2569) and removing all batch observer stuff if it is determined.

[dyladan]

The problem with the batch observer API is that the current API spec for creating an async instrument requires a callback since every async instrument has to know how to collect stats. However, the callback is optional in the current SDK, so that we can collect stats in the batch observer's callback instead of the callbacks of each async instrument. So things might get confusing for users if we are going forward without the batch observer being removed:

const observable = meter.createObservableGauge('test', () => { /* a noop callback is required */ });
const observer = meter.createBatchObserver(result => {
  // a second callback!
  result.observe({}, [ observable.observation(0) ]);
});

Notably, BatchObserverResult has to be working with observable.observation, and it is part of the Observable API so if we're going to keep the BatchObserver then all SDKs have to implement the method. I'd believe this is not promising.

I'm still leaning on hard compliance with API spec and marking the callback parameter as required (#2569) and removing all batch observer stuff if it is determined.

I agree with you here. It doesn't solve the problem about atomic/transactional changes but I believe the batch observer API will be different than we have it here so it will need to be changed anyways, and it complicates the SDK quite a bit so removing it will make the SDK implementation changes simpler I believe.