opentelemetry-python icon indicating copy to clipboard operation
opentelemetry-python copied to clipboard

Improve timeout and retry mechanic of exporters

Open LarsMichelsen opened this issue 1 year ago • 3 comments

Description

I took a deep look into https://github.com/open-telemetry/opentelemetry-python/pull/3764, tried to separate the individual changes. While I think the approach of the PR in general is good, I changed some aspects of the change. For example, I don't agree with the change picking the lowest of the given timeouts (see here).

Having the change split into multiple steps, it's still a big undertaking. So I guess a reviewer will have to take some time to get into it. Nevertheless, I hope that someone will be found who feels up to it.

Fixes #4043

Type of change

Please delete options that are not relevant.

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] This change requires a documentation update

How Has This Been Tested?

Several tests were added (retry logic; shutdown logic; new timeout export argument) and of course executed also :)

Does This PR Require a Contrib Repo Change?

  • [ ] Yes. - Link to PR:
  • [x] No.

Checklist:

  • [x] Followed the style guidelines of this project
  • [x] Changelogs have been updated
  • [x] Unit tests have been added
  • [ ] Documentation has been updated

LarsMichelsen avatar Sep 08 '24 15:09 LarsMichelsen

I recognized the public symbols test being red (https://github.com/open-telemetry/opentelemetry-python/actions/runs/10760943700/job/29900823921). I understand it's purpose, but I guess there is some guidance needed on which names are acceptable to be new public names and which not. Any advice?

Regarding the changelog: Is it intended the I add the change log entry?

LarsMichelsen avatar Sep 11 '24 06:09 LarsMichelsen

I recognized the public symbols test being red (https://github.com/open-telemetry/opentelemetry-python/actions/runs/10760943700/job/29900823921). I understand it's purpose, but I guess there is some guidance needed on which names are acceptable to be new public names and which not. Any advice?

Regarding the changelog: Is it intended the I add the change log entry?

In that case we need a changelog for this one

emdneto avatar Sep 12 '24 00:09 emdneto

To my understanding the following new public symbols are necessary:

- exporter/opentelemetry-exporter-otlp-proto-common/src/opentelemetry/exporter/otlp/proto/common/exporter.py
	ExportResultT
	ExportPayloadT
	RetryableExportError
	RetryingExporter
	acquire_timeout

If I understood CONTRIBUTING.md correctly, I can not do anything further about it and now some maintainer / code owner needs to judge it.

LarsMichelsen avatar Sep 14 '24 16:09 LarsMichelsen

Closing since #4564 was merged

emdneto avatar Nov 17 '25 12:11 emdneto