PeriodicExportingMetricReader should collect all metrics on shutdown

[aabmass]

What are you trying to achieve?

Capture metrics which may be abnormal around the time my service stops. When my application exits or I otherwise call MeterProvider.shutdown(), I want to do a final collection of metrics for all push based exporters. I would potentially also expect MeterProvider.forceFlush() to do the same thing.

What did you expect to see?

Calling MeterProvider.shutdown() should cause any registered PeriodicExportingMetricReader to do a final collection and send the data out through the push exporters. Right now this behavior is not specified as far as I can tell.

Additional context.

The Python SDK does this already and I believe Java does as well (cc @jsuereth). However, JS SDK doesn't do this right now open-telemetry/opentelemetry-js#3278 and you have to manually call reader.collect().

[trask]

I believe Java does as well

just to clarify, in Java this functionality is part of the autoconfiguration layer (which is a separate artifact from the SDK itself):

https://github.com/open-telemetry/opentelemetry-java/blob/f6deb4c1b7b9ed250f3f7cda992a012b6e738af2/sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkBuilder.java#L371-L379

[aabmass]

@trask it looks like that code triggers the SDK to shut down when the process is ending, which is a separate important issue.

This issue is about what the shutdown behavior is; the spec doesn't require PeriodicExportingMetricReader.shutdown() to trigger a collection of metrics at all (this code in Java).

[reyang]

The tracing spec has added something similar https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#shutdown-1.

Shutdown MUST include the effects of ForceFlush.

[danielgblanco]

Python and Java implementations do indeed force flush on shutdown, but Node does not (even though this comment indicates that it does, it doesn't). This could be a source of confusion for users, and maybe worth specifying?

[joebowbeer]

Currently, the OTEL Metrics SDK spec documents shutdown and forceFlush as two separate methods, with no interdependencies:

https://opentelemetry.io/docs/specs/otel/metrics/sdk/#shutdown

Are you proposing that MeterProvider.shutdown() should be spec'd to call MeterProvider.forceFlush()?

Or that on PeriodicExportingMetricReader.shutdown() should be spec'd to call reader.collect()?

[trask]

Tracing and logs shutdown includes flush in the specification.

@open-telemetry/technical-committee should metric shutdown also include flush?

[jack-berg]

@open-telemetry/technical-committee should metric shutdown also include flush?

I don't see any reason not to. Java and python have been doing this for years without issue. It doesn't appear that anyone has any arguments on why we shouldn't do this. If no one makes such an argument, let's update the spec.

[jack-berg]

@jsuereth has agreed to sponsor this.