open-telemetry / open-telemetry/opentelemetry-python
Make timeout of `Exporter.export` configurable and decide how to update Exporter interfaces
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 2.6k
- Forks
- 1k
- Avg merge
- 4d 15h
- Merged PRs (30d)
- 19
Description
Is your feature request related to a problem?
Related to https://github.com/open-telemetry/opentelemetry-python/issues/4043 and https://github.com/open-telemetry/opentelemetry-python/issues/2284.
I would like to update Exporter.export to accept a timeout parameter. This way the BatchProcessor’s forceflush and shutdown can pass a timeout from the user (or a default) on to export. Right now the timeout passed to forceFlush is not used, and shutdown can hang for a few minutes if the exporters are failing/retrying.
In order to update Exporter.export I need to update the Exporter interfaces. When I looked into doing that I noticed the Exporter interfaces (trace,metrics, logs) are not in-sync with each other and not in line with the spec, it'd be good to fix that.
Describe the solution you'd like
The ideal Exporter interface to me is:
class Span/Log/MetricExporter(abc.ABC):
@abc.abstractmethod
def export(self, data: Metric/Span/Log, timeout_millis: Optional[int] = None, **kwargs) -> Metric/Log/TraceExportResult:
@abc.abstractmethod
def shutdown(self, timeout_millis: Optional[int] = 30000, **kwargs) -> None:
@abc.abstractmethod
def forceFlush(self, timeout_millis: Optional[int] = None, **kwargs) -> bool:
We use **kwargs to indicate that we may add additional params in the future.If **kwargs is added to the method signature and we add a param, the exporter isn’t broken.
I think we should update the existing interfaces to be as close to this as possible without breaking users:
Adding a param to an existing method is not breaking because @abc.abstractmethod does not enforce params, just that the method exists on the Class. It'd cause pylint errors, but IMO that is a good thing because it’ll notify people that they aren’t implementing the interface correctly.
Adding @abc.abstractmethod to an existing method is breaking (the SpanExporter interface doesn’t use abc.ABC at all which means there is nothing enforced on classes which implement it), adding a new method with the @abc.abstractmethod is also breaking (forceFlush method is missing from the LogExporter).
I think we should then also add v2 versions of the Span/LogExporter interface, and recommend people use those – and update any internal exporter’s use those. The MetricExporter is already almost exactly like my proposed ideal interface above..
Describe alternatives you've considered
Adding a new export_with_timeout method to the interfaces. I don't like this as much because it forces people to implement 2 methods, when 1 method with an additional param should be enough. Also MetricExporter.export already has the timeout param which makes this awkward..
Additional Context
No response
Would you like to implement a fix?
None
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by comparing the trace, metrics, and logs Exporter interfaces in the linked SDK files, then read related issues 4043 and 2284 and the referenced specification section. Determine a compatible timeout and interface-versioning approach, including effects on BatchProcessor forceflush and shutdown; the issue is complete when the interface changes and affected internal exporters are defined and covered by tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- observability-sre
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100