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

MetricReader.ForceFlush is not part of the spec #1086

Closed
brettmc opened this issue Jul 19, 2023 · 4 comments · Fixed by #1097
Closed

MetricReader.ForceFlush is not part of the spec #1086

brettmc opened this issue Jul 19, 2023 · 4 comments · Fixed by #1097
Labels
bug Something isn't working signal:metrics Related to metrics signal tc-review technical committee review feedback

Comments

@brettmc
Copy link
Collaborator

brettmc commented Jul 19, 2023

From open-telemetry/community#1536 (comment)

MetricReader.ForceFlush is not part of the spec.

update: issue has been replaced by open-telemetry/opentelemetry-specification#3609

@brettmc brettmc added bug Something isn't working tc-review technical committee review feedback signal:metrics Related to metrics signal labels Jul 19, 2023
@brettmc
Copy link
Collaborator Author

brettmc commented Jul 19, 2023

@reyang It looks like our implementation matches what that PR is proposing, should we just wait and watch? It also looks like we're doing what java does: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/MetricReader.java#L54

@pellared
Copy link
Member

pellared commented Jul 27, 2023

In .NET SDK only the exporters implement ForceFlush. This is why open-telemetry/opentelemetry-specification#3563 was rejected.

I think there is nothing wrong doing what open-telemetry/opentelemetry-specification#3563 is proposing. You may be interested in open-telemetry/opentelemetry-go#4375 for inspiration.

@brettmc
Copy link
Collaborator Author

brettmc commented Aug 2, 2023

Thanks @pellared that was helpful. I think I understand now, and I'll look at splitting off another interface so that ForceFlush need only be implemented by a Push Metric Exporter. It's not required by a Pull exporter (although we don't actually have an implementation of one).

@pellared
Copy link
Member

pellared commented Aug 2, 2023

@brettmc FYI open-telemetry/opentelemetry-specification#3563 is back in business, but it also supports the change. I would be thankful if you could review the specification PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working signal:metrics Related to metrics signal tc-review technical committee review feedback
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants