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

[metric spec review] MetricReader implements non-specified ForceFlush #3394

Closed
dyladan opened this issue Nov 7, 2022 · 3 comments
Closed
Labels
sdk:metrics Issues and PRs related to the Metrics SDK
Milestone

Comments

@dyladan
Copy link
Member

dyladan commented Nov 7, 2022

MetricReader doesn't have ForceFlush according to the SDK spec.

async forceFlush(options?: ForceFlushOptions): Promise<void> {

Originally posted by @reyang in open-telemetry/community#1204 (comment)

@dyladan dyladan changed the title [metric spec review] 6 [metric spec review] MetricReader implements non-specified ForceFlush Nov 7, 2022
@dyladan
Copy link
Member Author

dyladan commented Nov 7, 2022

MetricReader doesn't have ForceFlush according to the SDK spec.

open-telemetry/opentelemetry-js@d154066/experimental/packages/opentelemetry-sdk-metrics/src/export/MetricReader.ts#L171

Hmm, correct. 🤔
However, it is referenced in the spec for the MeterProvider.

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

Both the Python and Java SDK implementations seem to have solved it in the same way. May be worth opening a spec issue over if one does not yet exist. 🤔

originally posted by @pichlermarc

@dyladan dyladan added this to the Metrics GA milestone Nov 7, 2022
@pichlermarc
Copy link
Member

Adding to above comment:

I think this is just something that is missing in the specification, I have created an issue (open-telemetry/opentelemetry-specification#2924) to clarify.

IMO this should not block the release as there is a precedent of other SDKs implementing it in the same way as we do.

@legendecas legendecas added the sdk:metrics Issues and PRs related to the Metrics SDK label Nov 7, 2022
@dyladan
Copy link
Member Author

dyladan commented Nov 8, 2022

Given the spec wording that exists and other implementations, I think we're ok to close this. @legendecas @pichlermarc @reyang feel free to reopen or comment if you disagree

@dyladan dyladan closed this as completed Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sdk:metrics Issues and PRs related to the Metrics SDK
Projects
None yet
Development

No branches or pull requests

3 participants