Skip to content
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

Is a MetricReader supposed to implement ForceFlush? #2924

Open
pichlermarc opened this issue Nov 7, 2022 · 4 comments
Open

Is a MetricReader supposed to implement ForceFlush? #2924

pichlermarc opened this issue Nov 7, 2022 · 4 comments
Assignees
Labels
area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory

Comments

@pichlermarc
Copy link
Member

What are you trying to achieve?

The specification for ForceFlush on the MeterProvider mentions that

ForceFlush MUST invoke ForceFlush on all registered MetricReader and Push Metric Exporter instances.

However, the specification for MetricReader's operations does not specify ForceFlush.

What did you expect to see?

It seems like the spec hints that MetricReader should implement ForceFlush. If this is the intention, it would be nice to have this clarified by explicitly specifying it in the MetricReader operations spec. 🙂

The goal of this issue is to determine if MetricReader should implement ForceFlush, and - if yes - clarify the spec accordingly.

Additional context.

This came up during the spec review of the JavaScript Metrics SDK, see open-telemetry/community#1204 (comment)

Aside from the JavaScript SDK at least 2 other language SDKs do already implement a ForceFlush on the MetricReader, see:

@srikanthccv
Copy link
Member

IIRC, when we discussed the same issue for Python implementation (issue here open-telemetry/opentelemetry-python#2554), I believe it was @jack-berg who pointed us that there is a text in MetricReader spec that says.

The SDK SHOULD provide a way to allow MetricReader to respond to MeterProvider.ForceFlush and MeterProvider.Shutdown. OpenTelemetry SDK authors MAY decide the language idiomatic approach, for example, as OnForceFlush and OnShutdown callback functions.

I think it could have been clearer if there was a ForceFlush defined in the list of MetricReader operations, but an implementation with ForceFlush is still a spec complaint.

@jsuereth
Copy link
Contributor

@srikanthccv Answered this well. You can use different method names, appropriate to the paradigm you use for MetricReader.

However, the key is that you implement a working ForceFlush for appropriate readers. (E.g. PeriodicMetricReader should perform a flush when this is called, while a Pull-based reader/exporter may be unable to).

@reyang reyang added the area:sdk Related to the SDK label Nov 18, 2022
@reyang
Copy link
Member

reyang commented Nov 18, 2022

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#forceflush mentioned:

This method provides a way for provider to notify the registered MetricReader and MetricExporter instances, so they can do as much as they could to consume or send the metrics. Note: unlike Push Metric Exporter which can send data on its own schedule, Pull Metric Exporter can only send the data when it is being asked by the scraper, so ForceFlush would not make much sense.

@reyang
Copy link
Member

reyang commented Nov 18, 2022

@jsuereth @jmacd I feel it will probably help if we define MetricReader.ForceFlush in the spec, what do you think?

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

No branches or pull requests

4 participants