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

Implement force_flush for span exporters #2872

Closed
lzchen opened this issue Aug 11, 2022 · 4 comments · Fixed by #2919
Closed

Implement force_flush for span exporters #2872

lzchen opened this issue Aug 11, 2022 · 4 comments · Fixed by #2919
Labels
bug Something isn't working tracing

Comments

@lzchen
Copy link
Contributor

lzchen commented Aug 11, 2022

From specs: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#forceflush

Flushing implementation is usually done by the span processor. This addition is just to be compliant with the specs. The only point of contention is if we make this an abstractmethod, it is a breaking change for anyone who is implementing their own span exporter. We probably shouldn't do this, even though it's fixing a "bug" since it is not spec compliant. Thoughts?

@lzchen lzchen added bug Something isn't working tracing labels Aug 11, 2022
@srikanthccv
Copy link
Member

I agree. On the other hand, how to enforce that the exporter implementation must have force_flush? If it's an abstract method then the processor can expect all implementations to have the method and call it. Otherwise, it needs to defensively handle it. I think it's not worth introducing breaking changes, but the processor should not contain hacky code because we got some things wrong with the base exporter class.

@lzchen
Copy link
Contributor Author

lzchen commented Aug 12, 2022

Can we just make force_flush a no-op in the base class and not make it an abstract method? That way we can still assume all exporters have the method but if they want to implement their own they can override it?

@srikanthccv
Copy link
Member

Looks like we don't have the @abstractmethod for the SpanExporter base class methods so this new addition with nop is fine. Do you know how base classes are divided into abstract classes that must be implemented vs nop to be overridden?

@lzchen
Copy link
Contributor Author

lzchen commented Aug 16, 2022

@srikanthccv

Do you know how base classes are divided into abstract classes that must be implemented vs nop to be overridden?

Good question. I'm not sure about the Pythonic preferred design principle but it seems like we only started implementing with @abstractmethod afterwards? i.e. we "felt like it". I believe @ocelotl was the first to start adding abstract base classes after seeing our classes were not designed properly haha.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working tracing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants