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

PeriodicReader.Shutdown now applies the periodic reader's timeout by default #4356

Merged
merged 4 commits into from
Jul 25, 2023

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Jul 24, 2023

From https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#shutdown,

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 authors MAY decide if they want to make the shutdown timeout configurable.

We don't currently have a timeout, so this PR adds a timeout to meet the specification.

We could hard-code the timeout, but I think it makes the most sense to re-use the timeout set with WithTimeout(), since Shutdown essentially is a collectAndExport call, which the shutdown is intended for. We can always add a separate timeout for Shutdown, as long as it defaults to the general timeout.

Fixes #3663

This is also relevant for #3666. For MetricExporter.Export:

Export MUST NOT block indefinitely, there MUST be a reasonable upper limit after which the call must time out with an error result (Failure).

That implies we should set a timeout on contexts we pass to Export, which this PR does.

@dashpole dashpole marked this pull request as ready for review July 24, 2023 17:22
@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #4356 (2c41d82) into main (d8d3502) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4356     +/-   ##
=======================================
- Coverage   83.4%   83.4%   -0.1%     
=======================================
  Files        184     184             
  Lines      14374   14376      +2     
=======================================
- Hits       12001   11999      -2     
- Misses      2145    2149      +4     
  Partials     228     228             
Files Changed Coverage Δ
sdk/metric/periodic_reader.go 84.1% <100.0%> (+0.1%) ⬆️

... and 2 files with indirect coverage changes

@dashpole dashpole force-pushed the shutdown_timeout branch 2 times, most recently from 450de18 to 0412711 Compare July 24, 2023 17:27
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Can we update WithTimeout documentation to include this behavior?

@dashpole
Copy link
Contributor Author

Can we update WithTimeout documentation to include this behavior?

Done

CHANGELOG.md Outdated Show resolved Hide resolved
@pellared pellared merged commit c1a644a into open-telemetry:main Jul 25, 2023
@MrAlias MrAlias modified the milestone: Metric SDK v0.40.0 Aug 3, 2023
@MrAlias MrAlias added this to the v1.17.0/v0.40.0 milestone Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Verify compliant metric SDK specification implementation: MetricReader/MetricReader operations/Shutdown
5 participants