Skip to content

fix: timeout being ignored in MeterProvider shutdown_with_timeout#3438

Open
partg952 wants to merge 1 commit intoopen-telemetry:mainfrom
partg952:add-timeout-duration-to-shutdown
Open

fix: timeout being ignored in MeterProvider shutdown_with_timeout#3438
partg952 wants to merge 1 commit intoopen-telemetry:mainfrom
partg952:add-timeout-duration-to-shutdown

Conversation

@partg952
Copy link
Copy Markdown
Contributor

@partg952 partg952 commented Mar 26, 2026

Fixes #2574

Changes

Passed down the ignored timeout argument in shutdown_with_timeout in MeterProvider

When calling the shutdown_with_timeout in MeterProvider the timeout was being ignored and not being passed down the incoming inner function calls
This PR fixes that by calling shutdown_with_timeout at each level and passing timeout along with it.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@partg952 partg952 requested a review from a team as a code owner March 26, 2026 15:52
@scottgerring
Copy link
Copy Markdown
Member

Hey @partg952 , thanks for opening the PR!
I think you need to plumb the timeout further still; for instance, for PeriodicReader the timeout is discarded:

fn shutdown_with_timeout(&self, _timeout: Duration) -> OTelSdkResult {
self.inner.shutdown()
}

and a 5 second duration is used instead:

fn shutdown(&self) -> OTelSdkResult {
self.shutdown_with_timeout(Duration::from_secs(5))
}

(ManualReader is fine as is).

I reckon with that fixed, we'd also want a changelog entry in opentelemetry-sdk, because the observed behaviour of the shutdown will change - we'll honour the API!

Could you also add a regression test that verifies the timeout is passed through?

Copy link
Copy Markdown
Member

@scottgerring scottgerring left a comment

Choose a reason for hiding this comment

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

left a comment above with some suggestions!

@partg952
Copy link
Copy Markdown
Contributor Author

Hey @scottgerring ,Thanks for taking a look , I will definitely edit it further and then add the regression test for it.

Thanks!

@partg952
Copy link
Copy Markdown
Contributor Author

Hey @partg952 , thanks for opening the PR! I think you need to plumb the timeout further still; for instance, for PeriodicReader the timeout is discarded:

fn shutdown_with_timeout(&self, _timeout: Duration) -> OTelSdkResult {
self.inner.shutdown()
}

and a 5 second duration is used instead:

fn shutdown(&self) -> OTelSdkResult {
self.shutdown_with_timeout(Duration::from_secs(5))
}

(ManualReader is fine as is).

I reckon with that fixed, we'd also want a changelog entry in opentelemetry-sdk, because the observed behaviour of the shutdown will change - we'll honour the API!

Could you also add a regression test that verifies the timeout is passed through?

Hey @scottgerring , I have a little unclarity here, in periodic reeader, the shutdown with timeout is calling inner.shutdown normally without respecting the timeout , so do you want me to implement the whole functionality or just pass it down through the consequent function calls?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ability to add timeout duration to shutdown

2 participants