-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Verify compliant metric SDK specification implementation: MetricExporter/Push Metric Exporter #3666
Comments
Requirements from reading the Push Metric Exporter Section of the spec.
Notes: |
Our exporter interface has an export function that takes a batch of metrics as an argument. opentelemetry-go/sdk/metric/exporter.go Line 52 in e26d8bd
The batch of metrics includes the meter name and version as the scope in the batch of metrics. Similar to OTLP, the scope is grouped with the metrics that were created by the meter: opentelemetry-go/sdk/metric/metricdata/data.go Lines 36 to 39 in e26d8bd
(Interpretation) Push Reader must not call Export concurrently. collectAndExport calls from the periodic collect and forceflush are serialized in the run() function: opentelemetry-go/sdk/metric/periodic_reader.go Lines 168 to 185 in e26d8bd
The only other call to export comes during shutdown, which stops the above run() loop before exporting: opentelemetry-go/sdk/metric/periodic_reader.go Lines 326 to 340 in e26d8bd
(Interpretation) There must be a cancellation mechanism for Export. Also there is no way to stop execution of an exporter that won't respect cancellation, this is a limitation of the language. Export takes a context, which provides a cancellation mechanism, and the reader's timeout is applied to the context before Export() is called: opentelemetry-go/sdk/metric/exporter.go Line 52 in e26d8bd
opentelemetry-go/sdk/metric/periodic_reader.go Lines 226 to 233 in e26d8bd
The timeout is not applied during Shutdown(). That will be fixed by #4356
We do not implement any retry logic.
Our export function takes a batch of metric points: opentelemetry-go/sdk/metric/exporter.go Line 52 in e26d8bd
We do not directly rely on the OTLP proto files. The metricdata correctly mirrors the data model.
Export returns an error, which allows determining if the export succeeded or failed.
ForceFlush returns an error.
We only invoke ForceFlush on exporters when ForceFlush is invoked on the associated Reader. The SDK does not make any other calls to ForceFlush: opentelemetry-go/sdk/metric/periodic_reader.go Lines 299 to 317 in e26d8bd
Should we add disclaimers on ForceFlush to mirror this language in the specification?
ForceFlush is implemented as a blocking API. collectAndExport calls DO use the reader's timeout, but ForceFlush on the PeriodicReader does not apply a timeout to ForceFlush calls to the exporter. Should we apply the PeriodicReader's timeout on the ForceFlush call to the exporter?
Timeout is configurable via the context argument.
Our Exporter interface has a Shutdown Method: opentelemetry-go/sdk/metric/exporter.go Line 72 in e26d8bd
(Interpretation) Push Reader must call Shutdown on its Exporter
(Interpretation) Push Reader must not call Shutdown after it has been called once. exporter.Shutdown() is called within a block that is invoked within shutdownOnce.Do(, which ensures exporter.Shutdown is only invoked once.
(Non-Normative) After the call to Shutdown subsequent calls to Export are not allowed and should return a Failure result. We return ErrReaderShutdown in subsequent calls to reader.Shutdown:
(Interpretation) There must be a cancellation mechanism for Shutdown. Also there is no way to stop execution of an exporter that won't respect cancellation, this is a limitation of the language. Shutdown takes a context as a argument, which is used for timeout and cancellation (optional) shutdown timeout configurable. Timeout is configurable via the provided context.Context argument. |
After #4356 and #4359, the only item above that isn't addressed is: Should we apply a timeout on the ForceFlush call to the exporter? I'm leaning toward not doing this for a few reasons:
We can discuss on Thursday. |
I think that we already had some precedence where we agreed that using |
From SIG meeting: Ideally, the timeout in the passed context from the user would be used for both the |
|
Closing as this looks done. Please reopen if this was in error. |
The text was updated successfully, but these errors were encountered: