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.

Sign up for GitHub

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

Include effects of force flush in Sdk shutdown #3085

Closed
wants to merge 3 commits into from
Closed

Include effects of force flush in Sdk shutdown #3085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea5e52f
Include effects of force flush in Sdk shutdown
legendecas Jan 9, 2023
3d0c761
fixup! Include effects of force flush in Sdk shutdown
legendecas Jan 13, 2023
4b954f7
Merge branch 'main' into metric-sdk-shutdown
legendecas Jan 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ release.
"base2_exponential_bucket_histogram". Mark exponential histogram data model and
base2 exponential histogram aggregation as stable.
([#3041](https://github.com/open-telemetry/opentelemetry-specification/pull/3041))
- Include effects of force flush in Sdk shutdown.
([#3085](https://github.com/open-telemetry/opentelemetry-specification/pull/3085))

### Logs

Expand Down
2 changes: 2 additions & 0 deletions specification/metrics/sdk.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -895,6 +895,8 @@ SHOULD return some failure for these calls, if possible.
`Shutdown` SHOULD provide a way to let the caller know whether it succeeded,
failed or timed out.

`Shutdown` SHOULD include the effects of `ForceFlush`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is ForceFlush?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This statement is identical to the statement in SpanProcessor.Shutdown in trace sdk. Do you suggest that we should clarify the subject of the ForceFlush (and update the trace sdk spec too)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, it's fair for SpanProcessor to say "Shutdown should include the effects of ForceFlush" because SpanProcessor has both Shutdown and ForceFlush. It is weird for MetricReader to say so because MetricReader doesn't have ForceFlush.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification. MetricReader.ForceFlush is being clarified at #3084, since it is already mentioned in the spec.


`Shutdown` SHOULD complete or abort within some timeout. `Shutdown` MAY be
implemented as a blocking API or an asynchronous API which notifies the caller
via a callback or an event. [OpenTelemetry SDK](../overview.md#sdk) authors MAY
Expand Down