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

Add timeout to Exporter interface #203

Closed
snehilchopra opened this issue Jul 22, 2020 · 6 comments
Closed

Add timeout to Exporter interface #203

snehilchopra opened this issue Jul 22, 2020 · 6 comments
Labels
enhancement New feature or request

Comments

@snehilchopra
Copy link
Contributor

snehilchopra commented Jul 22, 2020

This issue concerns the addition of a timeout parameter to the exporter interface's Export function.

This can help propagate a timeout from the processor to the configured exporter, and test for timeouts in the exporter's Export method as well.

Please let me know your thoughts on this.
@reyang @pyohannes

@pyohannes
Copy link
Contributor

This is not part of the spec. I think this should go into the spec first we add this.

The spec implies that the Export function should have an implicit timeout:

Export() must not block indefinitely, there must be a reasonable upper limit after which the call must time out with an error result (Failure).

This could be set via the exporter constructor (which makes sense, because most likely one wants to have the same timeout for each call to Export).

However, I would prefer to leave this to vendor implementors to decide. For some a timeout might not make sense, they might just internally batch spans and asynchronously send those.

@pyohannes pyohannes added the enhancement New feature or request label Jul 22, 2020
@snehilchopra
Copy link
Contributor Author

snehilchopra commented Jul 23, 2020

Alright. I see your point.

However, if this is left to be vendor-specific, I believe the processor (batch or simple) doesn't need to take this into account in its testing suite, as any exporter could be configured to it?

I am talking with regard to #195 (specifically the TestForceFlushTimeout test) in which I have an implementation of the batch processor.

@pyohannes
Copy link
Contributor

I believe the processor (batch or simple) doesn't need to take this into account in its testing suite, as any exporter could be configured to it?

Yes, that's my understanding. From processor side, you can rely that Export() doesn't block indefinitely and there's a reasonable upper limit after which it returns.

There can be exporters that are badly implemented and block for a long time. In that instance, the batch processor will have to drop more spans.

@snehilchopra
Copy link
Contributor Author

Yes, that's my understanding. From processor side, you can rely that Export() doesn't block indefinitely and there's a reasonable upper limit after which it returns.

So if I am understanding this correctly, the processor can basically treat the exporter as a blackbox that returns after a certain upper time limit (which is vendor-specific). All we have to do is check how much time it takes for the exporter->Export() method to return, subtract that from our timeout (say from the ForceFlush's timeout) and consequently see if we timed out or not.

Is that right? Please correct me if I am wrong.

@snehilchopra
Copy link
Contributor Author

@pyohannes

@lalitb
Copy link
Member

lalitb commented Apr 19, 2021

This is not in the specs, and so not supported. As @pyohannes mentioned vendor can pass this as an option to the exporter constructor, and implement this timeout implicitly.

@lalitb lalitb closed this as completed Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants