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

Document and implement honoring timeout in context passed to Readers #4166

Closed
2 of 5 tasks
MrAlias opened this issue Jun 2, 2023 · 0 comments · Fixed by #4267
Closed
2 of 5 tasks

Document and implement honoring timeout in context passed to Readers #4166

MrAlias opened this issue Jun 2, 2023 · 0 comments · Fixed by #4267
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Jun 2, 2023

The specification requires asynchronous callbacks to use a timeout to limit execution:

The implementation SHOULD use a timeout to prevent indefinite callback execution.

The idiomatic way to handle timeouts in Go is to pass them along via a context to the point of computation. This is already plumbed in, a context is accepted to Reader.Collect and used during processing by passing it to the callback functions. Support for timeouts in these context should be added and documented:

  • Document reader's Collect methods as honoring the timeout in the passed context.
  • Update the periodicReader to pass the export timeout it holds to all callbacks during regular periodic collect calls.
    • This will change the meaning of the "export timeout" to effectively refer to the whole export process instead of the explicit export part. It seems like this will be the user expected behavior. When they set the value they expect the collection and sending of data to take no more then the configured value, it would be unexpected if the collection took greater than the timeout time.

All callbacks are already documented that they need to honor the timeout they are passed, no update needs to be applied there:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants