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

Explicitly define metric reader force flush operation #3084

Closed

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Jan 9, 2023

Related: #2983
Related: open-telemetry/opentelemetry-js#3278

Changes

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

The method MetricReader.ForceFlush is already mentioned in https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#forceflush. However, the specification doesn't explicitly define the method MetricReader.ForceFlush.

This also suggests that ForceFlush of Periodic exporting MetricReader should call Collect to consume the metrics.

@legendecas legendecas force-pushed the metric-reader-flush branch 3 times, most recently from 80cfeb3 to 721a073 Compare January 9, 2023 03:41
@reyang
Copy link
Member

reyang commented Jan 12, 2023

The method MetricReader.ForceFlush is already mentioned in https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#forceflush. However, the specification doesn't explicitly define the method MetricReader.ForceFlush.

This also suggests that ForceFlush of Periodic exporting MetricReader should call Collect to consume the metrics.

Could this be fixed by changing:

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

to:

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

@legendecas
Copy link
Member Author

legendecas commented Jan 13, 2023

@reyang for pull metric reader, I don't think it is meaningful to invoke Collect on force flush, similar to pull metric exporter mentioned at https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#pull-metric-exporter.

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions
Copy link

github-actions bot commented Feb 1, 2023

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Feb 1, 2023
@legendecas
Copy link
Member Author

Not stale, friendly ping @reyang

@reyang
Copy link
Member

reyang commented Feb 2, 2023

Not stale, friendly ping @reyang

Hi @legendecas, what's expected from me?

@github-actions github-actions bot removed the Stale label Feb 3, 2023
@legendecas
Copy link
Member Author

@reyang has your question #3084 (comment) been addressed with #3084 (comment)? If so, is this PR still looking good to you?

@reyang
Copy link
Member

reyang commented Feb 3, 2023

@reyang has your question #3084 (comment) been addressed with #3084 (comment)? If so, is this PR still looking good to you?

I didn't ask a question, in the comment I had a suggestion. With the change proposed by the current PR, I find it hard to understand what's the difference between MetricReader.Collect and MetricReader.ForceFlush.

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Feb 11, 2023
@legendecas
Copy link
Member Author

@reyang MetricReader.Collect can be initiated by external collectors like prometheus scraping. However, MetricReader.ForceFlush is intended for sdk to notify MetricReaders that they should consume and send as much metric as they can.

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

The statement in the spec has already mentioned that SDK's ForceFlush should invoke MetricReader's ForceFlush. This change doesn't change this behavior.

@github-actions github-actions bot removed the Stale label Feb 14, 2023
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Feb 22, 2023
@github-actions
Copy link

github-actions bot commented Mar 1, 2023

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants