Add general timeout mechanism in the export layer

[dgzlopes]

This PR implements:

• A general timeout mechanism in utils.py using signal. Example usage:

from opentelemetry.sdk.util import timeout_in_seconds
try:
  with timeout_in_seconds(5):
    func_example()
except TimeoutError:
  print('func_example took too long to complete')

Added set_timeout_signal_handler() for it's usage on multithreaded code (all signal handler functions must be set in the main thread).

• Timeouts to exports in SimpleExportSpanProcessor() and BatchExportSpanProcessor().

Closes #346

Signed-off-by: Daniel González Lopes [email protected]

[dgzlopes]

It's ok to have no timeout as the default timeout?

[codeboten]

I'd vote for a default timeout, maybe 60 sec that could return an warning to the user that something's gone wrong.

[dgzlopes]

Okay! 60s sounds like a reasonable time to wait :)

[codecov-io]

Codecov Report

Merging #385 into master will decrease coverage by 0.16%.
The diff coverage is 70.96%.

@@            Coverage Diff             @@
##           master     #385      +/-   ##
==========================================
- Coverage   85.22%   85.06%   -0.17%     
==========================================
  Files          38       38              
  Lines        1916     1942      +26     
  Branches      226      230       +4     
==========================================
+ Hits         1633     1652      +19     
- Misses        218      224       +6     
- Partials       65       66       +1

Impacted Files	Coverage Δ
...sdk/src/opentelemetry/sdk/trace/export/__init__.py	84.48% <60%> (-2.31%)	⬇️
opentelemetry-sdk/src/opentelemetry/sdk/util.py	85.14% <81.25%> (-0.74%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0173263...8a18978. Read the comment docs.

[ocelotl]

The idea of having a general purpose timeout mechanism is great, but keep in mind that the specification requires export and shutdown to not block indefinitely. This approach (using a @contextmanager to make a context object that is used when export and shutdown are called) puts the responsibility of being specification compliant on the code that does the calling of export and shutdown instead of putting the responsibility on these 2 methods themselves not to block indefinitely.

Please consider making a decorator that is then applied directly to the functions export and shutdown. In this way, they can be called directly and the caller won't have to worry about making them not block indefinitely, something like this:

def timeout(function):

    def inner(*args, timeout=90, **kwargs):

        print(timeout)

        function(*args, **kwargs)

    return inner


@timeout
def export(first, second):
    print(first)
    print(second)


export(1, 2, timeout=9)

9
1
2

[c24t]

Exporters might have a better way to implement timeouts, e.g. in the zipkin exporter we'd pass a timeout arg to requests.post which sets a timeout on the underlying socket.

I think a general purpose timeout in processor is fine, but exporters should implement timeout logic themselves: #346 (comment).

[ocelotl]

I think is better if you document the function attribute instead of using the name of the function as documentation for the attribute itself, something like this:

def timeout(time):
    """
    ...
    time: time to wait before timing out, in seconds
    """
    ...

[c24t]

@ocelotl you and @toumorokoshi can duke it out over hungarian notation. 😄

[ocelotl]

This is a function, there is no need to add _function to its name. The name of the function should describe what the function does, in this case, it raises a TimeoutError.

[ocelotl]

This function defines a inner function and then registers it into signal, but it is being called in an __init__. Keep in mind that you most likely want to do this only once, not every time that an object is initialized. Consider getting rid of this function and just do the registering into signal when this module is imported.

[c24t]

I think it's a great idea to have a general-purpose timeout in span processors, but I don't think we should do this with signals. As written, this is going to interfere with applications that use SIGALRM, e.g. by firing the signal after the instrumented app registered a different handler.

You could implement this instead using cancelable Futures or async awaitables.

[c24t]

For this to be a general-purpose timeout func, shouldn't it reset the old signal handler after cancelling the alarm?

[c24t]

It looks like you're setting the handler here because export runs in a worker thread? But since we initialize the span processor right at startup, this is going to change the SIGALRM handler for the entire life of the process, not just while we're waiting for an export to timeout.

[c24t]

We could still use a timeout like this, but not using SIGALRM. Closing for now, we can consider a different implementation later.