Skip to content

Spec out the MeterProvider ForceFlush and Shutdown requirement - #1913

Merged
jsuereth merged 5 commits into
open-telemetry:mainfrom
reyang:reyang/metrics-sdk-flush-shutdown
Sep 14, 2021
Merged

Spec out the MeterProvider ForceFlush and Shutdown requirement#1913
jsuereth merged 5 commits into
open-telemetry:mainfrom
reyang:reyang/metrics-sdk-flush-shutdown

Conversation

@reyang

@reyang reyang commented Sep 9, 2021

Copy link
Copy Markdown
Member

This was previously left as TODO since it has dependency on the exporter (#1864) and reader (#1888) interface.

@reyang
reyang requested review from a team September 9, 2021 00:23
This method provides a way for provider to do any cleanup required.

`Shutdown` MUST be called only once for each `MeterProvider` instance. After the
call to `Shutdown`, subsequent attempts to get a `Meter` are not allowed. SDKs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest that subsequent calls just return failure status.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the wording from https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#shutdown.

I think the tricky part is "return failure status", we try to give language clients flexibility by saying "provide some way" so they can decide if exception or return value or something else (e.g. a global error handler) makes more sense.

Comment thread specification/metrics/sdk.md Outdated

TODO
This method provides a way for provider to notify the registered
[MetricReader](#metricreader) and [MetricExporter](#metricexporter) instances,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly MetricExporter is not directly connected to the Provider, so just notify the Reader is enough correct? It is Reader's responsibility to notify Exporter

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Pull Exporters we've agreed to give implementers flexibility so they could (although we haven't seen a strong reason to do so) implement the Pull Exporter bypassing the Reader. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#pull-metric-exporter

Implementors MAY choose the best idiomatic design for their language. For example, they could generalize the Push Metric Exporter interface design and use that for consistency, they could model the pull exporter as MetricReader, or they could design a completely different pull exporter interface.

The wording here ensures that all the Readers and Exporters will be covered. An implementation could achieve this by having the Provider notifying the Readers, and Readers notifying Exporters.

@reyang reyang added area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory labels Sep 13, 2021
@jsuereth
jsuereth merged commit efbcc0d into open-telemetry:main Sep 14, 2021
@reyang
reyang deleted the reyang/metrics-sdk-flush-shutdown branch September 14, 2021 03:57
owais pushed a commit to owais/opentelemetry-specification that referenced this pull request Sep 14, 2021
…telemetry#1913)

* spec out the MeterProvider ForceFlush and Shutdown requirement

* adjust wording

* adjust wording

* fix wording
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
…telemetry#1913)

* spec out the MeterProvider ForceFlush and Shutdown requirement

* adjust wording

* adjust wording

* fix wording
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants