IMPORTANT - Fixing collecting data from observers when using batch observer in first run

[obecny]

I discovered a bug when using batch observer. The batch observers needs to be run first so that the batcher can update observers first. After this the rest of observers and metrics should be run to collect the data. Otherwise when using the batch observers the meter.getBatcher().checkPointSet() was returning 0 records at first run.

[codecov]

Codecov Report

Merging #1470 into master will decrease coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1470      +/-   ##
==========================================
- Coverage   94.07%   93.93%   -0.14%     
==========================================
  Files         153      148       -5     
  Lines        4656     4401     -255     
  Branches      959      899      -60     
==========================================
- Hits         4380     4134     -246     
+ Misses        276      267       -9     

Impacted Files	Coverage Δ
...s/opentelemetry-metrics/src/BatchObserverMetric.ts	93.10% <ø> (ø)
packages/opentelemetry-metrics/src/Meter.ts	96.42% <100.00%> (+0.42%)	⬆️
packages/opentelemetry-metrics/src/Metric.ts	97.22% <100.00%> (+0.16%)	⬆️
packages/opentelemetry-metrics/src/export/types.ts	100.00% <100.00%> (ø)
...ages/opentelemetry-exporter-collector/src/types.ts
...lemetry-exporter-collector/src/transformMetrics.ts
.../opentelemetry-exporter-collector/src/transform.ts
...kages/opentelemetry-exporter-collector/src/util.ts
...ry-exporter-collector/src/CollectorExporterBase.ts
packages/opentelemetry-tracing/src/Span.ts	100.00% <0.00%> (+1.01%)	⬆️

[obecny]

@open-telemetry/javascript-approvers this is important bug fix, please have a look, thx

[legendecas]

PR-wise LGTM.

However, I'm wondering if a batch observer should be an instance of Metric. According to https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/metrics/api.md#batch-observer the batch observers merely acts as an co-ordinator for other instruments. There may not be possibly instruments can be bound by the batch observer. Yet the implementation of BatchObserverMetric does inherit from Metric which can be bound to create new instruments -> those created instruments can not be updated by the callback of BatchObserverMetric.

In my thoughts, BatchObserverMetric can be simplified as separate categories with metrics and reduce confusion that BatchObserverMetric itself can be updated with values, and rename BatchObserverMetric to BatchObserver to indicate that itself is not a Metric.

[dyladan]

I agree with @legendecas but let's get this bugfix merged then we can follow up if all agree

[obecny]

PR-wise LGTM.

However, I'm wondering if a batch observer should be an instance of Metric. According to https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/metrics/api.md#batch-observer the batch observers merely acts as an co-ordinator for other instruments. There may not be possibly instruments can be bound by the batch observer. Yet the implementation of BatchObserverMetric does inherit from Metric which can be bound to create new instruments -> those created instruments can not be updated by the callback of BatchObserverMetric.

In my thoughts, BatchObserverMetric can be simplified as separate categories with metrics and reduce confusion that BatchObserverMetric itself can be updated with values, and rename BatchObserverMetric to BatchObserver to indicate that itself is not a Metric.

Lets investigate this in separate PR -> #1489