feat(sdk-metrics): PeriodicExportingMetricReader now flushes pending tasks at shutdown

[macno]

Which problem is this PR solving?

Metric provider behave differently from traces and logs providers during shutdown process, being the only one that does not flush pending tasks.

Although it's not explicitly required (yet) by the specs for metrics and logs as it is for traces, this PR adds flushing metrics on shutdown, with the intent of eliminating a not infrequent confusion and wrong assumption among users.
This way we consistently ensure the flush for all supported signals upon shutdown.

The same approach has already been taken by java and python SDKs.

Short description of the changes

now MeterProvider#shutdown() calls MeterProvider#forceFlush()

Type of change

• New feature (non-breaking change which adds functionality)

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added updated

[pichlermarc]

Hi @macno thanks for working on this 🙂

I think the agreement on open-telemetry/opentelemetry-specification#2983 was on the PeriodicExporingMetricReader - not the MeterProvider. Would you mind adapting the code accordingly? 🙂

[macno]

Hi @macno thanks for working on this 🙂

I think the agreement on open-telemetry/opentelemetry-specification#2983 was on the PeriodicExporingMetricReader - not the MeterProvider. Would you mind adapting the code accordingly? 🙂

Hi @pichlermarc thanks for taking time to look at the PR.

I'm not sure if I read the same agreement, to me seems like an open point.
From my understanding, this is the shutdown flow:

graph TD;
    D@{ shape: procs, label: "metricCollector"}
    SDK-- shutdown() ---meterProvider;
    meterProvider-- shutdown() ---D
    D-- shutdown() ---metricReader

Loading

so calling forceFlush in the meterProvider will cause all the registered metricReader to execute forceFlush, meaning you don't have to implement the same logic in all the metricReader implementations to achieve the same behaviour.

What's the reason to move it to PeriodicExporingMetricReader ?

[pichlermarc]

[...]
so calling forceFlush in the meterProvider will cause all the registered metricReader to execute forceFlush, meaning you don't have to implement the same logic in all the metricReader implementations to achieve the same behaviour.

What's the reason to move it to PeriodicExporingMetricReader ?

The equivalent of PeriodicExportingMetricReader is BatchSpanProcessor and BatchLogRecordProcessor in the other SDKs and that behavior is specified for their base interfaces. Looking into what the Java and Python SIGs implemented, their MeterProviders don't apply the effects forceFlush() on shutdown(), but the PeriodicExportingMetricReader does it.

While it is true that adding it to MeterProvider would be the easiest, it seems that this intentionally split. Though I don't know what the original reasoning behind that, the spec does not elaborate on it. Anyway, that why I'm not very confident about going against the grain on this.

I asked here again, but I'm fairly certain that my assumption is correct. 🤔

Some links:

• Logs SDK Spec for LogRecordProcessor#shutdown()
• Trace SDK Spec for SpanProcessor#shutdown()
• Java Implementation of MeterProvider#shutdown() does not include any forceFlush() effects
• Python Implementation of MeterProvider#shutdown() does not include forceFlush() effects
• Java implementation of PeriodicExportingMetricReader#shutdown() does include collection before shutdown
• Python implementation of PeriodicExportingMetricReader#shutdown() does include collection before shutdown

[macno]

Ok, I will proceed as requested

[pichlermarc]

Looks like there are a few tests that rely on the old behavior, we'll need to remove the forceFlush in those:

• opentelemetry-js/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/test/OTLPMetricExporter.test.ts

  Line 77 in 556eecf

  meterProvider.forceFlush();

• opentelemetry-js/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/node/OTLPMetricExporter.test.ts

  Line 217 in 484af40

  meterProvider.forceFlush();

• opentelemetry-js/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/browser/OTLPMetricExporter.test.ts

  Line 95 in 484af40

  await meterProvider.forceFlush();

• opentelemetry-js/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/browser/OTLPMetricExporter.test.ts

  Line 55 in 484af40

  await meterProvider.forceFlush();

• opentelemetry-js/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/browser/OTLPMetricExporter.test.ts

  Line 95 in 484af40

  await meterProvider.forceFlush();

• opentelemetry-js/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/browser/OTLPMetricExporter.test.ts

  Line 55 in 484af40

  await meterProvider.forceFlush();

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.65%. Comparing base (484af40) to head (a2d7810).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5242      +/-   ##
==========================================
+ Coverage   94.60%   94.65%   +0.05%     
==========================================
  Files         315      315              
  Lines        8011     8012       +1     
  Branches     1617     1617              
==========================================
+ Hits         7579     7584       +5     
+ Misses        432      428       -4     

Files with missing lines	Coverage Δ
packages/sdk-metrics/src/MeterProvider.ts	100.00% <ø> (ø)
...etrics/src/export/PeriodicExportingMetricReader.ts	98.21% <100.00%> (+0.03%)	⬆️

... and 4 files with indirect coverage changes

[pichlermarc]

Please also add a changelog entry in ./CHANGELOG.md, then we can merge this. 🙂

[pichlermarc]

Looks good, thanks 🙂